Page MenuHomePhabricator

[WebAssembly] Do not use EHCatchret symbols with wasm EH
ClosedPublic

Authored by dschuff on Feb 16 2021, 4:56 PM.

Details

Summary

D94835 (revision 080866470d3e ) added support for WinEH to export public symbols pointing to
basic blocks which are catchret targets for use with Windows CET.
Wasm currently doesn't support public symbols to non-function code
addresses (they get treated like new functions in asm but then don't
lower to object files correctly).
It created them unconditionally for all catchret targets.

This change disables those symbols unless the exceptionHandlingType
is WinEH (since they aren't used with ExceptionHandling::Wasm)

Diff Detail

Event Timeline

dschuff created this revision.Feb 16 2021, 4:56 PM
dschuff requested review of this revision.Feb 16 2021, 4:56 PM
Herald added a project: Restricted Project. · View Herald TranscriptFeb 16 2021, 4:56 PM
dschuff edited the summary of this revision. (Show Details)Feb 16 2021, 4:57 PM

This test is basically copied from llvm/test/CodeGen/X86/win-catchpad-nested.ll which is a configuration that triggers the labels. It doesn't have the itanium-personality stuff that wasm EH code usually does. But does it look like a sane test?

aheejin accepted this revision.Feb 17 2021, 5:52 AM

Thanks for taking care of this. I think the test case is fine, but it'd better to make it a valid wasm EH file, because in the current state does not generate a valid wasm code. I made edits to do that so you can just apply them.

llvm/test/CodeGen/WebAssembly/eh-catchpad-nested.ll
6 ↗(On Diff #324136)
10 ↗(On Diff #324136)
21 ↗(On Diff #324136)
%cp1 = catchpad within %cs1 [i32 1]
%0 = call i8* @llvm.wasm.get.exception(token %cp1)
%1 = call i32 @llvm.wasm.get.ehselector(token %cp1)
28 ↗(On Diff #324136)

newline

30 ↗(On Diff #324136)
%cp2 = catchpad within %cs2 [i32 2]
%2 = call i8* @llvm.wasm.get.exception(token %cp2)
%3 = call i32 @llvm.wasm.get.ehselector(token %cp2)
This revision is now accepted and ready to land.Feb 17 2021, 5:52 AM

Sorry, on second thought, I don't even think we need a new test case for this. All existing exception-using test cases will show the same behavior, so I think you can add

; Check that no $ehgcr symbols are emitted
; CHECK-NOT: $ehgcr

to somewhere like test/CodeGen/WebAssebly/exception.ll.

Ah, you are right, I initially thought that we needed a new test because none of ours failed when their patch went in. But the reason for that was that the output is all assembly rather than object files.
I added --implicit-check-not to exception.ll and that does catch the error.

dschuff updated this revision to Diff 324359.Feb 17 2021, 11:05 AM

Update existing test instead of using new one

This revision was landed with ongoing or failed builds.Feb 17 2021, 11:23 AM
This revision was automatically updated to reflect the committed changes.