Bug 21013 – dwarfeh: Comparing LSDA is too unreliable to determine if two exceptions can be merged

Status
NEW
Severity
major
Priority
P2
Component
druntime
Product
D
Version
D2
Platform
All
OS
Linux
Creation time
2020-07-05T13:11:52Z
Last change time
2024-12-07T13:40:27Z
Keywords
GDC
Assigned to
No Owner
Creator
Iain Buclaw
Moved to GitHub: dmd#17189 →

Comments

Comment #0 by ibuclaw — 2020-07-05T13:11:52Z
The run-time implementation of D EH personality routines tries to cater for merging two in-flight exceptions. In order to do this for Dwarf/Unwind EH, it assumes that each function will have only one LSDA, and it will be unique to that function. See [1] for combining two in-flight exceptions before switching to the handler, and [2] for determining which of the in-flight Exceptions takes precedence when search for a catch/finally handler. [1] https://github.com/dlang/druntime/blob/d3dfa0778fbad77482b0ae8e7e528b55aa417c19/src/rt/dwarfeh.d#L413-L418 [2] https://github.com/dlang/druntime/blob/d3dfa0778fbad77482b0ae8e7e528b55aa417c19/src/rt/dwarfeh.d#L479-L485 -------------------------------------------------------- Line [1] Breaks if the function is partitioned into two. e.g: --- void bug1513a() { throw new Exception("d"); } void bug1513() { try { try { bug1513a(); } finally { throw new Exception("f"); } } catch(Exception e) { assert(e.msg == "d"); // <-- Assertion failure here assert(e.next.msg == "f"); assert(!e.next.next); } } --- No combining happens because there are two LSDA's for bug1513(). --- _D4test7bug1513FZv: push rbp mov rbp, rsp call _D4test8bug1513aFZv jmp .L5 // // LSDA for bug1513() // _D4test7bug1513FZv.cold: .L5: ... // // LSDA for bug1513.cold() // --- -------------------------------------------------------- Line [2] breaks if one function is inlined into another. e.g: --- void test4() { void throw_catch() { try { throw new MyException; } catch (MyException) { } catch (Exception) { assert(false); // <-- Assertion failure here } } try { try { throw new Exception("a"); } finally { throw_catch(); } } catch(Exception e) { assert(e.msg == "a"); assert(!e.next); } } --- The function throw_catch() is inlined into its parent, and so both now share the same LSDA. This means that the catch handler for MyException is now ignored because there's already an in-flight Exception that takes precedence because they appear to be thrown from the same function.
Comment #1 by ibuclaw — 2020-07-05T13:37:46Z
One stable input that could instead be used is the function FQN, or perhaps better, the hash of it. This cannot be done by changing the constructor signature of Exception/Error to: @nogc @safe pure nothrow this(string msg, string file = __FILE__, size_t line = __LINE__, Throwable nextInChain = null, string func = __PRETTY_FUNCTION__); This does not work, as __FUNCTION__ will be the name of the constructor that super() was called, not the function of the originating throw statement. (Not to mention that it breaks far to much user code to do so). So the _d_throw function signature must change instead (I'm using a string here for example purposes, a hash_t would be better IMO): void _d_throw(Throwable object, string func); So the calling code: throw new Exception("msg"); Gets lowered as: _d_throwX(Exception.__ctor(_d_newclass(&Exception.__class)), "foo.bar"); And the ExceptionHeader created in _d_throwX saves this information: ExceptionHeader *eh = ExceptionHeader.create(object, func); The two referenced lines in the D personality functions would then be updated as presented below: [1] --- // Don't combine when the exceptions are from different functions if (eh.func != ehn.func) { //printf("break: %s %s\n", eh.func.ptr, ehn.func.ptr); break; } --- [2] --- // like __dmd_personality_v0, don't combine when the exceptions are // from different functions (fixes issue 19831, exception thrown and // caught while inside finally block) if (eh.func != ehn.func) { // printf("break: %s %s\n", eh.func.ptr, ehn.func.ptr); break; } ---
Comment #2 by ibuclaw — 2020-07-05T14:04:48Z
(In reply to Iain Buclaw from comment #1) > One stable input that could instead be used is the function FQN, or perhaps > better, the hash of it. > Though shortly after sending the last message, I now see that I was thinking only about problem [2], and not what would also work for [1] as well. What instead works for both would be if an internal state were to be updated when entering each try/catch or try/finally region. e.g: --- void bug1513a() { throw new Exception("d"); } void bug1513() { try { /// _d_eh_setContext("bug1513"); /// try { /// _d_eh_setContext("bug1513"); /// bug1513a(); } finally { throw new Exception("f"); } } catch(Exception e) { assert(e.msg == "d"); assert(e.next.msg == "f"); assert(!e.next.next); } } --- void test4() { void throw_catch() { try { /// _d_eh_setContext("test4.throw_catch"); /// throw new MyException; } catch (MyException) { } catch (Exception) { assert(false); } } try { /// _d_eh_setContext("test4"); /// try { /// _d_eh_setContext("test4"); /// throw new Exception("a"); } finally { throw_catch(); } } catch(Exception e) { assert(e.msg == "a"); assert(!e.next); } } --- Which means there's unfortunately going to be a heavy performance penalty for using EH.
Comment #3 by kinke — 2020-07-05T15:26:32Z
Excellent findings. I think that's finally the explaination for one remaining unittest failure with 32-bit ARM a few years back (std.random IIRC; could be worked around by disabling inlining). I'm still not convinced exception chaining is worth all these troubles.
Comment #4 by robert.schadek — 2024-12-07T13:40:27Z
THIS ISSUE HAS BEEN MOVED TO GITHUB https://github.com/dlang/dmd/issues/17189 DO NOT COMMENT HERE ANYMORE, NOBODY WILL SEE IT, THIS ISSUE HAS BEEN MOVED TO GITHUB