Correct TSAN tasking integration #36929

Keno · 2020-08-05T18:50:26Z

In order for tsan to work, any setjmp/longjmp must be executed while
that task's tsan state is active. As a result, we cannot switch the
tsan state until said setjmp is completed, but need to do it
(as basically the only thing happening) between the setjmp and the
subsequent longjmp. To facilitate this without too much disruption,
move the tsan state into the jl_ucontext_t, which seems appropriate
since it's additional state that needs to be restored on context
switch.

Also forbid using TSAN from Clang < 11, where the runtime library
has bugs that cause us to exhaust the maximum number of allowed
mappings.

With this, TSAN works, but obviously has a fairly large number of complaints.
For reference, here is my Make.user:

USE_BINARYBUILDER_LLVM=0
USECLANG=1
SANITIZE=1
SANITIZE_THREAD=1
JULIA_PRECOMPILE=0
override CC=/home/keno/llvm-project-build/bin/clang
override CXX=/home/keno/llvm-project-build/bin/clang++

vtjnash · 2020-08-06T16:44:48Z

Make.inc

+ifeq ($(LLVM_ASSERTIONS),0)
+CXX_DISABLE_ASSERTION += -DNDEBUG
+endif
+


As this variable does not have the actual runtime value (which should get set from llvm-config), we have a header (julia_assert.h) that is supposed to already deal with this.

Looks like llvm-config no longer sets NDEBUG, but it still needs to match what LLVM expects for ABI reasons.

vtjnash · 2020-08-06T16:45:25Z

src/gc-stacks.c

+#ifdef JL_TSAN_ENABLED
+                if (t->ctx.tsan_state) {
+                    t->ctx.tsan_state = NULL;
+                    __tsan_destroy_fiber(t->ctx.tsan_state);


This is a NULL pointer?

Suggested change

__tsan_destroy_fiber(t->ctx.tsan_state);

__tsan_destroy_fiber(NULL);

vtjnash · 2020-08-06T16:47:09Z

src/task.c

@@ -279,6 +277,10 @@ JL_DLLEXPORT jl_task_t *jl_get_next_task(void)
    return ptls->current_task;
 }

+#ifdef JL_TSAN_ENABLED
+char tsan_state_corruption[] = "TSAN state corrupted. Exiting HARD!\n";


Suggested change

char tsan_state_corruption[] = "TSAN state corrupted. Exiting HARD!\n";

const char tsan_state_corruption[] = "TSAN state corrupted. Exiting HARD!\n";

vtjnash · 2020-08-06T16:50:23Z

src/task.c

+        // Something went really wrong - don't even assume that we can
+        // use assert/abort which involve lots of signal handling that
+        // looks at the tsan state.
+        write(STDERR_FILENO, tsan_state_corruption, sizeof(tsan_state_corruption));


Suggested change

write(STDERR_FILENO, tsan_state_corruption, sizeof(tsan_state_corruption));

write(STDERR_FILENO, tsan_state_corruption, sizeof(tsan_state_corruption) - 1);

JeffBezanson · 2020-08-06T23:05:22Z

src/task.c

+                tsan_destroy_ctx(ptls, &lastt->ctx);
+                jl_set_fiber(&t->ctx); // (doesn't return)
+                abort(); // unreachable
+            } else {


Style: line break between } and else

In order for tsan to work, any setjmp/longjmp must be executed while that task's tsan state is active. As a result, we cannot switch the tsan state until said setjmp is completed, but need to do it (as basically the only thing happening) between the setjmp and the subsequent longjmp. To facilitate this without too much disruption, move the tsan state into the jl_ucontext_t, which seems appropriate since it's additional state that needs to be restored on context switch. Also forbid using TSAN from Clang < 11, where the runtime library has bugs that cause us to exhaust the maximum number of allowed mappings.

This a component of #36929 which happens to fix building on FreeBSD 11. Co-authored-by: Keno Fischer <keno@juliacomputing.com>

Looks like #36929 got reverted without comment. Not sure what happened there. Re-apply it to fix TSAN.

This reverts commit 15772ba "Update references to tsan state (#42440)", and fixes integration. Looks like #36929 got reverted when trying to simplify the code, but it now isn't legal. Since these must be inlined in the right point in the runtime call/return context, use a macro to ensure that.

This reverts commit 15772ba "Update references to tsan state (JuliaLang#42440)", and fixes integration. Looks like JuliaLang#36929 got reverted when trying to simplify the code, but it now isn't legal. Since these must be inlined in the right point in the runtime call/return context, use a macro to ensure that.

Keno requested review from vchuravy and vtjnash August 5, 2020 18:50

Keno force-pushed the kf/fixtsan branch 5 times, most recently from b69fa10 to 7f9ab34 Compare August 6, 2020 04:33

vtjnash approved these changes Aug 6, 2020

View reviewed changes

JeffBezanson reviewed Aug 6, 2020

View reviewed changes

Keno force-pushed the kf/fixtsan branch from 7f9ab34 to 5cd24f2 Compare August 6, 2020 23:33

Keno merged commit 7c0cb30 into master Aug 7, 2020

Keno deleted the kf/fixtsan branch August 7, 2020 05:02

Keno mentioned this pull request Sep 20, 2020

Strange race condition while running conjugate gradients in multithreaded code #36297

Closed

ranjanan mentioned this pull request Nov 1, 2020

Build error using Clang-11 and thread sanitizer #38251

Closed

ararslan added a commit that referenced this pull request Feb 19, 2021

Set NDEBUG when LLVM assertions are disabled

c39f8c5

This a component of #36929 which happens to fix building on FreeBSD 11. Co-authored-by: Keno Fischer <keno@juliacomputing.com>

Keno added a commit that referenced this pull request Oct 1, 2021

Re-correct TSAN integration

8aabc09

Looks like #36929 got reverted without comment. Not sure what happened there. Re-apply it to fix TSAN.

Keno mentioned this pull request Oct 1, 2021

Re-correct TSAN integration #42447

Closed

Keno added a commit that referenced this pull request Oct 1, 2021

Re-correct TSAN integration

b249b33

Looks like #36929 got reverted without comment. Not sure what happened there. Re-apply it to fix TSAN.

Keno added a commit that referenced this pull request Oct 1, 2021

Re-correct TSAN integration

c9369ad

Looks like #36929 got reverted without comment. Not sure what happened there. Re-apply it to fix TSAN.

vtjnash pushed a commit that referenced this pull request Oct 3, 2021

Re-correct TSAN integration

47c20ec

Looks like #36929 got reverted without comment. Not sure what happened there. Re-apply it to fix TSAN.

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Correct TSAN tasking integration #36929

Correct TSAN tasking integration #36929

Keno commented Aug 5, 2020

vtjnash Aug 6, 2020

Keno Aug 6, 2020

vtjnash Aug 6, 2020

vtjnash Aug 6, 2020

vtjnash Aug 6, 2020

JeffBezanson Aug 6, 2020

	__tsan_destroy_fiber(t->ctx.tsan_state);
	__tsan_destroy_fiber(NULL);

	char tsan_state_corruption[] = "TSAN state corrupted. Exiting HARD!\n";
	const char tsan_state_corruption[] = "TSAN state corrupted. Exiting HARD!\n";

	write(STDERR_FILENO, tsan_state_corruption, sizeof(tsan_state_corruption));
	write(STDERR_FILENO, tsan_state_corruption, sizeof(tsan_state_corruption) - 1);

Correct TSAN tasking integration #36929

Correct TSAN tasking integration #36929

Conversation

Keno commented Aug 5, 2020

vtjnash Aug 6, 2020

Choose a reason for hiding this comment

Keno Aug 6, 2020

Choose a reason for hiding this comment

vtjnash Aug 6, 2020

Choose a reason for hiding this comment

vtjnash Aug 6, 2020

Choose a reason for hiding this comment

vtjnash Aug 6, 2020

Choose a reason for hiding this comment

JeffBezanson Aug 6, 2020

Choose a reason for hiding this comment