www.digitalmars.com         C & C++   DMDScript  

digitalmars.D.bugs - [Issue 904] New: Bad code generated for local _assert routine

reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=904

           Summary: Bad code generated for local _assert routine
           Product: D
           Version: 1.00
          Platform: PC
        OS/Version: Windows
            Status: NEW
          Keywords: wrong-code
          Severity: normal
          Priority: P2
         Component: DMD
        AssignedTo: bugzilla digitalmars.com
        ReportedBy: sean f4.ca


Given the code:

    extern (C) void _d_assert( char[] file, uint line );

    void call_assert( char[] file, uint line )
    {
        _d_assert( file, line );
    }

    void main()
    {
        call_assert( "a", 1 );
        assert( false );
    }

The code generation for call_assert and the automatically generated _assert
routine should be identical, but they aren't:

    _D4test11call_assertFAakZv      comdat
            assume  CS:_D4test11call_assertFAakZv
    L0:             push    EAX
                    push    dword ptr 0Ch[ESP]
                    push    dword ptr 0Ch[ESP]
                    call    near ptr __d_assert
                    add     ESP,0Ch
                    ret     8
    _D4test11call_assertFAakZv      ends
    _D4test8__assertFiZv    comdat
            assume  CS:_D4test8__assertFiZv
    L0:             push    EAX
                    push    dword ptr FLAT:_DATA[01Ch]
                    push    dword ptr FLAT:_DATA[018h]
                    call    near ptr __d_assert
                    ret
    _D4test8__assertFiZv    ends

You'll notice that the implicitly generated _assert routine doesn't clean up
its stack on exit.  This is fine for the normal case where an exception is
thrown from assert(), but any attempt to override _d_assert to behave otherwise
will resunt in an access violation.


--
Jan 29 2007
next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=904


bugzilla digitalmars.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |RESOLVED
         Resolution|                            |INVALID





The _d_assert() should never return, so there is no need to clean up after it.
It's a compiler support routine, and should not be overridden with something
that returns.


--
Jan 30 2007
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=904


sean f4.ca changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|RESOLVED                    |REOPENED
         Resolution|INVALID                     |





But surely that is no reason to generate invalid code.  What if the user wants
to halt the debugger on an assert via int 3, and optionally continue
afterwords?  Or report assertion failures during unit testing without littering
the code with try/catch blocks?  I am concerned because this change broke code
that has been in place and working for two years, and there seems no rational
explanation for the change.


--
Jan 31 2007
prev sibling next sibling parent d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=904






It also seems to contradict: http://www.digitalmars.com/techtips/unittests.html

"Provide a custom implementation of:
        extern (C) void _d_assert(char[] filename, uint line);

to do the logging. _d_assert is the function called when an assert trips. By
providing your own, it prevents the Phobos library version from being linked
in."


--
Jan 31 2007
prev sibling parent reply d-bugmail puremagic.com writes:
http://d.puremagic.com/issues/show_bug.cgi?id=904


bugzilla digitalmars.com changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|REOPENED                    |RESOLVED
         Resolution|                            |INVALID





It's not invalid code. The function is never supposed to return, therefore, it
doesn't need any cleanup code. Replacing it with a function that does return
will break use of asserts that assume that failed asserts don't continue.
Changing behavior globally is often a problem because of 3rd party code linked
in that assumes the defined behavior.

Inserting your own logging code doesn't change this, it should log and then not
return.

The optimizer assumes tripped asserts don't return, so having it return will
introduce some subtle bugs.

The local assert function has always been like this, it was probably a fluke
that it appeared to work in the past.

Logging errors and continuing is not what assert is for, so something else
should be used for that purpose.


--
Jan 31 2007
parent Sean Kelly <sean f4.ca> writes:
d-bugmail puremagic.com wrote:

 It's not invalid code. The function is never supposed to return, therefore, it
 doesn't need any cleanup code. Replacing it with a function that does return
 will break use of asserts that assume that failed asserts don't continue.
 Changing behavior globally is often a problem because of 3rd party code linked
 in that assumes the defined behavior.


This is a valid point.  I do think there is some value in providing a 
returning assert handler for debugging purposes, but I won't press the 
matter.  However, I remain somewhat concerned that limiting the behavior 
of the assert handler in this manner will inspire the use of custom 
error signaling routines and reduce the overall utility of assert, but 
perhaps this is unwarranted.


 The optimizer assumes tripped asserts don't return, so having it return will
 introduce some subtle bugs.


Valid as well.  I suppose I'll leave the ability to supply a custom 
assert handler in place and simply document that it must either throw or 
terminate the program.
Jan 31 2007