This is an archive of the discontinued LLVM Phabricator instance.

[tsan] Add support for pointer typed atomic stores, loads, and cmpxchg
ClosedPublic

Authored by zaks.anna on Mar 2 2016, 3:30 PM.

Details

Summary

TSan instrumentation functions for atomic stores, loads, and cmpxchg work on integer value types. This patch adds casts before calling TSan instrumentation functions in cases where the value is a pointer.

Diff Detail

Event Timeline

zaks.anna updated this revision to Diff 49681.Mar 2 2016, 3:30 PM
zaks.anna retitled this revision from to [tsan] Add support for pointer typed atomic stores, loads, and cmpxchg.
zaks.anna updated this object.
zaks.anna added reviewers: kcc, dvyukov.
zaks.anna added a subscriber: llvm-commits.
dvyukov edited edge metadata.Mar 3 2016, 11:43 PM

How did it work without these casts all that time?.. Is it the case that programs that used atomic<void*> failed to compile?

test/Instrumentation/ThreadSanitizer/atomic.ll
1196

Are you sure that we don't run these tests on 32-bit platforms? It will fail on 32-bit platform, right?

How did it work without these casts all that time?.. Is it the case that programs that used atomic<void*> failed to compile?

Looks like clang's IRGen inserts the bit casts in this case. (It's possible that it was written that way because the atomic loads and stores emission went through the same interface as emission for other atomics that were restricted to int values.) However, other front ends (such as Swift) do not insert the bit casts. Since these IR instructions are defined to work with pointer types, LLVM should support them even if the clang IRGen does not emit such code.

The cmpxchg was extended to allow pointer type operands on Feb 19th (see r261281) .

test/Instrumentation/ThreadSanitizer/atomic.ll
1196

I believe this will not fail since the test specifies the target layout, where pointers are 64 bits long:

target datalayout = "e-p:64:64:64-...

"p[n]:<size>:<abi>:<pref>

This specifies the size of a pointer and its <abi> and <pref>erred alignments for address space n. All sizes are in bits.  
The address space, n, is optional, and if not specified, denotes the default address space 0. The value of n must be in the range [1,2^23)."
dvyukov accepted this revision.Mar 4 2016, 11:59 AM
dvyukov edited edge metadata.

Eager to see Tsan for Swift!
LGTM

test/Instrumentation/ThreadSanitizer/atomic.ll
1196

ack

This revision is now accepted and ready to land.Mar 4 2016, 11:59 AM
This revision was automatically updated to reflect the committed changes.