This is an archive of the discontinued LLVM Phabricator instance.

[libc++abi][WebAssembly] Support Wasm EH
ClosedPublic

Authored by aheejin on Aug 25 2023, 8:19 PM.

Details

Reviewers
dschuff
sbc100
phosek
Group Reviewers
Restricted Project
Commits
rGe6cbba749490: [libc++abi][WebAssembly] Support Wasm EH
Summary

This adds Wasm-specific libc++abi changes to support Wasm exception
handling (https://github.com/WebAssembly/exception-handling).

Wasm EH requires __USING_WASM_EXCEPTIONS__ to be defined. Wasm EH's
LSDA handling mostly shares that of SjLj EH.
Changes are:

  • In Wasm, a destructor returns its argument.
  • Wasm EH currently only has one phase (search) that does both search and cleanup. So added an additional set_registers to support that.

The bulk of these changes was added back in Mar 2020 in
https://github.com/emscripten-core/emscripten/pull/10577 to emscripten
repo and has been used ever since. Now we'd like to upstream this so
that other toolchains that don't use emscripten libraries, e.g., WASI,
can use this too.

Companion patch: D158919

Diff Detail

Event Timeline

aheejin created this revision.Aug 25 2023, 8:19 PM
aheejin requested review of this revision.Aug 25 2023, 8:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 8:19 PM
Herald added a reviewer: Restricted Project. · View Herald Transcript

Thanks for posting this.
I don't really have any new comments about the code itself (I guess that's not surprising since I reviewed it before ...)
I notice there are no changes to the build system for this; presumably that's just because we don't have CMake support for building LLVM libraries for wasm in general. I wonder whether LLVM or the libc++abi maintainers have any policies about that.
Also, regarding the __USING_WASM_EXCEPTIONS__ macro: this is not predefined by the compiler but needs to be defined by the builder/build system, right? I wonder if that's something that belongs in <__cxxabi_config.h>

@dschuff

I notice there are no changes to the build system for this; presumably that's just because we don't have CMake support for building LLVM libraries for wasm in general. I wonder whether LLVM or the libc++abi maintainers have any policies about that.
Also, regarding the __USING_WASM_EXCEPTIONS__ macro: this is not predefined by the compiler but needs to be defined by the builder/build system, right? I wonder if that's something that belongs in <__cxxabi_config.h>

We currently define it in the library building script: https://github.com/emscripten-core/emscripten/blob/df74d74f5dfce1c2128fdb0dde1b09c178f7a38a/tools/system_libs.py#L1505-L1506

__USING_SJLJ_EXCEPTIONS__, a preexisting macro, seems to be defined here: https://github.com/llvm/llvm-project/blob/15b5ac38cf5d2272e28076357c207194f0ef6a6c/clang/lib/Frontend/InitPreprocessor.cpp#L913-L914; It's not defined in either __cxxabi_config.h or CMakeLists.txt.
We can maybe add something like

if (LangOpts.hasWasmExceptions())
  Builder.defineMacro("__USING_WASM_EXCEPTIONS__");

in InitProcessor.cpp too, like __USING_SJLJ_EXCEPTIONS__. So far it has not really necessary because we only use it within libraries and the libraries are built with system_libs.py, which defines it.

I see. It makes sense that this likely wouldn't be needed other than for building support libraries, but maybe we should just handle this similarly to the other EH styles and make a clang predefine as you showed.

Yeah we can do it, but that can be a different patch.

aheejin updated this revision to Diff 554903.Aug 30 2023, 11:22 PM

Fix bug in defining __USING_SJLJ_OR_WASM_EXCEPTIONS__

Gentle ping 😀

dschuff accepted this revision.Sep 21 2023, 9:58 AM

This LGTM from the wasm side. perhaps @phosek can just verify that there's not something weird that doesn't fit with how things are done in the library more generally

phosek accepted this revision.Sep 21 2023, 10:48 AM

LGTM

libcxxabi/src/cxa_personality.cpp
35

I'd slightly prefer inlining this as #if defined(__USING_SJLJ_EXCEPTIONS__) || defined (__USING_WASM_EXCEPTIONS__) to avoid introducing another define since it's only used in a handful of places.

This revision is now accepted and ready to land.Sep 21 2023, 10:48 AM
aheejin updated this revision to Diff 557224.Sep 22 2023, 12:31 AM
aheejin marked an inline comment as done.

Remove __USING_SJLJ_OR_WASM_EXCEPTIONS__

aheejin edited the summary of this revision. (Show Details)Sep 22 2023, 12:32 AM
aheejin edited the summary of this revision. (Show Details)
This revision was landed with ongoing or failed builds.Sep 22 2023, 12:34 AM
This revision was automatically updated to reflect the committed changes.