Skip to content

Fix lto0.test_exceptions_allowed_uncaught failure#19165

Merged
sbc100 merged 1 commit intomainfrom
fix_lto_exceptions_test
Apr 12, 2023
Merged

Fix lto0.test_exceptions_allowed_uncaught failure#19165
sbc100 merged 1 commit intomainfrom
fix_lto_exceptions_test

Conversation

@sbc100
Copy link
Copy Markdown
Collaborator

@sbc100 sbc100 commented Apr 12, 2023

This test started failing after #16627. Prior to that change the exceptions destructors were called via the following code in JS:

   // In Wasm, destructors return 'this' as in ARM
   {{{ makeDynCall('pp', 'destructor') }}}(info.excPtr);

For some reason the LTO build produces the following for for the call
to exception_header->exceptionDestructor(thrown_object) in
__cxa_decrement_exception_refcount:

  call_indirect 0 (type 0)                                               

Where as the normal non-LTO build produces:

  call 13 <invoke_ii>                                                    

Because invoke_ii goes via JS it uses the sloppy type checking and
doesn't trap, but call_indirect has strict type checking and so
does trap.

@sbc100 sbc100 requested a review from RReverser April 12, 2023 18:06
@sbc100 sbc100 force-pushed the fix_lto_exceptions_test branch from e4c6dd2 to e595b62 Compare April 12, 2023 18:06
@sbc100 sbc100 requested a review from aheejin April 12, 2023 18:08
This test started failing after #16627.  Prior to that change the
exceptions destructors were called via the following code in JS:

```
   // In Wasm, destructors return 'this' as in ARM
   {{{ makeDynCall('pp', 'destructor') }}}(info.excPtr);
```

For some reason the LTO build produces the following for for the call
to `exception_header->exceptionDestructor(thrown_object)` in
`__cxa_decrement_exception_refcount`:

```
  call_indirect 0 (type 0)
```

Where as the normal non-LTO build produces:

```
  call 13 <invoke_ii>
```

Because invoke_ii goes via JS it uses the sloppy type checking and
doesn't trap, but `call_indirect` has strict type checking and so
does trap.
@sbc100 sbc100 force-pushed the fix_lto_exceptions_test branch from e595b62 to 7920939 Compare April 12, 2023 18:35
@sbc100
Copy link
Copy Markdown
Collaborator Author

sbc100 commented Apr 12, 2023

I figured out the problem. See the new description.

@sbc100 sbc100 requested a review from dschuff April 12, 2023 18:40
@RReverser
Copy link
Copy Markdown
Collaborator

Thanks, I see now. So technically signature was incorrect even before the PR, but because it was used only from JS, it didn't matter?

@sbc100 sbc100 merged commit aa860ca into main Apr 12, 2023
@sbc100 sbc100 deleted the fix_lto_exceptions_test branch April 12, 2023 22:39
Comment on lines +27 to +28
// In wasm, destructors return 'this' as in ARM
void* (*exceptionDestructor)(void *);
Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I remember adding this years ago... I guess it was somehow lost during library updates. https://github.com/aheejin/emscripten/blob/d57db5bea1719319a680699c50b91fa3d88fa0ec/system/lib/libcxxabi/src/cxa_exception.hpp#L41-L46

By the way, wouldn't it be better to add ifdef as in the old code, in case when we try to upstream our changes?

Copy link
Copy Markdown
Collaborator Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This entire block is already within #ifdef __USING_EMSCRIPTEN_EXCEPTIONS__.

BTW, is there some way we can undef the destructors return 'this' as in ARM decision, since it makes all these patches needed? Is there some reason we need this behaviour?

Copy link
Copy Markdown
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Maybe? AFAIK @sunfishcode added that way back at the beginning, as a performance optimization. I don't know how much impact it actually has.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

4 participants