Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Thread-local variables corruption after setjmp/longjmp with threads+dynlink #14461

Closed
Faless opened this issue Jun 16, 2021 · 5 comments · Fixed by #15356
Closed

Thread-local variables corruption after setjmp/longjmp with threads+dynlink #14461

Faless opened this issue Jun 16, 2021 · 5 comments · Fixed by #15356

Comments

@Faless
Copy link
Contributor

Faless commented Jun 16, 2021

While working on the Godot web port, trying to build a threads+dynlink binary I stumbled upon some weird deadlocks which traced back to corrupted thread-local variables after doing a setjmp/longjmp on the main thread.

Here is a minimal reproduction:

// side.cpp
#include <emscripten/emscripten.h>
#include <iostream>
#include <setjmp.h>
#include <assert.h>

static thread_local bool v1 = false;
static thread_local int v2 = 42;

void JB(jmp_buf *buf) {
	longjmp(*buf, 1);
}

void JA() {
	jmp_buf buf;
	if (!setjmp(buf)) {
		std::cout << "Jump set" << std::endl;
		JB(&buf);
	} else {
		std::cout << "Jump over" << std::endl;
	}
}

extern EMSCRIPTEN_KEEPALIVE int js_main(int argc, char *argv[]) {
	v1 = true;
	v2 = 12;
	std::cout << "Started!" << std::endl;
	assert(v1 == true);
	assert(v2 == 12);
	JA();
	// One of the following fails
	assert(v1 == true);
	assert(v2 == 12);
	std::cout << "Over!" << std::endl;
	return 0;
}
// main.cpp
extern int js_main(int argc, char *argv[]);

int main(int argc, char *argv[]) {
	return js_main(argc, argv);
}

Build with:

em++ side.cpp -Os -s USE_PTHREADS=1 -o side.wasm -s SIDE_MODULE=2
em++ main.cpp -Os -s USE_PTHREADS=1 -o main.html -s MAIN_MODULE=1 side.wasm

Or see the attached zip for both sources and binary (compiled with emcc 2.0.24)

thread_local.zip

@sbc100
Copy link
Collaborator

sbc100 commented Jun 16, 2021

Thanks for the report. Interesting/useful that you don't even need to start any threads to reproduce.

@kripken
Copy link
Member

kripken commented Jun 16, 2021

cc @tlively as this is about thread-local.

@tlively
Copy link
Member

tlively commented Jun 16, 2021

Thanks for the cc, @kripken! @sbc100, the dylink stuff makes me think this is in your bailiwick, but let me know if you want help looking at it.

@sbc100
Copy link
Collaborator

sbc100 commented Jul 20, 2021

This looks related to the fact the TLS variables cannot be exported across module boundaries today. Its one of the limitations of pthreads + dynamic linking that we have right now.

The codegen for setjmp/longjmp lowering on llvm generates references to __THREW__ and __threwValue in the side module code but these are defined in the main module.

The mystery is why we don't see a linker error here... I'm continuing to investigate that part.

@sbc100
Copy link
Collaborator

sbc100 commented Jul 20, 2021

Indeed, this is not a supported configuration right now since setjmp/longjmp rely on cross module TLS variables which are not yet supported with threading.

Once this lands you will see a nice error at link time: https://reviews.llvm.org/D106385

sbc100 added a commit that referenced this issue Jul 20, 2021
This relies on exporting TLS symbols which we currently do not support.

By default we disable SUPPORT_LONGJMP in this configuration.  If the
user tries to explicitly enable it we error out.

See #14461
sbc100 added a commit that referenced this issue Jul 20, 2021
This relies on exporting TLS symbols which we currently do not support.

By default we disable SUPPORT_LONGJMP in this configuration.  If the
user tries to explicitly enable it we error out.

See #14461
sbc100 added a commit that referenced this issue Jul 20, 2021
This relies on exporting TLS symbols which we currently do not support.

By default we disable SUPPORT_LONGJMP in this configuration.  If the
user tries to explicitly enable it we error out.

See #14461
sbc100 added a commit to llvm/llvm-project that referenced this issue Jul 20, 2021
In https://reviews.llvm.org/D102044 we made exporting a TLS symbol
into an error, but we also want to error on import.

See emscripten-core/emscripten#14461

Differential Revision: https://reviews.llvm.org/D106385
sbc100 added a commit that referenced this issue Jul 20, 2021
This relies on exporting TLS symbols which we currently do not support.

By default we disable SUPPORT_LONGJMP in this configuration.  If the
user tries to explicitly enable it we error out.

See #14461
sbc100 added a commit that referenced this issue Jul 22, 2021
This relies on exporting TLS symbols which we currently do not support.

By default we disable SUPPORT_LONGJMP in this configuration.  If the
user tries to explicitly enable it we error out.

See #14461
arichardson pushed a commit to CTSRD-CHERI/llvm-project that referenced this issue Sep 29, 2021
In https://reviews.llvm.org/D102044 we made exporting a TLS symbol
into an error, but we also want to error on import.

See emscripten-core/emscripten#14461

Differential Revision: https://reviews.llvm.org/D106385
sbc100 added a commit that referenced this issue Oct 25, 2021
The underlying bug that caused this configuration
not to work (lack of TLS import/export) was fixed
in #14461.
sbc100 added a commit that referenced this issue Oct 25, 2021
The underlying bug that caused this configuration
not to work (lack of TLS import/export) was fixed
in #14971.

Fixes: #14461
sbc100 added a commit that referenced this issue Oct 26, 2021
The underlying bug that caused this configuration
not to work (lack of TLS import/export) was fixed
in #14971.

Fixes: #14461
sbc100 added a commit that referenced this issue Oct 26, 2021
The underlying bug that caused this configuration
not to work (lack of TLS import/export) was fixed
in #14971.

Fixes: #14461
sbc100 added a commit that referenced this issue Oct 26, 2021
The underlying bug that caused this configuration
not to work (lack of TLS import/export) was fixed
in #14971.

Fixes: #14461
mem-frob pushed a commit to draperlaboratory/hope-llvm-project that referenced this issue Oct 7, 2022
In https://reviews.llvm.org/D102044 we made exporting a TLS symbol
into an error, but we also want to error on import.

See emscripten-core/emscripten#14461

Differential Revision: https://reviews.llvm.org/D106385
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging a pull request may close this issue.

4 participants