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.


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=904


bugzilla@digitalmars.com changed:

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




------- Comment #1 from bugzilla@digitalmars.com  2007-01-31 01:53 -------
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.


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=904


sean@f4.ca changed:

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




------- Comment #2 from sean@f4.ca  2007-01-31 02:02 -------
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.


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=904





------- Comment #3 from afb@algonet.se  2007-01-31 02:45 -------
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."


-- 

http://d.puremagic.com/issues/show_bug.cgi?id=904


bugzilla@digitalmars.com changed:

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




------- Comment #4 from bugzilla@digitalmars.com  2007-01-31 03:02 -------
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.


-- 

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.