[PATCH] D83867: [TSan] Add option for emitting compound read-write instrumentation

Thu Jul 16 06:48:57 PDT 2020

melver added a comment.

In D83867#2155324 <https://reviews.llvm.org/D83867#2155324>, @glider wrote:

> What happens if the load and the store are separated by a barrier atomic load or store? Will they also be combined into a single operation?

Hmm, good question. My intent wasn't to change the existing behaviour of how it compounds reads and writes, so I think it shouldn't be in this change.

I added a test and it does compound them. This also affects normal TSAN actually, which by default compounds reads and writes to same addr to a single __tsan_write. Let me do this as a separate change after this:  https://reviews.llvm.org/D83949

================
Comment at: llvm/test/Instrumentation/ThreadSanitizer/read_before_write.ll:21
+; CHECK-UNOPT: __tsan_write
+; CHECK-COMPOUND: __tsan_read_write
 ; CHECK: ret void
----------------
dvyukov wrote:
> I think __tsan_read expectation will be matched against __tsan_read_write call. Is there a single way to refine it? Add an opening bracket or something?
I can just add the size.

================
Comment at: llvm/test/Instrumentation/ThreadSanitizer/read_before_write.ll:78
+; CHECK-COMPOUND-VOLATILE: __tsan_volatile_read
+; CHECK-COMPOUND: __tsan_read_write
+; CHECK-COMPOUND-VOLATILE: __tsan_volatile_write
----------------
glider wrote:
> Don't we want to treat pairs of volatile loads and stores as separate accesses? E.g. a volatile load may be racing with a completely different store somewhere else.
Yes. Note the difference between CHECK-COMPOUND and CHECK-COMPOUND-VOLATILE.

For the kernel we'll have the CHECK-COMPOUND-VOLATILE behaviour, i.e. -tsan-distinguish-volatile -tsan-compound-read-before-write is set. And it will treat them separately and not compound them.

If volatiles are not to be distinguished, it'll just compound them, i.e. revert to the standard behaviour (arguably also perfectly in line with the non-kernel standard, because racing volatiles are still a data race).

Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83867/new/

https://reviews.llvm.org/D83867