This is an archive of the discontinued LLVM Phabricator instance.

[libunwind][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
rG058222b23166: [libunwind][WebAssembly] Support Wasm EH
Summary

This adds Wasm-specific libunwind port to support Wasm exception
handling (https://github.com/WebAssembly/exception-handling).

Wasm EH requires __USING_WASM_EXCEPTIONS__ to be defined. This adds
Unwind-wasm.c, which defines libunwind APIs for Wasm. This also adds a
thread_local struct of type _Unwind_LandingPadContext, which serves
as a medium for input/output data between the user code and the
personality function. How all these work is explained in
https://github.com/WebAssembly/tool-conventions/blob/main/EHScheme.md.
(The doc is old and "You Shouldn't Prune Unreachable Resumes" section
doesn't apply anymore, but otherwise it should be good)

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: D158918

Diff Detail

Unit TestsFailed

Event Timeline

aheejin created this revision.Aug 25 2023, 8:19 PM
Herald added a project: Restricted Project. · View Herald TranscriptAug 25 2023, 8:19 PM
Herald added subscribers: pmatos, asb, wingo and 2 others. · View Herald Transcript
aheejin requested review of this revision.Aug 25 2023, 8:19 PM
Herald added projects: Restricted 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 libunwind maintainers have any policies about that.

@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 libunwind maintainers have any policies about that.

I replied to your similar libc++abi comment here: https://reviews.llvm.org/D158918#4623814
Not sure about the policy, but __USING_SJLJ_EXCEPTIONS__ isn't mentioned in the build system either.

__USING_SJLJ_EXCEPTIONS__ is compiler defined, it isn't a build system thing. In general, we would like to be able to build the targets that we are claiming to support.

libunwind/include/unwind_itanium.h
27

This is very confusing. Does WASM use SEH? This path is normally for Windows x86.

libunwind/src/Unwind-wasm.c
1

Missing copyright header.

44

Could just return the function rather than assign it to a variable.

76

Why not use the positive form?

if (index == 1)
  ((struct _Unwind_landingPadContext *)context)->selector = value;
libunwind/src/config.h
101

It might be useful to provide warnings should those macros be used on WASM? Right now the use of _LIBUNWIND_WEAK_ALIAS will fail with a weird preprocessor error.

aheejin updated this revision to Diff 554880.Aug 30 2023, 6:03 PM
aheejin marked 3 inline comments as done.

Address comments

libunwind/include/unwind_itanium.h
27

I actually don't really remember, because I wrote this more than 3 years ago. And note that it is !defined (negative), so I think I simply wanted to exclude them because we were not using them. Not sure what you mean by SEH or Windows x86.

I think I can just delete this. All our tests seem to pass without this change. The data structure can be a little larger but exceptions are rare so it wouldn't be a problem.

libunwind/src/config.h
101

We don't use it in Unwind-wasm.c so I think I just tried to avoid the error. But we also support compilation of weak alias, so I think there's no reason we need to error out. I added defined(__wasm__) with other architectures, like this, and removed this separate elif:

--- a/system/lib/libunwind/src/config.h
+++ b/system/lib/libunwind/src/config.h
@@ -83,7 +83,7 @@
   __asm__(".globl " SYMBOL_NAME(aliasname));                                   \
   __asm__(SYMBOL_NAME(aliasname) " = " SYMBOL_NAME(name));                     \
   _LIBUNWIND_ALIAS_VISIBILITY(SYMBOL_NAME(aliasname))
-#elif defined(__ELF__) || defined(_AIX)
+#elif defined(__ELF__) || defined(_AIX) || defined(__wasm__)
 #define _LIBUNWIND_WEAK_ALIAS(name, aliasname)                                 \
   extern "C" _LIBUNWIND_EXPORT __typeof(name) aliasname                        \
       __attribute__((weak, alias(#name)));
@@ -98,7 +98,6 @@
                                              SYMBOL_NAME(name)))               \
   extern "C" _LIBUNWIND_EXPORT __typeof(name) aliasname;
 #endif
-#elif defined(__wasm__)
 #else
 #error Unsupported target
 #endif

Gentle ping 😀

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

LGTM

This revision is now accepted and ready to land.Sep 21 2023, 10:46 AM
dschuff accepted this revision.Sep 21 2023, 11:24 AM

LGTM too

This revision was landed with ongoing or failed builds.Sep 22 2023, 12:39 AM
This revision was automatically updated to reflect the committed changes.
ldionne added inline comments.
libunwind/src/Unwind-wasm.c
2

This file isn't tied into the CMakeLists.txt. So for all we know, this code is dead code.

aheejin marked an inline comment as done.Sep 28 2023, 11:47 PM
aheejin added inline comments.
libunwind/src/Unwind-wasm.c
2

Thanks for pointing that out. Added this to CMakeLists.txt in https://github.com/llvm/llvm-project/pull/67770.