Make longjmp/exceptions helpers thread-safe #12056

kripken · 2020-08-26T17:02:50Z

This adds the thread-local annotation to those globals. Previously they were in
JS, which is effectively thread-local, but then we moved them to C which meant
they were stored in the same shared memory for all threads. A race could
happen if threads (or longjmp) operations happened at just the right time, one
writing to those globals and another reading.

Also make compiler-rt now build with a multithreaded variation, as the implementation
of those globals is in that library.

Also add a testcase that runs exceptions on multiple threads. It's not a guaranteed
way to notice a race, but it may help, and this is an area we didn't have coverage
of.

Fixes #12035

This has been a possible race condition for a very long time. I think it went
unnoticed because exceptions and longjmp tend to be used for exceptional
things, and not constantly being executed. And also until we get wasm
exceptions support they are also slow, so people have been trying to avoid
them as much as possible anyhow.

sbc100 · 2020-08-26T21:50:55Z

system/lib/libc/extras.c

@@ -31,3 +31,15 @@ long* _get_timezone() {
 void __lock(void* ptr) {}
 void __unlock(void* ptr) {}

+/* References to these longjmp- and exceptions-supporting things are generated


why move into this file?

These symbols seems to live more logically in compiler-rt, which is a library that hold internal symbols that the compiler can generate during codegee.

The main reason is that compiler-rt doesn't have a multithreaded variation atm, and adding one just for this seemed silly to me. Given libc already had a kind of "misc" file, this seemed reasonable. But I don't feel strongly, what do you prefer?

sbc100 · 2020-08-26T22:33:06Z

OK, maybe just mention that in the PR.

Maybe be worth putting it these in their own source file too, since they are locally part of the same functionality?

How about emscripten_exception_builtins.c? I can't really think of a better name...

kripken · 2020-08-26T23:01:46Z

@sbc100 sounds good, split off and renamed.

kripken · 2020-08-26T23:14:01Z

@tlively Are there any known issues with TLS that we should be aware of here?

tlively · 2020-08-27T00:04:56Z

No bugs, but our TLS is limited to the local-exec model, which means that thread_local data can only be accessed from the main executable and not from any shared libraries. As long as the functions that directly access these are statically linked in, there should be no problems.

kripken · 2020-08-27T00:13:32Z

I see, thanks @tlively That sounds fine then.

But trying to write a test for pthreads + exceptions, it fails for me. Here is my test and the diff: https://gist.github.com/kripken/6c9cd1d8e575c09e5636e68cae435bdd That's all with this PR, and then compiling the program without _Thread_local (on those special globals), and then with (as in the PR's default). The diff of the ThreadMain program shows that what was a global address, 2948 (which I am guessing is __THREW__) becomes just 0.

edit: example compile command: ./emcc exceptions.cpp -pthread -O2 -fexceptions --profiling

Maybe my testcase is bad somehow?

tlively · 2020-08-27T00:43:48Z

Hmm, I can't see anything wrong with it. I'll clone this branch locally and see if I can figure out what is happening.

tlively · 2020-08-27T01:03:23Z

Ok, I poked around a bit and I think this change is going to require an upstream LLVM change. Right now the Emscripten SjLj lowering pass constructs the __THREW__ and __threwValue globals if they are not already defined in the module, but it doesn't set any TLS mode on them. So codegen lowers accesses to them as normal accesses rather than as TLS accesses. That 0 address is probably supposed to be the offset into the TLS segment, but codegen didn't know to add in the TLS base.

kripken · 2020-08-27T01:21:12Z

Oh, that makes sense, yeah... only the linker sees that these are TLS, but if there are also necessary codegen changes then it's not a simple relocation the linker can fix up.

Ok, we'll wait on a backend change then. Not sure there is any other option here other than to go back to JS variables for these, which is slow.

sbc100 · 2020-09-24T21:43:02Z

system/lib/libc/emscripten_exception_builtins.c

+*/
+
+int _Thread_local __THREW__ = 0;
+int _Thread_local __threwValue = 0;


It looks like you can use the thread_local keyword if you include

#include <threads.h> thread_local int foo = 0;

I think that i slightly more readable and also the same keyword as C++ so less confusing.

Makes sense, done.

tlively · 2020-09-24T21:43:04Z

LLVM patch is up: https://reviews.llvm.org/D88262

Emscripten's longjump and exception mechanism depends on two global variables, `__THREW__` and `__threwValue`, which are changed to be defined as thread-local in emscripten-core/emscripten#12056. This patch updates the corresponding code in the WebAssembly backend to properly declare these globals as thread-local as well. Differential Revision: https://reviews.llvm.org/D88262

sbc100 · 2020-09-24T22:03:21Z

I'm tempted to say we should add a thread variant of compiler-rt just because this code seems to make so more sense in compiler-rt .. since its literally defining compiler-internally-generated symbols.

sbc100 · 2020-09-24T22:03:39Z

Then we could avoid the file rename and this patch would be way tiny.

kripken · 2020-09-24T22:15:20Z

This patch is already pretty tiny isn't it @sbc100 ? Not sure what you are thinking of trimming out.

Making compiler-rt have a threaded variant seems like a larger change.

However, I do not feel strongly about this either way.

kripken · 2020-09-24T22:28:45Z

Changed to use a compiler-rt variant. Slightly sad this increases our SDK downloads by 150K, but if this feels cleaner sgtm.

kripken · 2020-09-25T02:45:44Z

This now includes undoing the test disabling in #12337. After LLVM rolls in this should pass. Locally it looks ok for me on the disabled tests.

Note that I had to fix up one test, test_warning_flags, and also I had to add libc_rt_wasm to also have a multithreaded variation.

kripken · 2020-09-25T14:56:48Z

A bunch more libraries must now be built with -mt variations, asan_js and embind. I am not actually sure why these and not others, so this worries me - will users hit more library combinations we don't test?

It seems like when the user builds with pthreads, we now need all libraries to be built with atomics. So it's not just some libraries we annotate with -mt, but they all must be. So like relocatable, we may as well have another cache dir, really.

Or is this just libraries that use setjmp or exceptions, and so they have __THREW__ which is now atomic?

kripken · 2020-09-25T15:02:03Z

asan_js does not use exceptions or longjmp. Literally the only difference in the .o files (between the normal and -mt versions of it) is the features section. So I'm not sure why the LLVM change made us need to build it with an -mt variation while before we didn't...

@tlively I'm very confused here, please advise...

system/lib/compiler-rt/emscripten_exception_builtins.c

sbc100 · 2020-09-25T16:10:26Z

tools/system_libs.py

@@ -1086,7 +1086,7 @@ class libwebgpu_cpp(MTLibrary):
  src_files = ['webgpu_cpp.cpp']


-class libembind(Library):


I don't think this should be needed as part of this change. At least that is not the intention behind the recent llvm changes.

What error were you seeing without this?

The usual error on features not matching: can't link in this .o because it disallows atomics.

Specifically

wasm-ld: error: --shared-memory is disallowed by bind.o because it was not compiled with 'atomics' or 'bulk-memory' features.

Strange.. I can't see how that would start happening as a result of this change. @tlively will hopefully know what is going on here.

Building bind.o with an -mt variant fixes it, of course.

What's not clear to me is why this worked before, and what @tlively 's change in LLVM makes different so that it doesn't work now...

Hmm I think I know what's going on. I'll do another LLVM patch to explain and fix it.

dschuff · 2020-09-25T16:37:08Z

Does this really need to be in compiler-rt? It seems we could avoid the -mt build issues just by moving it to libcxxabi?

edit: Nevermind, I guess this is also used for setjmp/longjmp. We could potentially split it so setjmp and EH use different variables, and put one in libcxxabi and the other in libc (both of those already have threaded variants, right?)

kripken · 2020-09-25T17:46:24Z

It may be good to land this as-is (with the comment cleanup @sbc100 mentioned) as it now passes tests while enabling the ones that were disabled, so it is strictly better than what is in trunk atm. Then after the LLVM update we can adjust.

However if we think the LLVM update may end up not rolling in by us re-enabling tests here, we should probably just wait, to avoid any annoying back-and-forth.

sbc100 · 2020-09-25T17:53:13Z

I think maybe we should get the bottom of things before landing.

I am rather confused about why you started seeing wasm-ld: error: --shared-memory is disallowed by bind.o because it was not compiled with 'atomics' or 'bulk-memory' features.

This reverts commit 36c0ee0.

This reverts commit d42e148.

This reverts commit 7955ce2.

tlively · 2020-09-25T18:23:25Z

The way my previous LLVM patch worked, it unconditionally made __THREW__ and __threwValue thread-local and depended on the CoalesceFeaturesAndStripAtomics pass to downgrade it to a normal variable if necessary. Unfortunately that had the unintended side effect of making every non-TLS-supporting object strip TLS, which normally makes the object thread-unsafe and causes it to be disallowed from being linked into a shared-memory module. The fix is in https://reviews.llvm.org/D88323, which checks features before making the variables thread-local.

kripken · 2020-09-25T18:24:48Z

Passes locally for me with that patch. Uploaded simplified version here that can be run once that has rolled in.

sbc100 · 2020-09-25T20:42:11Z

Look like Thomas's change is rolling in now: https://chromium-review.googlesource.com/c/emscripten-releases/+/2432788

One possible downside I just realized is that we now have a very hard dependency on this version llvm... but maybe its only for programs that use exceptions of setjmp/longjmp which isn't so bad.

tlively · 2020-09-25T21:11:44Z

Look like Thomas's change is rolling in now: https://chromium-review.googlesource.com/c/emscripten-releases/+/2432788

One possible downside I just realized is that we now have a very hard dependency on this version llvm... but maybe its only for programs that use exceptions of setjmp/longjmp which isn't so bad.

Did we decide to try to support other versions of LLVM?

dschuff · 2020-09-25T21:16:29Z

AFAIK we haven't really committed to that, although we also think that it should mostly just work (although we aren't testing it anywhere either, so it's probably broken in some way or other).

tlively · 2020-09-25T22:27:56Z

This kind of ABI change that requires a simultaneous Emscripten and LLVM change seems kind of rare, but is definitely the kind of thing that would make support stable LLVM difficult.

Emscripten's longjump and exception mechanism depends on two global variables, `__THREW__` and `__threwValue`, which are changed to be defined as thread-local in emscripten-core/emscripten#12056. This patch updates the corresponding code in the WebAssembly backend to properly declare these globals as thread-local as well. Differential Revision: https://reviews.llvm.org/D88262

kripken added 4 commits August 25, 2020 16:57

Make asm.js threading globals thread-safe

5786570

make compiler-rt an MT library?

0faaddd

fix

0ae338f

Merge remote-tracking branch 'origin/master' into exc

e4f69ed

kripken requested a review from sbc100 August 26, 2020 17:02

kripken mentioned this pull request Aug 26, 2020

Exception Handling - exception on the wrong thread when using pthreads #12035

Closed

sbc100 approved these changes Aug 26, 2020

View reviewed changes

kripken added 2 commits August 26, 2020 15:56

Merge remote-tracking branch 'origin/master' into exc

3cba11a

feedback: refactor into a side file

fccf48c

kripken added 2 commits August 26, 2020 16:43

add a test

a9741ea

Merge remote-tracking branch 'origin/master' into exc

c4d680f

add a test that can't pass yet without a wasm backend fix [ci skip]

daff2df

sbc100 reviewed Sep 24, 2020

View reviewed changes

kripken added 3 commits September 24, 2020 15:17

Merge remote-tracking branch 'origin/master' into exc

5a4ea53

use threads.h [ci skip]

faa03bc

add a compiler-rt mt variant [ci skip]

06ab316

kripken added 4 commits September 24, 2020 21:27

Merge remote-tracking branch 'origin/master' into exc

586767e

revert the extra disabled test too [ci skip]

e55647a

comment

c76b122

embind and asan_js libraries now also need to be built with -mt

d42e148

sbc100 reviewed Sep 25, 2020

View reviewed changes

-mt

36c0ee0

kripken added 5 commits September 25, 2020 10:59

Revert "-mt"

05257b8

This reverts commit 36c0ee0.

Revert "embind and asan_js libraries now also need to be built with -mt"

5657df5

This reverts commit d42e148.

Revert "Also build libc_rt_wasm with -mt when needed"

b312b57

This reverts commit 7955ce2.

fix comment

44b8dda

cleaner [ci skip]

704a676

remove unneeded change after the llvm change

84ab45b

kripken merged commit e43a870 into master Sep 26, 2020

kripken deleted the exc branch September 26, 2020 00:20

sbc100 mentioned this pull request Dec 2, 2020

relocation R_WASM_MEMORY_ADDR_TLS_SLEB cannot be used against __THREW__ in non-TLS section: .bss #12941

Open

MCJack123 mentioned this pull request Jan 3, 2021

longjmp in Lua producing unexpected behavior in Emscripten 2.0.11 #13166

Open

		@@ -1086,7 +1086,7 @@ class libwebgpu_cpp(MTLibrary):
		src_files = ['webgpu_cpp.cpp']


		class libembind(Library):

Make longjmp/exceptions helpers thread-safe #12056

Make longjmp/exceptions helpers thread-safe #12056

Conversation

kripken commented Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

kripken Aug 26, 2020 • edited Loading

Choose a reason for hiding this comment

sbc100 commented Aug 26, 2020

kripken commented Aug 26, 2020

kripken commented Aug 26, 2020

tlively commented Aug 27, 2020

kripken commented Aug 27, 2020 • edited Loading

tlively commented Aug 27, 2020

tlively commented Aug 27, 2020

kripken commented Aug 27, 2020

Choose a reason for hiding this comment

Choose a reason for hiding this comment

tlively commented Sep 24, 2020

sbc100 commented Sep 24, 2020

sbc100 commented Sep 24, 2020

kripken commented Sep 24, 2020

kripken commented Sep 24, 2020

kripken commented Sep 25, 2020

kripken commented Sep 25, 2020

kripken commented Sep 25, 2020 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

dschuff commented Sep 25, 2020 • edited Loading

kripken commented Sep 25, 2020

sbc100 commented Sep 25, 2020

tlively commented Sep 25, 2020

kripken commented Sep 25, 2020

sbc100 commented Sep 25, 2020

tlively commented Sep 25, 2020

dschuff commented Sep 25, 2020

tlively commented Sep 25, 2020

kripken commented Aug 26, 2020 •

edited

Loading

kripken Aug 26, 2020 •

edited

Loading

kripken commented Aug 27, 2020 •

edited

Loading

kripken commented Sep 25, 2020 •

edited

Loading

dschuff commented Sep 25, 2020 •

edited

Loading