RFC: Add/update TSAN submode for lock correctness validation only

Runtimes Sanitizers
1

TLDR

This RFC proposes introducing a lightweight ThreadSanitizer submode that focuses exclusively on lock correctness validation, enabled by:

-fsanitize=thread-lock-correctness
# other suggestion: -fsanitize=thread-dd

Proposal

Currently, -fsanitize=thread offers detection for lock order inversions (potential deadlocks) and other lock misuses. These features can be enabled on top of TSan’s regular data-race detection functionality, but cannot be enabled without also enabling data-race detection.

Being able to only enable those lock-correctness related features without enabling data-race detection too would enable us to:

  • Validate lock-correctness for programs which require more address space than permitted by the ThreadSanitizer memory mappings (see here).
  • Validate lock-correctness in combination with other sanitizers, which have an incompatibility caused by TSan’s shadow-memory/instrumentation to memory accesses.
  • Make available a subset of the TSan features for 32-bit platforms (or any other platform without supported mappings!)

This seems to be in line with the current TSAN code: compiler-rt/lib/tsan/dd already contains a standalone runtime for only the deadlock detector. However, this is not integrated with any part of the compiler and there is no way of enabling it. It is effectively dead code.
(The downstream implementation in GCC does not make use of the standalone dd either.)

This proposes to introduce compiler-rt/lib/tsan_lc as a lightweight version of the thread-sanitizer without the data-race detection feature, enabled with -fsanitize=thread-lock-correctness.

Having this be part of libsanitizer rather than an external library using only LD_PRELOAD interception still makes sense because we can make use of existing infrastructure and the compiler instrumentation makes performant stack-trace generation possible when using second_deadlock_stack=1.

If accepted, after upstreaming this to LLVM, I intend to port it to GCC as well.

1 Like
2

I’m not immediately sure who to ping since there is no CodeOwner for TSan.
Pinging some recent/frequent contributors: @DanBlackwell @fiigii @MaskRay @vitalybuka
Please let me know if I missed someone.

3

cc: @tsan

4

I think this is an interesting proposal, thanks for writing it up! I believe that maybe I’m thinking of this from a different angle, so I have a couple of questions:

Based on the issues you describe here being related to shadow memory incompatibility (and not runtime library size), do you think it makes sense to instead add an option that forces the existing TSan runtime to skip shadow memory allocation and disables the race detector checks? If not, do you imagine the ‘regular’ TSan runtime bundling in the deadlock detector, or in order to do data race and deadlock detection simultaneously you would load both runtimes?

1 Like
5

Hi, thanks for the reply!

do you think it makes sense to instead add an option that forces the existing TSan runtime to skip shadow memory allocation and disables the race detector checks?

I tried this first but feel like it is too intrusive. The memory-access instrumentation is compiler-inserted, so disabling it at runtime requires a branch on every memory access. There are also a lot of interceptors we don’t want for the deadlock-detection-only mode, and they would all need the same check.

If not, do you imagine the ‘regular’ TSan runtime bundling in the deadlock detector, or in order to do data race and deadlock detection simultaneously you would load both runtimes?

Right now I have a working implementation as a new separate runtime, which cannot be composed with the regular TSan runtime. I’d imagine the regular TSan runtime to keep its deadlock detection unchanged then.
Having the two as separate but composable runtimes would be really cool, but also has drawbacks:

  • TSan’s deadlock state is embedded in SyncVar and comes for free with the shadow memory lookup already needed for race detection. Having two composable runtimes would require an additional map lookup on every lock operation for the regular TSan runtime.
    I’m also not sure how the stack-trace generation would nicely compose between both, so that it doesn’t do work twice on every function entry/exit.

  • Both runtimes would want to intercept the same pthread_* functions. I think there might be a way to compose the interceptions, but it would be hard to get right.

6

I tried this first but feel like it is too intrusive. The memory-access instrumentation is compiler-inserted, so disabling it at runtime requires a branch on every memory access.

There are many ‘hidden’ flags in the LLVM pass [here], for your prototype did you wire up the -fsanitize=thread-lock-correctness flag to use that pass and insert just the instrumentation you need (and none of what you don’t)?

There are also a lot of interceptors we don’t want for the deadlock-detection-only mode, and they would all need the same check.

I agree, though I suppose there may be some loader magic you could play to ‘overwrite’ the unneeded interceptors with wrappers that just call directly through to the real functions (e.g. have a libTSanDeadlockInterceptorPassthroughs that you preload after the TSan library is loaded). It would be brittle to new interceptors being added to the TSan runtime though.

It looks like @tsan introduced the dd directory [here]; so he may have some more insight as to why it got integrated into TSan rather than having a standalone option.

7

did you wire up the -fsanitize=thread-lock-correctness flag to use that pass and insert just the instrumentation you need (and none of what you don’t)?

Yes, exactly. Sorry, I misunderstood earlier. I thought you were asking whether it made sense to disable this at runtime via TSAN_OPTIONS=....