For panic=unwind on Wasm targets, define __cpp_exception tag

[hoodmane]

Since llvm/llvm-project#159143, llvm no longer weak links the __cpp_exception tag into each object that uses it. They are now defined in compiler-rt. Rust doesn't seem to get them from compiler-rt so llvm decides they need to be imported. This adds them to libunwind.

See also rust-lang/rust#152241 which includes a test for this change.

[bjorn3]

For wasi should this be provided by wasi-libc in case C++ code is used?

[hoodmane]

I am not sure what is happening with wasi cc @alexcrichton . But I'm happy to guard this to also be skipped on wasi if we think that's the right choice. I think the only case where there could be problems is with dynamic linking.

[alexcrichton]

I'd agree with bjorn3 that this shouldn't be defined here, but in general with dynamic linking I don't think this should be defined in compiler-builtins either because that would mean that there'd be an exception-type-per-shared-object which I don't think is what's desired.

Is this just for Emscripten targets? If so, shouldn't Emscripten be the one providing this definition? If this isn't for Emscripten, is it for wasm32-unknown-unknown? If so where does Rust end up needing __cpp_exception from?

[hoodmane]

It is for wasm32-unknown-unknown. I have a cfg on this code that excludes Emscripten because Emscripten is already handling it. Rust uses cpp_exception for panics and catch_unwind. It is emitted by llvm, and by default it will import it from env.__cpp_exception unless you define the symbol explicitly. wasm-bindgen is currently using walrus to transform the generated module and convert the __cpp_exception import into a tag definition:
https://github.com/wasm-bindgen/wasm-bindgen/pull/4938/changes#diff-8d702d8da596b2105e4e94a3855489a49792843bd621954a3feacf845b6d42afR375-R389

[hoodmane]

Specifically, panic uses the wasm_throw llvm intrinsic and catch_unwind uses the wasm_catch llvm intrinsic. Both of these intrinsics generate a reference to this symbol:
wasm_throw definition
wasm_catch definition
__cpp_exception symbol generated here

[alexcrichton]

Makes sense, thanks for the info. In that situation though this definition I think might be best to reside in the unwind crate in the standard library? There's no need to have it compiler-builtins specifically and unwinding is the true source of the symbol as well?

Additionally I think it would be good to restrict it to target_os="unknown" to only affect wasm32-unknown-unknown. The responsibility for defining this symbol on other toolchains lies elsewhere (e.g. with Emscripten or WASI targets). External toolchains will have more knowledge of what to do in the face of shared libraries, for example.

[hoodmane]

Yeah, that's fine with me.

[tgross35]

If this just needs to be a completely empty function body, could this just be:

intrinsics! {
    #[unsafe(naked)]
    pub fn __cpp_exception -> usize {
        core::arch::naked_asm!("");
    }
}

intrinsics + naked will handle weak linkage plus symbol visibility concerns which aren't covered here.

Also if you rebase, CI should be working again.

[rustbot]

This PR was rebased onto a different main commit. Here's a range-diff highlighting what actually changed.

Rebasing is a normal part of keeping PRs up to date, so no action is needed—this note is just to help reviewers.

[hoodmane]

If this just needs to be a completely empty function body,

It shouldn't be making a function at all. It is defining a wasm-eh exception tag. For reference here is the corresponding code in compiler-builtins:
https://github.com/llvm/llvm-project/blob/main/compiler-rt/lib/builtins/wasm/__cpp_exception.S#L22-L24

I think the weak linkage works out alright -- for instance it worked when I removed the #[cfg(not(target_os = "emscripten"))] and linked it into an Emscripten binary, which will also get a second copy from llvm's compiler-rt. Not sure about visibility. The issue on Emscripten is that we don't want to make this symbol when linking a dynamic library.

[tgross35]

I don't think the symbol is weak as-is, did it complain if linker errors are enabled?

$ llvm-objdump target/wasm32-unknown-unknown/debug/libcompiler_builtins.rlib -t | grep cpp
00000000 g       TAG    00000000 __cpp_exception

Adding the following gets what I think we want and I think should address the dylib concern:

".weak __cpp_exception",
".hidden __cpp_exception",

$ llvm-objdump target/wasm32-unknown-unknown/debug/libcompiler_builtins.rlib -t | grep cpp
target/wasm32-unknown-unknown/debug/libcompiler_builtins.rlib(compiler_builtins-d7189ad935ac03f6.cd49fsxm0kcppv9dqnwf3hslx.1dcdpkz.rcgu.o):     file format wasm
00000000  w      TAG    00000000 .hidden __cpp_exception

Though I'm curious why LLVM doesn't do this.

[alexcrichton]

For dylibs specifically my concern is that compiler-rt is the wrong place for this symbol. Compiler-builtins is linked to all crates and is intended to be linked statically, but this symbol needs to be defined precisely once amongst a set of dynamic libraries to have exceptions work correctly (e.g. otherwise you couldn't throw in one dylib and catch in another). Solving that would require libc.so or libunwind.so to provide this symbol and all other dylibs would need to link against that to get the symbol. In that situation, placing this in compiler-builtins isn't the right location in my opinion

[hoodmane]

@tgross35 that makes sense to me I can add those. Though as you say, llvm doesn't set that stuff.
@alexcrichton that makes sense to me but this seems to match what llvm is doing.

@sbc100 can you explain what's going on? Particularly regarding the previous two comments on symbol visibility and dynamic linking. I see in Emscripten that libcxxabi always includes __cpp_exception.S, but if I manually link it into an Emscripten dynamic library the resulting dynamic library doesn't import the tag anymore. Is the point that libcxxabi is only linked to the executable?

[bjorn3]

Ideally LLVM would allow us to define our own rust exception tag that we could define in eg libcore. This would also avoid the need for the exception canary on wasm (we did still need it on native though).

[sbc100]

@tgross35 that makes sense to me I can add those. Though as you say, llvm doesn't set that stuff. @alexcrichton that makes sense to me but this seems to match what llvm is doing.

@sbc100 can you explain what's going on? Particularly regarding the previous two comments on symbol visibility and dynamic linking. I see in Emscripten that libcxxabi always includes __cpp_exception.S, but if I manually link it into an Emscripten dynamic library the resulting dynamic library doesn't import the tag anymore. Is the point that libcxxabi is only linked to the executable?

In emscripten we only link libcxxabi into the main program and never into shared libraries so there should always only be one definition of these things. (We also only link compiler-rt into the main program BTW).

If you want to add __cpp_exception into a library that is statically linked into more than one DSO then it would need to be defined weakly so that only one definition will "win" at runtime

[bjorn3]

We also only link compiler-rt into the main program BTW

Rustc does link compiler-builtins into every dylib however and uses hidden visibility for all symbols in it.

[sbc100]

I see, in that case maybe compiler-builtins is not the right place for this symbol after all, since it is required to have just once definition across the whole program.

Maybe it should live in libcxxabi instead? Or some other library that is linked just once.

Alternatively it could be defined weakly so that just one definition will be chosen by the dyanmic linker.

[hoodmane]

Thanks for the explanation @sbc100. So you would also recommend adding:

.weak __cpp_exception
.hidden __cpp_exception

to address dynamic libraries exactly as @tgross35 said?

[hoodmane]

Or is .weak enough? I'm not clear whether or not we want .hidden.

[bjorn3]

Probably not .hidden given that dylibs need to use the same tag as the main executable.

[tgross35]

Based on Alex's comments I guess it's best to move this back to our unwind then? Unless you have a reason to prefer it in c-b @bjorn3.

Sorry for the back and forth here @hoodmane.

[alexcrichton]

Personally, yeah, my read on this is that this would be well-suited for unwind. As this only applies to wasm32-unknown-unknown that's sort of The Location to put unwinding-related things. While dynamic linking is unlikely to be used on that target, the Rust standard library would already have to be in only one shared library and by putting it in the unwind crate there'd be a guaratnee that one crate would export it and everything else would link to it.

When in unwind I don't think it would be either hidden or weak, it'd be the only strong definition. That would then cause a clash if something C/C++-related were linked in but that's a bug anyway for wasm32-unknown-unknown most likely. Once in that realm it's probably best to use the Emscripten or WASI toolchains in which case Rust will no longer be responsible for defining this symbol.

I do agree with @bjorn3, however, where it would be best for Rust to be able to define its own tag type. That way we wouldn't have to worry about any of this and __rust_panic (or w/e it's called) would be located in the unwind crate for all wasm targets and that would work out.

[hoodmane]

@alexcrichton do you mean that panics shouldn't be C++ exceptions anymore? Or would this only apply to the wasm32-unknown-unknown target? Are panics C++ exceptions on other targets?

[hoodmane]

I've updated rust-lang/rust#152241 based on the feedback here. Thanks everyone!

[alexcrichton]

I'd ideally like that for Rust panics on all WebAssembly targets that a Rust-specific tag were used. There's no reason to get __cpp_exception engaged here beyond the fact that it's a built-in detail of __builtin_wasm_throw or the LLVM intrinsic. That would mean that a Rust panic wouldn't look like a C++ exception, yes, and my impression is that accurately reflects the desired semantics where a C++ catch shouldn't be able to catch a Rust panic