[D-runtime] win32 regression in exception handling

[Jonathan M Davis]

On Wednesday 26 January 2011 01:11:10 Don Clugston wrote:
> On 26 January 2011 09:33, Jonathan M Davis <jmdavisProg at gmx.com> wrote:
> > On Wednesday 26 January 2011 00:25:34 Brad Roberts wrote:
> >> On 1/24/2011 8:00 PM, Brad Roberts wrote:
> >> > Now that the dust of the svn -> git migration has settled some,
> >> > there's a failure that's stuck with us.  Since this druntime change,
> >> > the win32 dmd test34.d test has failed:
> >> > 
> >> > https://github.com/D-Programming-Language/druntime/commit/559df80b3d9c
> >> > 512 72c5804de8df27af75099b977
> >> > 
> >> > http://d.puremagic.com/test-results/test_data.ghtml?dataid=43441
> >> > 
> >> >   Before test 3:
> >> >   object.Error at src\rt\deh.d(631): Access Violation
> >> > 
> >> > I did a test build at the previous commit and the test passes.  So
> >> > that's definitely the one that introduces the problem.  It might well
> >> > be just exposing a dormant problem, but either way, it stopped
> >> > working at that point.
> >> 
> >> Ok.. I finally took some time to dive into this one.  It's failing on a
> >> catch (DerivedFromError) situation.
> >> 
> >> From test34.d:
> >> 
> >> class B58
> >> {    long x;
> >>      void set(long i) { writeln("B58::set(long)"); x = i; }
> >>      void set(int i)  { writeln("B58::set(int)"); x = i; }
> >>      long squareIt()  { writeln("B58::squareit()"); return x * x; }
> >> }
> >> class D58 : B58
> >> {
> >>      long square;
> >>      void set(long i) { writeln("D58::set(long)"); B58.set(i); square =
> >> x * x; } long squareIt()  { writeln("D58::squareit()"); return square;
> >> } }
> >> 
> >> long foo58(B58 b)
> >> {   int i;
> >>     try
> >>     {
> >>         b.set(3);
> >>     }
> >>     catch (HiddenFuncError o)
> >>     {
> >>         i = 1;
> >>     }
> >>     assert(i == 1);
> >>     return b.squareIt();
> >> }
> >> 
> >> void test58()
> >> {
> >>     writeln(foo58(new D58));
> >> }
> >> 
> >> Catching Error derived classes is illegal-ish now, right?  I haven't
> >> narrowed it down to exactly where the segv is coming from (the line
> >> number above is the part of the eh mechanism that's constructing the
> >> error object in the SEH receiving code, not the point of the segv..
> >> unless that line of code is segving somehow.
> >> 
> >> Don, do you have the time to dig a little since you've been all over
> >> that part of the code recently?
> > 
> > As I understand it, catching Errors is perfectly legal, but because none
> > of the proper cleanup is going to have been done between the point that
> > Error was thrown from and the point that it was caught it, the state of
> > things might be a bit messed up. The unit testing functions that I wrote
> > which are currently under review catch Errors, and they'd be somewhat
> > crippled if they couldn't.
> > 
> > 
> > 
> > Catching Errors is not generally a good idea, but as far as I understand
> > it, it's supposed to be legal.
> 
> Yes, that's correct, it's legal to catch errors.
> There is an obvious disastrous problem with that commit: the inclusion
> of __FILE__ and __LINE__ in the constructors for Error and Throwable
> is completely wrong. They should not be in there. That assumes that
> the exceptions are thrown via a throw statement, which is not correct,
> and will cause serious problems. Dunno if that's the only problem,
> though.
> Sorry, don't have any more time right now.

How else would an error be thrown? I was suprised that they weren't there in the 
first place. Any exception types that I've ever written do that. Theoretically, 
exceptions are supposed to always give the file and line number that they came 
from, so it makes perfect sense to me that they'de have default paramaters for 
it.

If it's really a problem, then it should be changed. But obviously I'm 
misunderstanding something here, because I would have thought that that would be 
the desired behavior, and I have no idea how you'd ever get an exception thrown 
without a throw statement.

- Jonathan M Davis