[TSan] Support initialize/finalize hooks in dynamic libraries

[yln]

Summary

Make TSan runtime initialization and finalization hooks work
even if these hooks are not built in the main executable. When these
hooks are defined in another library that is not directly linked against
the TSan runtime (e.g., Swift runtime) we cannot rely on the "strong-def
overriding weak-def" mechanics and have to look them up via dlsym().

Let's also define hooks that are easier to use from C-only code:

extern "C" void __tsan_on_initialize();
extern "C" int __tsan_on_finalize(int failed);

For now, these will call through to the old hooks. Eventually, we want
to adopt the new hooks downstream and remove the old ones.

This is part of the effort to support Swift Tasks (async/await and
actors) in TSan.

rdar://74256720

[yln]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

35 ↗

(On Diff #331354)

I think this should be okay?! dlsym() is posix and this is a posix file.
https://pubs.opengroup.org/onlinepubs/9699919799/functions/dlsym.html

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

Why swift runtime can't do that?
tsan_acquire and tsan_release are in public interface

[delcypher]

@yln Could we add a test case for some of this new functionality?

We could add a lit testcase that has something like:

RUN: %clang -fsanitize=thread %s -o %t ; %run %t | FileCheck -check-prefixes=CHECK-NO-HOOK,CHECK %s
RUN: %clang -fsanitize=thread -Dtest_task_set_tsan_hooks %s -o %t.with_hook ; %run %t.with_hook | FileCheck -check-prefixes=CHECK-HOOK,CHECK %s

// CHECK-NO-HOOK-NOT: swift_task_set_tsan_hooks CALLED

#if defined(test_swift_task_set_tsan_hooks)
void swift_task_set_tsan_hooks(void*, void*) {
  // CHECK-HOOK: swift_task_set_tsan_hooks CALLED
  printf("swift_task_set_tsan_hooks CALLED\n");
}
#endif


int main() {
  // CHECK: DONE
  printf("DONE\n");
}

That way we test both swift_task_set_tsan_hooks being available and not being available at run time.

Note I'm not sure if it's safe to call printf from the point where TSan calls swift_task_set_tsan_hooks but if it isn't we can probably work around this by setting a variable and reading it later from main().

[yln]

In D98810#2632872, @delcypher wrote:

@yln Could we add a test case for some of this new functionality?

We could add a lit testcase that has something like:

RUN: %clang -fsanitize=thread %s -o %t ; %run %t | FileCheck -check-prefixes=CHECK-NO-HOOK,CHECK %s
RUN: %clang -fsanitize=thread -Dtest_task_set_tsan_hooks %s -o %t.with_hook ; %run %t.with_hook | FileCheck -check-prefixes=CHECK-HOOK,CHECK %s

// CHECK-NO-HOOK-NOT: swift_task_set_tsan_hooks CALLED

#if defined(test_swift_task_set_tsan_hooks)
void swift_task_set_tsan_hooks(void*, void*) {
  // CHECK-HOOK: swift_task_set_tsan_hooks CALLED
  printf("swift_task_set_tsan_hooks CALLED\n");
}
#endif


int main() {
  // CHECK: DONE
  printf("DONE\n");
}

That way we test both swift_task_set_tsan_hooks being available and not being available at run time.

Note I'm not sure if it's safe to call printf from the point where TSan calls swift_task_set_tsan_hooks but if it isn't we can probably work around this by setting a variable and reading it later from main().

I thought about how to test this, but concluded that a test like this does not provide much value. The real tests for this are the integration tests in the Swift repo.
Let me know if you insist on adding a test like this.

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

It currently does, but we would like to avoid the dlsym() cost in the non-TSan case.

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

I guess you can do that by implementing weak callback like this__tsan_default_options
This name looks irrelevant, so similar weak can be added.

[yln]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

Thanks for this idea, I didn't think of it!

Just to make sure we are on the same page, you mean the following, right?

// === compiler-rt/tsan ===

// Don't modify, append-only:
struct tsan_functions {
  tsan_acquire,
  tsan_release,
};

SANITIZER_WEAK_DEFAULT_IMPL
void __tsan_receive_function_ptrs(const struct tsan_functions *fn_ptrs, size_t size) {
  // empty
}

// Call above during initialization


// === Swift runtime ===
void __tsan_receive_function_ptrs(const struct tsan_functions *fn_ptrs, size_t size) {
  // save & use function pointers
}

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

Better something more generic:

void __tsan_on_init() {
  p = dlsym(... , "__tsan_acquire")
  swift_task_set_tsan_hooks(p);
}

[delcypher]

In D98810#2632928, @yln wrote:

In D98810#2632872, @delcypher wrote:

@yln Could we add a test case for some of this new functionality?

We could add a lit testcase that has something like:

RUN: %clang -fsanitize=thread %s -o %t ; %run %t | FileCheck -check-prefixes=CHECK-NO-HOOK,CHECK %s
RUN: %clang -fsanitize=thread -Dtest_task_set_tsan_hooks %s -o %t.with_hook ; %run %t.with_hook | FileCheck -check-prefixes=CHECK-HOOK,CHECK %s

// CHECK-NO-HOOK-NOT: swift_task_set_tsan_hooks CALLED

#if defined(test_swift_task_set_tsan_hooks)
void swift_task_set_tsan_hooks(void*, void*) {
  // CHECK-HOOK: swift_task_set_tsan_hooks CALLED
  printf("swift_task_set_tsan_hooks CALLED\n");
}
#endif


int main() {
  // CHECK: DONE
  printf("DONE\n");
}

That way we test both swift_task_set_tsan_hooks being available and not being available at run time.

Note I'm not sure if it's safe to call printf from the point where TSan calls swift_task_set_tsan_hooks but if it isn't we can probably work around this by setting a variable and reading it later from main().

I thought about how to test this, but concluded that a test like this does not provide much value. The real tests for this are the integration tests in the Swift repo.
Let me know if you insist on adding a test like this.

@yln

I won't insist but let me try to persuade you that having a test in compiler-rt is a good idea. It gives us defence in depth. Sure integration tests in Swift should catch issues but it would be much better to catch issues earlier than that. I would say most people who work on compiler-rt aren't testing Swift. If we have a test in compiler-rt it increases the chance that an issue relating to the compiler-rt side of the contract with Swift will be caught earlier. This might be at an engineer's desk (i.e. running tests before landing a patch) or in LLVM's open source bots.

Given that the cost of writing and running such a test is pretty low I don't really see an argument for not doing it.

We could also make the test case I proposed a lot better by using the tsan_acquire and tsan_release function pointers passed into swift_task_set_tsan_hooks to actually suppress a race injected. So we'd run the test program twice

1. With swift_task_set_tsan_hooks not defined. In that case we should find the injected race.
2. With swift_task_set_tsan_hooks defined we'll call tsan_acquire and tsan_release to mask the injected race. We'll verify no race is detected.

Doing this would (very crudely) model how we intend to use the function pointers from Swift and verify that the function pointers we will pass actually do what the Swift runtime expects.

[yln]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

Got it! We could even hijack __tsan_default_options() to avoid the need for any modifications in TSan, but that would mean Swift user code can't use it anymore. Let me think about it.

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

alternative
somewhere is swift runtime define weak symbol for some public tsan function e.g. tsan_acquire and tsan_release
it they !=null the we run with tsan so set hooks

[yln]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

We also want to avoid that because weak references disable the "two-level namespace" symbol lookup in the linker/loader, again adding a cost to the non-TSan (common) case.

However, what you suggested earlier---having a weak definition in the TSan runtime and a strong (overriding) one in the Swift runtime sounds promising---we only have the "weak def overridden?" cost in the TSan case. I am just having a little trouble getting it to work. *fingers crossed* that it works the way we want for dylibs.

[yln]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

Unfortunately, trying to provide a strong definition across *.dylibs appears to be a dead end. Our linker guys said:

In general, strong overriding weak is only guaranteed within a linkage unit (e.g. when linking .o files).
There is just one case where strong in one linkage unit overrides a weak in another. That is when the strong dylib/exe was built, it linked against a dylib with a weak-def.

This is done in the name of performance so the loader can do less work.

when the strong dylib/exe was built, it linked against a dylib with a weak-def.

This is also what __tsan_default_options() relies on. It's usually defined in the main binary, which is linked against the TSan runtime. A potential workaround would be to link the TSan runtime when building the Swift runtime, but that is also far from optimal.

Are you okay with looking up a symbol, e.g., __tsan_on_init() via dlsym() and then calling it? (Similar to what this patch does now, but more generic.)

[yln]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

friendly ping 👋 @vitalybuka

[vitalybuka]

compiler-rt/lib/tsan/rtl/tsan_interceptors_posix.cpp

2675 ↗

(On Diff #331354)

LGTM, but InitializeInterceptors probably is not a good place, I guess it should be done somewhere near tsan_rtl.cpp:435
I just noticed tsan::OnInitialize
maybe we can just change how we invoke it
I am fine with either new tsan_on_initialize or OnInitialize.

I suspect OnInitialize is used only by Google internally
So maybe we even create __tsan_on_initialize then later we can switch from and remove OnInitialize.

[yln]

Adopt already-existing tsan::OnInitialize().

[yln]

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

58–77

Should I instead introduce a new extern "C" __tsan_on_initialize()? This would feel a bit cleaner to me than to lookup the mangled C++ name. Let me know what you prefer.

[yln]

• Introduce new hooks and call old ones

[yln]

Also added a test. It only explicitly tests the "hooks present" case since all other tests already cover the "hooks absent" case.
@delcypher

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

58–77

I went ahead and made this change. I have a slight preference for the current version, but I am happy to go either way. See previous diff for the C++ version.

Let me know which version you prefer @vitalybuka

[delcypher]

@yln the linked commit from the description uses swift_task_set_tsan_hooks. I don't see any use of that here. Are going for a different approach now?

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

69

Are we relying on the fact that the Darwin`__tsan_on_initialize` definition here is mangled? If we were to make these extern "C" would dlsym() end up returning a pointer to __tsan_on_initialize() in the TSan runtime instead of the implementation we want? If this is the case we should probably be explicit that we are relying on this.

88

Do these definitions need extern "C"?

[yln]

In D98810#2646332, @delcypher wrote:

@yln the linked commit from the description uses swift_task_set_tsan_hooks. I don't see any use of that here. Are going for a different approach now?

Updated summary. Swift side is here: https://github.com/apple/swift/pull/36478

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

69

The important part is that they are static/local to the translation unit (and cannot be looked up). Using the same name here avoids the need for a "call_on_initialize_hook()` function.

88

#define SANITIZER_WEAK_DEFAULT_IMPL \
  extern "C" SANITIZER_INTERFACE_ATTRIBUTE SANITIZER_WEAK_ATTRIBUTE NOINLINE

[vitalybuka]

fix linux

[vitalybuka]

This way works for Linux as well

[vitalybuka]

bool -> int

[vitalybuka]

interface

[yln]

Fix clang-tidy:

• 'const' type qualifier on return type has no effect [clang-diagnostic-ignored-qualifiers]
• 'auto ptr' can be declared as 'auto *ptr' [llvm-qualified-auto]

[yln]

Ahh, I accidentally overwrote your very last diff. Will fix. Also thanks so much, Vitaly!

[yln]

Restoring interface changes. Removing const from interface delcartions (clang-tidy).

[yln]

@delcypher Good to go now?

[delcypher]

@yln I've suggested some documentation we might want to add to the new interface. I'll leave it up to you if you want to add it. Other than that LGTM.

compiler-rt/include/sanitizer/tsan_interface.h

161 ↗

(On Diff #332853)

@yln: Suggestion: Should we document the failed parameter and the return value? E.g.

// `failed` - `0` if the TSan runtime did not detected issues. Non zero otherwise.
// Return value - Returns `0` if the TSan runtime should exit as if no issues occurred. Non zero otherwise.

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

69

Okay. Sounds like its intentional. Could you add a small comment explaining? It's a subtle point that's very easy to miss when looking at the code.

[yln]

compiler-rt/include/sanitizer/tsan_interface.h

161 ↗

(On Diff #332853)

Thanks! Will add when committing.

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

69

Vitaly updated the code to also dlsym() on Linux (instead of weak default implementation), so this aspect of the implementation went away.

compiler-rt/lib/tsan/rtl/tsan_rtl.cpp

 //===-- tsan_rtl.cpp ------------------------------------------------------===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 //

 #endif
 static char ctx_placeholder[sizeof(Context)] ALIGNED(64);
 Context *ctx;
 
 // Can be overriden by a front-end.
 #ifdef TSAN_EXTERNAL_HOOKS
 bool OnFinalize(bool failed);
 void OnInitialize();
+extern "C" void __tsan_on_initialize();
+extern "C" bool __tsan_on_finalize(bool failed);
+#elif SANITIZER_MAC
+#include <dlfcn.h>
+// On Darwin, strong definitions overriding weak ones is only guaranteed within
+// a linkage unit (e.g., when linking .o files).  There is just one case where
+// strong in one linkage unit overrides a weak in another.  That is when the
+// strong dylib/exe was built, it linked against a dylib with a weak-def
+// (__tsan_default_options() relies on this).  We want to use these hooks from
+// other libraries (i.e., the Swift runtime) so we need to use dlsym() to look
+// them up.
+static void __tsan_on_initialize() {
+  if (auto ptr = dlsym(RTLD_DEFAULT, "__tsan_on_initialize")) {
+    return reinterpret_cast<void (*)()>(ptr)();
+  }
+}
+static bool __tsan_on_finalize(bool failed) {
+  if (auto ptr = dlsym(RTLD_DEFAULT, "__tsan_on_finalize")) {
+    return reinterpret_cast<bool (*)(bool)>(ptr)(failed);
+  }
+  return failed;
+}
 #else
 SANITIZER_WEAK_CXX_DEFAULT_IMPL
 bool OnFinalize(bool failed) {
   return failed;
 }
 SANITIZER_WEAK_CXX_DEFAULT_IMPL
 void OnInitialize() {}
+// FIXME: adopt new hooks downstream and remove old ones.
+SANITIZER_WEAK_DEFAULT_IMPL void __tsan_on_initialize() {
+  OnInitialize();
+}
+SANITIZER_WEAK_DEFAULT_IMPL bool __tsan_on_finalize(bool failed) {
+  return OnFinalize(failed);
+}
 #endif
 
 static char thread_registry_placeholder[sizeof(ThreadRegistry)];
 
 static ThreadContextBase *CreateThreadContext(u32 tid) {
   // Map thread trace when context is created.
   char name[50];
   internal_snprintf(name, sizeof(name), "trace %u", tid);
#endif
 
   if (flags()->stop_on_start) {
     Printf("ThreadSanitizer is suspended at startup (pid %d)."
            " Call __tsan_resume().\n",
            (int)internal_getpid());
     while (__tsan_resumed == 0) {}
   }
 
-  OnInitialize();
+  __tsan_on_initialize();
 }
 
 void MaybeSpawnBackgroundThread() {
   // On MIPS, TSan initialization is run before
   // __pthread_initialize_minimal_internal() is finished, so we can not spawn
   // new threads.
 #if !SANITIZER_GO && !defined(__mips__)
   static atomic_uint32_t bg_thread = {};
#endif
 
   if (common_flags()->print_suppressions)
     PrintMatchedSuppressions();
 #if !SANITIZER_GO
   if (flags()->print_benign)
     PrintMatchedBenignRaces();
 #endif
 
-  failed = OnFinalize(failed);
+  failed = __tsan_on_finalize(failed);
 
 #if TSAN_COLLECT_STATS
   StatAggregate(ctx->stat, thr->stat);
   StatOutput(ctx->stat);
 #endif
 
   return failed ? common_flags()->exitcode : 0;
 }

compiler-rt/test/tsan/on_initialize_finalize_hooks.cpp

+// RUN: %clang_tsan -O1 %s -o %t.lib -fno-sanitize=thread -shared -DBUILD_LIB=1
+// RUN: %clang_tsan -O1 %s %t.lib -o %t
+// RUN: %run %t | FileCheck %s
+
+// Test that initialization/finalization hooks are called, even when they are
+// not defined in the main executable, but by another another library that
+// doesn't directly link against the TSan runtime.
+
+#include <stdio.h>
+
+#if BUILD_LIB
+
+extern "C" void __tsan_on_initialize() {
+  printf("__tsan_on_initialize()\n");
+}
+
+extern "C" bool __tsan_on_finalize(bool failed) {
+  printf("__tsan_on_finalize()\n");
+  return failed;
+}
+
+#else  // BUILD_LIB
+
+int main() {
+  printf("main()\n");
+  return 0;
+}
+
+#endif  // BUILD_LIB
+
+// CHECK: __tsan_on_initialize()
+// CHECK: main()
+// CHECK: __tsan_on_finalize()