This is an archive of the discontinued LLVM Phabricator instance.

[WebAssembly] LSDA info generation
ClosedPublic

Authored by aheejin on Oct 1 2018, 2:07 PM.

Details

Summary

This adds support for LSDA (exception table) generation for wasm EH.
Wasm EH mostly follows the structure of Itanium-style exception tables,
with one exception: a call site table entry in wasm EH corresponds to
not a call site but a landing pad.

In wasm EH, the VM is responsible for stack unwinding. After an
exception occurs and the stack is unwound, the control flow is
transferred to wasm 'catch' instruction by the VM, after which the
personality function is called from the compiler-generated code. (Refer
to WasmEHPrepare pass for more information on this part.)

This patch:

  • Changes wasm.landingpad.index intrinsic to take a token argument, to make this 1:1 match with a catchpad instruction
  • Stores landingpad index info and catch type info MachineFunction in before instruction selection
  • Lowers wasm.lsda intrinsic to an MCSymbol pointing to the start of an exception table
  • Adds WasmException class with overridden methods for table generation
  • Adds support for LSDA section in Wasm object writer

Diff Detail

Event Timeline

aheejin created this revision.Oct 1 2018, 2:07 PM
aheejin updated this revision to Diff 167831.Oct 1 2018, 2:10 PM

Fix encoding

aheejin edited the summary of this revision. (Show Details)Oct 1 2018, 2:11 PM
aheejin edited the summary of this revision. (Show Details)
aheejin updated this revision to Diff 167833.Oct 1 2018, 2:17 PM
  • Add comment
aheejin updated this revision to Diff 167839.Oct 1 2018, 3:01 PM

Fix comments

sbc100 added inline comments.Oct 1 2018, 3:04 PM
lib/CodeGen/AsmPrinter/EHStreamer.cpp
610

This seems a little off, could remove that requirement?

If we can't then perhaps invert this check we we only have a single return statement?

if (!IsWasm)
  Asm->EmitAlignment(2);
return GCCETSym;
test/CodeGen/WebAssembly/eh-lsda.ll
45

Ha.. I didn't know that assembler could be used to encode LEBs like this.

I'm assuming this assembly value (i.e. doesn't generate a relocations?)

aheejin added inline comments.Oct 1 2018, 3:30 PM
lib/CodeGen/AsmPrinter/EHStreamer.cpp
610

Yes it seemed a little off to me :( We can remove it, but it requires more aggressive refactoring. So basically the problem is, while it is better to move wasm-specific functionalities to WasmException class like as I did for WasmException::computeCallSiteTable, wasm can completely reuse the existing emitExceptionTable, except it requires to emit something before the final alignment directive. To refactor this, we should probably split this emitExceptionTable into 2-3 different functions, and make WasmException call only relevant functions from them. What do you think? Do you think it's worth it?

test/CodeGen/WebAssembly/eh-lsda.ll
45

Yeah it doesn't generate a relocation.

dschuff added inline comments.Oct 2 2018, 2:57 PM
lib/CodeGen/AsmPrinter/WasmException.cpp
31

Can this be const LandingPadInfo& instead of const auto for more clarity?

42

Is there some particular reason for this or is it just a limitation of our assembler and/or MC?

dschuff added inline comments.Oct 2 2018, 3:15 PM
lib/CodeGen/AsmPrinter/EHStreamer.cpp
610

If it causes a lot of ugliness, there's actually no reason we need to emit the size before the alignment; it could go after. The only difference is whether the calculated size of the object includes or doesn't include the padding that the align directive might emit. (This would mean that when emit the table in the object file, the resulting segment might contain up to 3 bytes of padding on the end instead of having a gap between the segments, but that's not the end of the world).

aheejin updated this revision to Diff 168545.Oct 5 2018, 3:08 PM
aheejin marked an inline comment as done.
  • const auto -> const LandingPadInfo&
lib/CodeGen/AsmPrinter/WasmException.cpp
42

Looks like wasm object writer & obj2yaml are structured to save sizes for data symbols. The size information is also shown in obj2yaml. I guess this info is used in the symbol table, and not elsewhere (so the final binary works without this info). We can do away with setting size for now if we really want, but I'm not sure if it's good to modify the symbol table structure, and the size info might be needed later for debug info or something..? @sbc100, what do you think?

aheejin updated this revision to Diff 168803.Oct 9 2018, 8:12 AM
  • Add original C++ code comment to test case
aheejin added a reviewer: rnk.Oct 9 2018, 1:28 PM
aheejin added inline comments.
lib/CodeGen/AsmPrinter/WasmException.cpp
42

Ping. If we agree that we shouldn't remove size info from the symbol table, I'll probably go ahead and emit the symbol after the alignment, as @dschuff said.

sbc100 added a comment.Oct 9 2018, 2:12 PM

Sorry for not replying earlier. Yes, we should generate and preserve size information for data symbols if possible.

Is that information what's used/needed to allow the linker to create a separate wasm segment per global?

sbc100 added a comment.Oct 9 2018, 3:10 PM

Is that information what's used/needed to allow the linker to create a separate wasm segment per global?

No, the linker operates on the input segments.. which normally map 1-to-1 with symbols (We can in theary support -fno-data-segments which would result in many symbols in a single segment, but in that case the linker cannot GC any of those symbols individually).

The size is mostly for compatibility with 'nm' and other tools that list symbols with their sizes.

aheejin updated this revision to Diff 168910.Oct 9 2018, 4:57 PM
  • Emit .size after .p2align
aheejin marked an inline comment as done.Oct 9 2018, 4:59 PM
dschuff accepted this revision.Oct 11 2018, 1:23 PM
dschuff added inline comments.
lib/CodeGen/AsmPrinter/WasmException.h
24

I realize this naming convention is pre-existing but ༼ ༎ຶ ෴ ༎ຶ༽

This revision is now accepted and ready to land.Oct 11 2018, 1:23 PM
aheejin added inline comments.Oct 11 2018, 1:31 PM
lib/CodeGen/AsmPrinter/WasmException.h
24

Yeah ***Exception inheriting from EHStreamer looks really confusing...

rnk accepted this revision.Oct 11 2018, 2:10 PM

Looks good, I had one style comment. Glad to see this coming together. :)

lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
1170–1195

I think this code would be more readable if you factor out the body of this if into a helper and give it some name, like mapWasmLandingPadIndex(MBB, CPI). The loop to iterate over all uses is a bit complicated, but the overall effect is pretty simple.

1186

I see, so this intrinsic only exists to associate this landing pad index number with the catchpad. If we had some other way to staple the number onto catchpad, that would be nicer. This seems like an OK way to do this.

aheejin added inline comments.Oct 11 2018, 3:02 PM
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
1186

Do you think I can put the index number in a bundle argument to catchpad? Like, a named bundle operand "index".

aheejin updated this revision to Diff 169761.Oct 15 2018, 3:37 PM
  • Factor out mapWasmLandingPadIndex
aheejin marked an inline comment as done.Oct 15 2018, 3:38 PM
aheejin added inline comments.
lib/CodeGen/SelectionDAG/SelectionDAGISel.cpp
1186

Operand bundles seem to only can be attached to CallInst and InvokeInst, so this is not gonna work.

This revision was automatically updated to reflect the committed changes.