Try to fix issue 19831 without understanding by copying the behavior from __dmd_personality_v0

[FeepingCreature]

NOT feeling good about this. Seems to work though???

Mostly opening this PR so tests get run.

This was not arrived at through any kind of insight, but mostly by flailing around.

Ping @donc . Help us Don Clugston, you're our only hope.

edit: @kinke ping? ("No. There is another.")

[dlang-bot]

Thanks for your pull request and interest in making D better, @FeepingCreature! We are looking forward to reviewing it, and you should be hearing from a maintainer soon.
Please verify that your PR follows this checklist:

• My PR is fully covered with tests (you can see the annotated coverage diff directly on GitHub with CodeCov's browser extension
• My PR is as minimal as possible (smaller, focused PRs are easier to review than big ones)
• I have provided a detailed rationale explaining my changes
• New or modified functions have Ddoc comments (with Params: and Returns:)

Please see CONTRIBUTING.md for more information.

---

If you have addressed all reviews or aren't sure how to proceed, don't hesitate to ping us with a simple comment.

Bugzilla references

Auto-close	Bugzilla	Severity	Description
✓	19831	major	throw/catch in scope(exit) crashes with illegal instruction

Testing this PR locally

If you don't have a local development environment setup, you can use Digger to test this PR:

dub fetch digger
dub run digger -- build "stable + druntime#2602"

[FeepingCreature]

oh no they're passing

[FeepingCreature]

Note from IRL: @ibuclaw looked it over and thinks it's basically sound, he might have some time tomorrow at the Hackathon to get it documented a bit better.

[donc]

Note that I only wrote the original Windows exception chaining, I don't know anything about how the Linux exception handler works. Does this bug apply to Windows as well?
BTW despite having implemented it, I think that exception chaining is a really, really bad idea. I don't think anybody would ever want to look at the exception chain, and it creates a host of problems.

[FeepingCreature]

Walter also said that exception chaining should be removed and we should go back to C++ and crashing on throw-in-finally.

I must strenuously object to at least that last part - scope(exit) is a core, heavily advertised feature of D and it'll look terrible (and basically make it unusable) if a basic feature like exceptions causes crashes inside it.

(I found this bug when I was looking at a case where I wanted to see the chained exception btw, and was upset it wasn't shown.)

[FeepingCreature]

Re Windows: I tried with wine+DMDwin32 and at least in that configuration, the problem does not occur.

[ibuclaw]

I think there needs to be a few more tests. See eh.d exception chaining tests, but adapt as you do here to trigger the bug.

[FeepingCreature]

I don't understand what you mean. eh.d exception chaining tests clearly did not trigger the bug. Should I just rewrite eh.d to be "eh.d, but every finally throws and catches an exception"?

You already can't catch in finally directly. I'm not sure how else you could order these elements to produce a test.

[FeepingCreature]

ping @ibuclaw

[ibuclaw]

@FeepingCreature there is a slight distinction between those tests and this test.

In eh.d you have:

void collideone()
{
    try
    {
        throw new Exception("x");
    }
    finally
    {
        throw new Exception("y");
    }
}

This test is:

void collideone()
{
    try
    {
        throw new Exception("x");
    }
    finally
    {
        funcThatThrowsAndCatches();
    }
}

I think that those tests could be copied to your new test file and amended as needed to check that the various weird chain cases and this mix well.

[FeepingCreature]

I'll take a look tomorrow.

[FeepingCreature]

@ibuclaw Is this better? It's hard to write comments and tests for code that I don't understand how it works. Since you said you did (:+1:), are the comments right like this?

[ibuclaw]

Seems reasonable, and tests look good.

[ibuclaw]

Note, if a function has been partitioned, this will also not match.

However this will never happen with dmd, and gdc turns off partitioning unless the optimization is explicitly requested on the command line.

[FeepingCreature]

....

throws up hands

I GUESS?!?

[kinke]

This broke runnable/eh.d for LDC [with its slight modifications in this file]. The testsuite passes again when NOT breaking here as long as currentLsd is null (if (currentLsd && currentLsd != ehn.languageSpecificData) ...), but I have no idea whether that's correct. @ibuclaw: any thoughts?

[ibuclaw]

Why would it be null?

[kinke]

AFAICT, each ExceptionHeader.languageSpecificData starts out with null and is only set after finding a matching handler/catch in the search phase (and only for D exceptions).

[ibuclaw]

Well the lsda is known since the beginning of the personality function, you could either eagerly set it, or pass it onto getClassInfo to initialize cuurentLsd with.

[kinke]

Yeah, the question is rather 'why does DMD have no problem with it'. I'll pass it through; pls ping me if GDC has problems too, so that we can come up with something here upstream.

[donc]

I would like to see a test where the exception is thrown in one function and caught in another. If this works, your PR has my blessing.

void thrower()
{
   throw new Exception("a");
}

void functhatthrowsandslowlycatches()
{
     try {
         thrower();
    }
    catch(Exception e){}
}

void test5()
{
   try {
      throw new Exception("x");
   }
   finally {  
      functhatthrowsandslowlycatches();
  }
}

[FeepingCreature]

@donc See test5. Seems to work.

[donc]

LGTM.

[thewilsonator]

As this fixes a severity major bug it should go in stable, unless you don't feel confident about doing so.