[asan] Do not instrument pointers with address space attributes

[zaks.anna]

Summary

Do not instrument pointers with address space attributes since we cannot track them anyway. Instrumenting them results in false positives in ASan and a compiler crash in TSan. (The compiler should not crash in any case, but that's a different problem.)

[kubamracek]

LGTM with two nits.

lib/Transforms/Instrumentation/AddressSanitizer.cpp

953

Are we sure the cast is *always* valid? Maybe a dyn_cast + if would be better, but it’s up to you.

test/Instrumentation/ThreadSanitizer/tsan_address_space_attr.ll

32

Can we check for the presence of the instrumentation call instead? The presence or non-presence of “addrspacecast” is a secondary thing.

[kubamracek]

test/Instrumentation/ThreadSanitizer/tsan_address_space_attr.ll

32

(or CHECK-NOT for both if the previous code inserted an invalid addrspacecast)

[zaks.anna]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

953

I'll double check again, but I think we do always get PointerType here.

test/Instrumentation/ThreadSanitizer/tsan_address_space_attr.ll

32

Can you clarify what you want to test for?

Codegen crashes on the addrspacecast instruction. I can also test that the load is not instrumented with an additional check.

[kubamracek]

test/Instrumentation/ThreadSanitizer/tsan_address_space_attr.ll

32

I can also test that the load is not instrumented with an additional check.

Yes please, that’s exactly what I meant.

[zaks.anna]

Closed by r273339.

lib/Transforms/Instrumentation/AddressSanitizer.cpp

Value *AddressSanitizer::isInterestingMemoryAccess(Instruction *I,
   } else if (AtomicCmpXchgInst *XCHG = dyn_cast<AtomicCmpXchgInst>(I)) {
     if (!ClInstrumentAtomics) return nullptr;
     *IsWrite = true;
     *TypeSize = DL.getTypeStoreSizeInBits(XCHG->getCompareOperand()->getType());
     *Alignment = 0;
     PtrOperand = XCHG->getPointerOperand();
   }
 
+  // Do not instrument acesses from different address spaces; we cannot deal
+  // with them.
+  if (PtrOperand) {
+    Type *PtrTy = cast<PointerType>(PtrOperand->getType()->getScalarType());
+    if (PtrTy->getPointerAddressSpace() != 0)
+      return nullptr;
+  }
+
   // Treat memory accesses to promotable allocas as non-interesting since they
   // will not cause memory violations. This greatly speeds up the instrumented
   // executable at -O0.
   if (ClSkipPromotableAllocas)
     if (auto AI = dyn_cast_or_null<AllocaInst>(PtrOperand))
       return isInterestingAlloca(*AI) ? AI : nullptr;
 
   return PtrOperand;

lib/Transforms/Instrumentation/ThreadSanitizer.cpp

if (GlobalVariable *GV = dyn_cast<GlobalVariable>(Addr)) {
     if (GV->hasSection()) {
       StringRef SectionName = GV->getSection();
       // Check if the global is in the PGO counters section.
       if (SectionName.endswith(getInstrProfCountersSectionName(
             /*AddSegment=*/false)))
         return false;
     }
   }
+
+  // Do not instrument acesses from different address spaces; we cannot deal
+  // with them.
+  if (Addr) {
+    Type *PtrTy = cast<PointerType>(Addr->getType()->getScalarType());
+    if (PtrTy->getPointerAddressSpace() != 0)
+      return false;
+  }
+
   return true;
 }
 
 bool ThreadSanitizer::addrPointsToConstantData(Value *Addr) {
   // If this is a GEP, just analyze its pointer operand.
   if (GetElementPtrInst *GEP = dyn_cast<GetElementPtrInst>(Addr))
     Addr = GEP->getPointerOperand();
 

test/Instrumentation/AddressSanitizer/asan_address_space_attr.ll

+; RUN: opt < %s -asan -S | FileCheck %s
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+; Checks that we do not instrument loads and stores comming from custom address space.
+; These result in invalid (false positive) reports.
+; int foo(int argc, const char * argv[]) {
+;   void *__attribute__((address_space(256))) *gs_base = (((void * __attribute__((address_space(256))) *)0));
+;   void *somevalue = gs_base[-1];
+;   return somevalue;
+; }
+
+define i32 @foo(i32 %argc, i8** %argv) sanitize_address {
+entry:
+  %retval = alloca i32, align 4
+  %argc.addr = alloca i32, align 4
+  %argv.addr = alloca i8**, align 8
+  %gs_base = alloca i8* addrspace(256)*, align 8
+  %somevalue = alloca i8*, align 8
+  store i32 0, i32* %retval, align 4
+  store i32 %argc, i32* %argc.addr, align 4
+  store i8** %argv, i8*** %argv.addr, align 8
+  store i8* addrspace(256)* null, i8* addrspace(256)** %gs_base, align 8
+  %0 = load i8* addrspace(256)*, i8* addrspace(256)** %gs_base, align 8
+  %arrayidx = getelementptr inbounds i8*, i8* addrspace(256)* %0, i64 -1
+  %1 = load i8*, i8* addrspace(256)* %arrayidx, align 8
+  store i8* %1, i8** %somevalue, align 8
+  %2 = load i8*, i8** %somevalue, align 8
+  %3 = ptrtoint i8* %2 to i32
+  ret i32 %3
+}
+; CHECK-NOT: call void @__asan_report_load8

test/Instrumentation/ThreadSanitizer/tsan_address_space_attr.ll

+; RUN: opt < %s -tsan -S | FileCheck %s
+target datalayout = "e-m:o-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-apple-macosx10.11.0"
+
+; Checks that we do not instrument loads and stores comming from custom address space.
+; These result in crashing the compiler.
+; int foo(int argc, const char * argv[]) {
+;   void *__attribute__((address_space(256))) *gs_base = (((void * __attribute__((address_space(256))) *)0));
+;   void *somevalue = gs_base[-1];
+;   return somevalue;
+; }
+
+define i32 @foo(i32 %argc, i8** %argv) sanitize_thread {
+entry:
+  %retval = alloca i32, align 4
+  %argc.addr = alloca i32, align 4
+  %argv.addr = alloca i8**, align 8
+  %gs_base = alloca i8* addrspace(256)*, align 8
+  %somevalue = alloca i8*, align 8
+  store i32 0, i32* %retval, align 4
+  store i32 %argc, i32* %argc.addr, align 4
+  store i8** %argv, i8*** %argv.addr, align 8
+  store i8* addrspace(256)* null, i8* addrspace(256)** %gs_base, align 8
+  %0 = load i8* addrspace(256)*, i8* addrspace(256)** %gs_base, align 8
+  %arrayidx = getelementptr inbounds i8*, i8* addrspace(256)* %0, i64 -1
+  %1 = load i8*, i8* addrspace(256)* %arrayidx, align 8
+  store i8* %1, i8** %somevalue, align 8
+  %2 = load i8*, i8** %somevalue, align 8
+  %3 = ptrtoint i8* %2 to i32
+  ret i32 %3
+}
+; CHECK-NOT: addrspacecast