tsan: properly instrument unaligned accesses

[dvyukov]

Summary

If a memory access is unaligned, emit tsan_unaligned_read/write callbacks instead of tsan_read/write.
Required to change semantics of tsan_unaligned_read/write to not do the user memory. But since they were unused (other than through sanitizer_unaligned_load/store) this is fine.
Fixes long standing issue 17:
https://code.google.com/p/thread-sanitizer/issues/detail?id=17

[kcc]

test/Instrumentation/ThreadSanitizer/unaligned.ll

12

CHECK-LABEL, here and below

test/tsan/aligned_vs_unaligned_race.cc

16

you probably want to update the test here (remove sleep)

[dvyukov]

All done
PTAL

[kcc]

LGTM with one nit

test/tsan/aligned_vs_unaligned_race.cc

1

do you still need deflake here?

[dvyukov]

test/tsan/aligned_vs_unaligned_race.cc

1

Currently all positive tests use it.
We need a larger cleanup later. But let it be consistent for now.

16

Done

[dvyukov]

Committed as rev 227230 and 227231.

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

private:
   IntegerType *OrdTy;
   // Callbacks to run-time library are computed in doInitialization.
   Function *TsanFuncEntry;
   Function *TsanFuncExit;
   // Accesses sizes are powers of two: 1, 2, 4, 8, 16.
   static const size_t kNumberOfAccessSizes = 5;
   Function *TsanRead[kNumberOfAccessSizes];
   Function *TsanWrite[kNumberOfAccessSizes];
+  Function *TsanUnalignedRead[kNumberOfAccessSizes];
+  Function *TsanUnalignedWrite[kNumberOfAccessSizes];
   Function *TsanAtomicLoad[kNumberOfAccessSizes];
   Function *TsanAtomicStore[kNumberOfAccessSizes];
   Function *TsanAtomicRMW[AtomicRMWInst::LAST_BINOP + 1][kNumberOfAccessSizes];
   Function *TsanAtomicCAS[kNumberOfAccessSizes];
   Function *TsanAtomicThreadFence;
   Function *TsanAtomicSignalFence;
   Function *TsanVptrUpdate;
   Function *TsanVptrLoad;
for (size_t i = 0; i < kNumberOfAccessSizes; ++i) {
     SmallString<32> ReadName("__tsan_read" + itostr(ByteSize));
     TsanRead[i] = checkInterfaceFunction(M.getOrInsertFunction(
         ReadName, IRB.getVoidTy(), IRB.getInt8PtrTy(), nullptr));
 
     SmallString<32> WriteName("__tsan_write" + itostr(ByteSize));
     TsanWrite[i] = checkInterfaceFunction(M.getOrInsertFunction(
         WriteName, IRB.getVoidTy(), IRB.getInt8PtrTy(), nullptr));
 
+    SmallString<64> UnalignedReadName("__tsan_unaligned_read" +
+        itostr(ByteSize));
+    TsanUnalignedRead[i] = checkInterfaceFunction(M.getOrInsertFunction(
+        UnalignedReadName, IRB.getVoidTy(), IRB.getInt8PtrTy(), nullptr));
+
+    SmallString<64> UnalignedWriteName("__tsan_unaligned_write" +
+        itostr(ByteSize));
+    TsanUnalignedWrite[i] = checkInterfaceFunction(M.getOrInsertFunction(
+        UnalignedWriteName, IRB.getVoidTy(), IRB.getInt8PtrTy(), nullptr));
+
     Type *Ty = Type::getIntNTy(M.getContext(), BitSize);
     Type *PtrTy = Ty->getPointerTo();
     SmallString<32> AtomicLoadName("__tsan_atomic" + itostr(BitSize) +
                                    "_load");
     TsanAtomicLoad[i] = checkInterfaceFunction(M.getOrInsertFunction(
         AtomicLoadName, Ty, PtrTy, OrdTy, nullptr));
 
     SmallString<32> AtomicStoreName("__tsan_atomic" + itostr(BitSize) +
if (IsWrite && isVtableAccess(I)) {
     return true;
   }
   if (!IsWrite && isVtableAccess(I)) {
     IRB.CreateCall(TsanVptrLoad,
                    IRB.CreatePointerCast(Addr, IRB.getInt8PtrTy()));
     NumInstrumentedVtableReads++;
     return true;
   }
-  Value *OnAccessFunc = IsWrite ? TsanWrite[Idx] : TsanRead[Idx];
+  const unsigned Alignment = IsWrite
+      ? cast<StoreInst>(I)->getAlignment()
+      : cast<LoadInst>(I)->getAlignment();
+  Type *OrigTy = cast<PointerType>(Addr->getType())->getElementType();
+  const uint32_t TypeSize = DL->getTypeStoreSizeInBits(OrigTy);
+  Value *OnAccessFunc = nullptr;
+  if (Alignment == 0 || Alignment >= 8 || (Alignment % (TypeSize / 8)) == 0)
+    OnAccessFunc = IsWrite ? TsanWrite[Idx] : TsanRead[Idx];
+  else
+    OnAccessFunc = IsWrite ? TsanUnalignedWrite[Idx] : TsanUnalignedRead[Idx];
   IRB.CreateCall(OnAccessFunc, IRB.CreatePointerCast(Addr, IRB.getInt8PtrTy()));
   if (IsWrite) NumInstrumentedWrites++;
   else         NumInstrumentedReads++;
   return true;
 }
 
 static ConstantInt *createOrdering(IRBuilder<> *IRB, AtomicOrdering ord) {
   uint32_t v = 0;

lib/tsan/rtl/tsan_interface.h

 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_read16(void *addr);
 
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_write1(void *addr);
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_write2(void *addr);
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_write4(void *addr);
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_write8(void *addr);
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_write16(void *addr);
 
-SANITIZER_INTERFACE_ATTRIBUTE u16 __tsan_unaligned_read2(const uu16 *addr);
-SANITIZER_INTERFACE_ATTRIBUTE u32 __tsan_unaligned_read4(const uu32 *addr);
-SANITIZER_INTERFACE_ATTRIBUTE u64 __tsan_unaligned_read8(const uu64 *addr);
-SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_write2(uu16 *addr, u16 v);
-SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_write4(uu32 *addr, u32 v);
-SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_write8(uu64 *addr, u64 v);
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_read2(const void *addr);
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_read4(const void *addr);
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_read8(const void *addr);
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_read16(const void *addr);
+
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_write2(void *addr);
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_write4(void *addr);
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_write8(void *addr);
+SANITIZER_INTERFACE_ATTRIBUTE void __tsan_unaligned_write16(void *addr);
 
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_vptr_read(void **vptr_p);
 SANITIZER_INTERFACE_ATTRIBUTE
 void __tsan_vptr_update(void **vptr_p, void *new_val);
 
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_func_entry(void *call_pc);
 SANITIZER_INTERFACE_ATTRIBUTE void __tsan_func_exit();
 

lib/tsan/rtl/tsan_interface.cc

void __tsan_read16(void *addr) {
   MemoryRead(cur_thread(), CALLERPC, (uptr)addr + 8, kSizeLog8);
 }
 
 void __tsan_write16(void *addr) {
   MemoryWrite(cur_thread(), CALLERPC, (uptr)addr, kSizeLog8);
   MemoryWrite(cur_thread(), CALLERPC, (uptr)addr + 8, kSizeLog8);
 }
 
-u16 __tsan_unaligned_read2(const uu16 *addr) {
+// __tsan_unaligned_read/write calls are emitted by compiler.
+
+void __tsan_unaligned_read2(const void *addr) {
   UnalignedMemoryAccess(cur_thread(), CALLERPC, (uptr)addr, 2, false, false);
-  return *addr;
 }
 
-u32 __tsan_unaligned_read4(const uu32 *addr) {
+void __tsan_unaligned_read4(const void *addr) {
   UnalignedMemoryAccess(cur_thread(), CALLERPC, (uptr)addr, 4, false, false);
-  return *addr;
 }
 
-u64 __tsan_unaligned_read8(const uu64 *addr) {
+void __tsan_unaligned_read8(const void *addr) {
   UnalignedMemoryAccess(cur_thread(), CALLERPC, (uptr)addr, 8, false, false);
-  return *addr;
 }
 
-void __tsan_unaligned_write2(uu16 *addr, u16 v) {
+void __tsan_unaligned_read16(const void *addr) {
+  UnalignedMemoryAccess(cur_thread(), CALLERPC, (uptr)addr, 16, false, false);
+}
+
+void __tsan_unaligned_write2(void *addr) {
   UnalignedMemoryAccess(cur_thread(), CALLERPC, (uptr)addr, 2, true, false);
-  *addr = v;
 }
 
-void __tsan_unaligned_write4(uu32 *addr, u32 v) {
+void __tsan_unaligned_write4(void *addr) {
   UnalignedMemoryAccess(cur_thread(), CALLERPC, (uptr)addr, 4, true, false);
-  *addr = v;
 }
 
-void __tsan_unaligned_write8(uu64 *addr, u64 v) {
+void __tsan_unaligned_write8(void *addr) {
   UnalignedMemoryAccess(cur_thread(), CALLERPC, (uptr)addr, 8, true, false);
-  *addr = v;
 }
 
+void __tsan_unaligned_write16(void *addr) {
+  UnalignedMemoryAccess(cur_thread(), CALLERPC, (uptr)addr, 16, true, false);
+}
+
+// __sanitizer_unaligned_load/store are for user instrumentation.
+
 extern "C" {
 SANITIZER_INTERFACE_ATTRIBUTE
-uint16_t __sanitizer_unaligned_load16(void *addr)
-    ALIAS("__tsan_unaligned_read2");
+u16 __sanitizer_unaligned_load16(const uu16 *addr) {
+  __tsan_unaligned_read2(addr);
+  return *addr;
+}
+
 SANITIZER_INTERFACE_ATTRIBUTE
-uint32_t __sanitizer_unaligned_load32(void *addr)
-    ALIAS("__tsan_unaligned_read4");
+u32 __sanitizer_unaligned_load32(const uu32 *addr) {
+  __tsan_unaligned_read4(addr);
+  return *addr;
+}
+
 SANITIZER_INTERFACE_ATTRIBUTE
-uint64_t __sanitizer_unaligned_load64(void *addr)
-    ALIAS("__tsan_unaligned_read8");
+u64 __sanitizer_unaligned_load64(const uu64 *addr) {
+  __tsan_unaligned_read8(addr);
+  return *addr;
+}
+
 SANITIZER_INTERFACE_ATTRIBUTE
-void __sanitizer_unaligned_store16(void *addr, uint16_t v)
-    ALIAS("__tsan_unaligned_write2");
+void __sanitizer_unaligned_store16(uu16 *addr, u16 v) {
+  __tsan_unaligned_write2(addr);
+  *addr = v;
+}
+
 SANITIZER_INTERFACE_ATTRIBUTE
-void __sanitizer_unaligned_store32(void *addr, uint32_t v)
-    ALIAS("__tsan_unaligned_write4");
+void __sanitizer_unaligned_store32(uu32 *addr, u32 v) {
+  __tsan_unaligned_write4(addr);
+  *addr = v;
+}
+
 SANITIZER_INTERFACE_ATTRIBUTE
-void __sanitizer_unaligned_store64(void *addr, uint64_t v)
-    ALIAS("__tsan_unaligned_write8");
+void __sanitizer_unaligned_store64(uu64 *addr, u64 v) {
+  __tsan_unaligned_write8(addr);
+  *addr = v;
 }
+}  // extern "C"
 
 void __tsan_acquire(void *addr) {
   Acquire(cur_thread(), CALLERPC, (uptr)addr);
 }
 
 void __tsan_release(void *addr) {
   Release(cur_thread(), CALLERPC, (uptr)addr);
 }

test/Instrumentation/ThreadSanitizer/unaligned.ll

+; RUN: opt < %s -tsan -S | FileCheck %s
+
+target datalayout = "e-p:64:64:64-i1:8:8-i8:8:8-i16:16:16-i32:32:32-i64:64:64-f32:32:32-f64:64:64-v64:64:64-v128:128:128-a0:0:64-s0:64:64-f80:128:128-n8:16:32:64-S128"
+
+define i16 @test_unaligned_read2(i16* %a) sanitize_thread {
+entry:
+  %tmp1 = load i16* %a, align 1
+  ret i16 %tmp1
+}
+
+; CHECK-LABEL: define i16 @test_unaligned_read2(i16* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i16* %a to i8*
+; CHECK-NEXT:   call void @__tsan_unaligned_read2(i8* %1)
+; CHECK-NEXT:   %tmp1 = load i16* %a, align 1
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i16
+
+define i32 @test_unaligned_read4(i32* %a) sanitize_thread {
+entry:
+  %tmp1 = load i32* %a, align 2
+  ret i32 %tmp1
+}
+
+; CHECK-LABEL: define i32 @test_unaligned_read4(i32* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i32* %a to i8*
+; CHECK-NEXT:   call void @__tsan_unaligned_read4(i8* %1)
+; CHECK-NEXT:   %tmp1 = load i32* %a, align 2
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i32
+
+define i64 @test_unaligned_read8(i64* %a) sanitize_thread {
+entry:
+  %tmp1 = load i64* %a, align 4
+  ret i64 %tmp1
+}
+
+; CHECK-LABEL: define i64 @test_unaligned_read8(i64* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i64* %a to i8*
+; CHECK-NEXT:   call void @__tsan_unaligned_read8(i8* %1)
+; CHECK-NEXT:   %tmp1 = load i64* %a, align 4
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i64
+
+define i128 @test_unaligned_read16(i128* %a) sanitize_thread {
+entry:
+  %tmp1 = load i128* %a, align 1
+  ret i128 %tmp1
+}
+
+; CHECK-LABEL: define i128 @test_unaligned_read16(i128* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i128* %a to i8*
+; CHECK-NEXT:   call void @__tsan_unaligned_read16(i8* %1)
+; CHECK-NEXT:   %tmp1 = load i128* %a, align 1
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i128
+
+define i128 @test_aligned_read16(i128* %a) sanitize_thread {
+entry:
+  %tmp1 = load i128* %a, align 8
+  ret i128 %tmp1
+}
+
+; CHECK-LABEL: define i128 @test_aligned_read16(i128* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i128* %a to i8*
+; CHECK-NEXT:   call void @__tsan_read16(i8* %1)
+; CHECK-NEXT:   %tmp1 = load i128* %a, align 8
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret i128
+
+define void @test_unaligned_write2(i16* %a) sanitize_thread {
+entry:
+  store i16 1, i16* %a, align 1
+  ret void
+}
+
+; CHECK-LABEL: define void @test_unaligned_write2(i16* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i16* %a to i8*
+; CHECK-NEXT:   call void @__tsan_unaligned_write2(i8* %1)
+; CHECK-NEXT:   store i16 1, i16* %a, align 1
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void
+
+define void @test_unaligned_write4(i32* %a) sanitize_thread {
+entry:
+  store i32 1, i32* %a, align 1
+  ret void
+}
+
+; CHECK-LABEL: define void @test_unaligned_write4(i32* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i32* %a to i8*
+; CHECK-NEXT:   call void @__tsan_unaligned_write4(i8* %1)
+; CHECK-NEXT:   store i32 1, i32* %a, align 1
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void
+
+define void @test_unaligned_write8(i64* %a) sanitize_thread {
+entry:
+  store i64 1, i64* %a, align 1
+  ret void
+}
+
+; CHECK-LABEL: define void @test_unaligned_write8(i64* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i64* %a to i8*
+; CHECK-NEXT:   call void @__tsan_unaligned_write8(i8* %1)
+; CHECK-NEXT:   store i64 1, i64* %a, align 1
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void
+
+define void @test_unaligned_write16(i128* %a) sanitize_thread {
+entry:
+  store i128 1, i128* %a, align 1
+  ret void
+}
+
+; CHECK-LABEL: define void @test_unaligned_write16(i128* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i128* %a to i8*
+; CHECK-NEXT:   call void @__tsan_unaligned_write16(i8* %1)
+; CHECK-NEXT:   store i128 1, i128* %a, align 1
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void
+
+define void @test_aligned_write16(i128* %a) sanitize_thread {
+entry:
+  store i128 1, i128* %a, align 8
+  ret void
+}
+
+; CHECK-LABEL: define void @test_aligned_write16(i128* %a)
+; CHECK:        call void @__tsan_func_entry(i8* %0)
+; CHECK-NEXT:   %1 = bitcast i128* %a to i8*
+; CHECK-NEXT:   call void @__tsan_write16(i8* %1)
+; CHECK-NEXT:   store i128 1, i128* %a, align 8
+; CHECK-NEXT:   call void @__tsan_func_exit()
+; CHECK: ret void

test/tsan/aligned_vs_unaligned_race.cc

-// RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
+// RUN: %clangxx_tsan -O1 %s -o %t && %deflake %run %t | FileCheck %s
 // Race between an aligned access and an unaligned access, which
 // touches the same memory region.
-// This is a real race which is not detected by tsan.
-// https://code.google.com/p/thread-sanitizer/issues/detail?id=17
-#include <pthread.h>
-#include <stdio.h>
+#include "test.h"
 #include <stdint.h>
 
 uint64_t Global[2];
 
 void *Thread1(void *x) {
   Global[1]++;
+  barrier_wait(&barrier);
   return NULL;
 }
 
 void *Thread2(void *x) {
+  barrier_wait(&barrier);
   char *p1 = reinterpret_cast<char *>(&Global[0]);
-  uint64_t *p4 = reinterpret_cast<uint64_t *>(p1 + 1);
-  (*p4)++;
+  struct __attribute__((packed, aligned(1))) u_uint64_t { uint64_t val; };
+  u_uint64_t *p4 = reinterpret_cast<u_uint64_t *>(p1 + 1);
+  (*p4).val++;
   return NULL;
 }
 
 int main() {
+  barrier_init(&barrier, 2);
   pthread_t t[2];
   pthread_create(&t[0], NULL, Thread1, NULL);
   pthread_create(&t[1], NULL, Thread2, NULL);
   pthread_join(t[0], NULL);
   pthread_join(t[1], NULL);
   printf("Pass\n");
-  // CHECK-NOT: ThreadSanitizer: data race
+  // CHECK: ThreadSanitizer: data race
   // CHECK: Pass
   return 0;
 }

test/tsan/unaligned_norace.cc

 // RUN: %clangxx_tsan -O1 %s -o %t && %run %t 2>&1 | FileCheck %s
 #include <pthread.h>
 #include <stdio.h>
 #include <stdlib.h>
 #include <stdint.h>
 
 uint64_t objs[8*3*3*2][3];
 
 extern "C" {
-uint16_t __tsan_unaligned_read2(void *addr);
-uint32_t __tsan_unaligned_read4(void *addr);
-uint64_t __tsan_unaligned_read8(void *addr);
-void __tsan_unaligned_write2(void *addr, uint16_t v);
-void __tsan_unaligned_write4(void *addr, uint32_t v);
-void __tsan_unaligned_write8(void *addr, uint64_t v);
+void __tsan_unaligned_read2(void *addr);
+void __tsan_unaligned_read4(void *addr);
+void __tsan_unaligned_read8(void *addr);
+void __tsan_unaligned_write2(void *addr);
+void __tsan_unaligned_write4(void *addr);
+void __tsan_unaligned_write8(void *addr);
 }
 
 static void access(char *p, int sz, int rw) {
   if (rw) {
     switch (sz) {
-    case 0: __tsan_unaligned_write2(p, 0); break;
-    case 1: __tsan_unaligned_write4(p, 0); break;
-    case 2: __tsan_unaligned_write8(p, 0); break;
+    case 0: __tsan_unaligned_write2(p); break;
+    case 1: __tsan_unaligned_write4(p); break;
+    case 2: __tsan_unaligned_write8(p); break;
     default: exit(1);
     }
   } else {
     switch (sz) {
     case 0: __tsan_unaligned_read2(p); break;
     case 1: __tsan_unaligned_read4(p); break;
     case 2: __tsan_unaligned_read8(p); break;
     default: exit(1);