[D-runtime] win32 regression in exception handling

[Walter Bright]

Brad Roberts wrote:
> On 1/26/2011 10:23 PM, Walter Bright wrote:
>   
>> Jonathan M Davis wrote:
>>     
>>> On Wednesday 26 January 2011 21:32:13 Brad Roberts wrote:
>>>  
>>>       
>>>> On 1/26/2011 11:19 AM, Brad Roberts wrote:
>>>>    
>>>> Fixed enough to not crash any more.  The problem is that while there are
>>>> default args on the function, the caller is always responsible for setting
>>>> up the stack correctly.  The calls to the HiddenFuncError code are all
>>>> compiler generated and only generates the one original parameter right
>>>> now.  So I reverted back to the version before the changes.
>>>>
>>>> I don't know that we're done with this discussion.. but at least the tests
>>>> are passing once again.
>>>>     
>>>>         
>>> Well, I'm not sure that I entirely understand what the problem is, but it sounds like the problem relates to the
>>> extern(C) functions in particular. So, perhaps all of those should be reverted. Proper D code can use the Error types
>>> directly if it likes, and then I would expect that the default arguments would work just fine. It sounds like the
>>> default arguments are causing problems in cases where what we're dealing with is on a low enough level that the normal
>>> D rules don't necessarily apply. I did verify that default arguments worked with extern(C) before checking in those
>>> changes, but apparently what I tested wasn't good enough. Perhaps it's because I then called the extern(C) functions
>>> from D rather than C or C++. That might explain why I missed it.
>>>
>>> So, does it sound like it would just be a good idea to revert the changes on the extern(C) functions? I'd rather be
>>> safe than sorry, and while I do think that in general, it's best to have the file and line arguments for exception
>>> types have default arguments, if we're talking about low level and/or C/C++ code which is dealing with it, it might be
>>> best to just play it safe and not have the default arguments on the extern(C) funchtions.
>>>
>>>   
>>>       
>> I think it's better to figure out exactly what went wrong rather than all the guessing going on. Is it a mismatch
>> between calling conventions between the caller and the callee? Just what instruction was the seg fault on?
>>     
>
> I wasn't guessing, though potentially misunderstanding.  It _is_ the caller not the callee that is required to deal with
> all args, including optional, right?
>
> That said, I was slightly off, but only slightly.  The compiler isn't creating the exception class directly, but
> indirectly by way of a runtime function call.
>
> The compiler creates a call site like:
>
> push object
> call _d_hidden_func
>
> in rt/dmain2.d it lands in:
>
> extern (C) void _d_hidden_func()
>
> That function's a tad odd.. no parameters declared, though it receives one.  It's asm code recognizes this and moves it
> from [E|R]AX to a local 'o'.  It then calls onHiddenFuncError(o).  That function is extern'ed at the top of the file as:
>
> extern (C) void onHiddenFuncError(Object o);
>
> The _real_ function is declared over in core/exception.d as:
>
> extern (C) void onHiddenFuncError( Object o ) { ... }
>
> That function in the big round of exception changes was changed to take additional optional parameters.  The extern
> declaration in dmain2.d wasn't changed, however, so the call resulted in garbage data for the new parameters.
>
> Those are passed directly into the ctor for HiddenFuncError.  Inside the ctor for HiddenFuncError were passed to Error's
> ctor by way of a super(...) call.  The appropriate Error ctor passes them straight through to Throwable's ctor where
> they're stored.  I stopped short of finding where the bad data was used, since I was sure that the behavior was wrong.
> Reverting the extra parameters so that the two declarations matched and all is well with the world, or at least that
> corner of it.  An alternate fix might well have been to sync up the declarations, but the file and line number would
> have been useless anyway.  They'd always be the landing code in the rt not the point of the problem.  Not hard to fix,
> but would require changing dmd and the runtime api.
>
> If you want to look at a similar case, the SwitchError handling is still broken, which is what's causing test4 to fail.
>  It's the one you commented on a couple days ago.  Now that test34.d is passing due to the revert I did, the win 32 auto
> tester is failing on it.  Feel free to dig into it. :)
>
>   

Ok, that's a great analysis. Here is what is going on from the compiler 
perspective. The compiler replaces addresses of virtual functions in the 
vtbl[] with &_d_hidden_func. The point is so that an exception can be 
thrown for a hidden function. The only argument that is guaranteed to be 
passed is the object reference as the first parameter.

If more parameters are desired for onHiddenFuncError(), they must be 
added by _d_hidden_func. There is no way for _d_hidden_func to get the 
file/line from whence it came.