[asan] Add options -asan-detect-invalid-pointer-cmp and -asan-detect-invalid-pointer-sub options.

[pgousseau]

Summary

This is in preparation to a driver patch to add gcc 8's -fsanitize=pointer-compare and -fsanitize=pointer-subtract.
Disabled by default as this is still an experimental feature.
This is the llvm side of https://reviews.llvm.org/D59221

[pgousseau]

Ping!

[morehouse]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

575 ↗

(On Diff #190117)

These parameter lists are getting too long. Do we need to store the options or can we just check ClInvalidPointer* instead? Or, can we use an options struct?

test/Instrumentation/AddressSanitizer/asan-detect-invalid-pointer-pair.ll

2 ↗

(On Diff #190117)

I think we can do --check-prefixes=CMP,NOSUB.

12 ↗

(On Diff #190117)

Do we also need to add --check-prefix=ALL?

[vitalybuka]

What is exactly going to be checked by -fsanitize=pointer-compare and -fsanitize=pointer-subtract?
Why this needs to be done in AddressSanitizer?
Is this going just checheck that pointers are from the same allocation?

[pgousseau]

Use backend option directly rather than introducing new parameters.
Add missing ALL check to test.

[pgousseau]

lib/Transforms/Instrumentation/AddressSanitizer.cpp

575 ↗

(On Diff #190117)

Yes the parameter are not necessary, I have removed them thanks!

[pgousseau]

In D59220#1443749, @vitalybuka wrote:

What is exactly going to be checked by -fsanitize=pointer-compare and -fsanitize=pointer-subtract?
Why this needs to be done in AddressSanitizer?
Is this going just checheck that pointers are from the same allocation?

-fsanitize=pointer-compare and -fsanitize=pointer-subtract instrument comparison operation (<, <=, >, >=) and '-' operation with pointer operands.
It relies on AddressSanitizer to check the origin of the allocation. gcc also requires -fsanitize=address for -fsanitize=pointer-compare and -fsanitize=pointer-subtract, so I think it is expected to depend on Asan? Thanks!

[morehouse]

test/Instrumentation/AddressSanitizer/asan-detect-invalid-pointer-pair.ll

2 ↗

(On Diff #190117)

Well now we can just do --check-prefixes=CMP,NOSUB,ALL

[pgousseau]

test/Instrumentation/AddressSanitizer/asan-detect-invalid-pointer-pair.ll

2 ↗

(On Diff #190117)

Done thanks!

llvm/trunk/lib/Transforms/Instrumentation/AddressSanitizer.cpp

static cl::opt<bool> ClInitializers("asan-initialization-order",
                                     cl::desc("Handle C++ initializer order"),
                                     cl::Hidden, cl::init(true));
 
 static cl::opt<bool> ClInvalidPointerPairs(
     "asan-detect-invalid-pointer-pair",
     cl::desc("Instrument <, <=, >, >=, - with pointer operands"), cl::Hidden,
     cl::init(false));
 
+static cl::opt<bool> ClInvalidPointerCmp(
+    "asan-detect-invalid-pointer-cmp",
+    cl::desc("Instrument <, <=, >, >= with pointer operands"), cl::Hidden,
+    cl::init(false));
+
+static cl::opt<bool> ClInvalidPointerSub(
+    "asan-detect-invalid-pointer-sub",
+    cl::desc("Instrument - operations with pointer operands"), cl::Hidden,
+    cl::init(false));
+
 static cl::opt<unsigned> ClRealignStack(
     "asan-realign-stack",
     cl::desc("Realign stack to the value of this flag (power of two)"),
     cl::Hidden, cl::init(32));
 
 static cl::opt<int> ClInstrumentationWithCallsThreshold(
     "asan-instrumentation-with-call-threshold",
     cl::desc(

 
 static bool isPointerOperand(Value *V) {
   return V->getType()->isPointerTy() || isa<PtrToIntInst>(V);
 }
 
 // This is a rough heuristic; it may cause both false positives and
 // false negatives. The proper implementation requires cooperation with
 // the frontend.
-static bool isInterestingPointerComparisonOrSubtraction(Instruction *I) {
+static bool isInterestingPointerComparison(Instruction *I) {
   if (ICmpInst *Cmp = dyn_cast<ICmpInst>(I)) {
-    if (!Cmp->isRelational()) return false;
-  } else if (BinaryOperator *BO = dyn_cast<BinaryOperator>(I)) {
-    if (BO->getOpcode() != Instruction::Sub) return false;
+    if (!Cmp->isRelational())
+      return false;
+  } else {
+    return false;
+  }
+  return isPointerOperand(I->getOperand(0)) &&
+         isPointerOperand(I->getOperand(1));
+}
+
+// This is a rough heuristic; it may cause both false positives and
+// false negatives. The proper implementation requires cooperation with
+// the frontend.
+static bool isInterestingPointerSubtraction(Instruction *I) {
+  if (BinaryOperator *BO = dyn_cast<BinaryOperator>(I)) {
+    if (BO->getOpcode() != Instruction::Sub)
+      return false;
   } else {
     return false;
   }
   return isPointerOperand(I->getOperand(0)) &&
          isPointerOperand(I->getOperand(1));
 }
 
 bool AddressSanitizer::GlobalIsLinkerInitialized(GlobalVariable *G) {
for (auto &Inst : BB) {
           if (MaybeMask) {
             if (TempsToInstrument.count(Addr))
               continue; // We've seen this (whole) temp in the current BB.
           } else {
             if (!TempsToInstrument.insert(Addr).second)
               continue; // We've seen this temp in the current BB.
           }
         }
-      } else if (ClInvalidPointerPairs &&
-                 isInterestingPointerComparisonOrSubtraction(&Inst)) {
+      } else if (((ClInvalidPointerPairs || ClInvalidPointerCmp) &&
+                  isInterestingPointerComparison(&Inst)) ||
+                 ((ClInvalidPointerPairs || ClInvalidPointerSub) &&
+                  isInterestingPointerSubtraction(&Inst))) {
         PointerComparisonsOrSubtracts.push_back(&Inst);
         continue;
       } else if (isa<MemIntrinsic>(Inst)) {
         // ok, take it.
       } else {
         if (isa<AllocaInst>(Inst)) NumAllocas++;
         CallSite CS(&Inst);
         if (CS) {

llvm/trunk/test/Instrumentation/AddressSanitizer/asan-detect-invalid-pointer-pair.ll

+; RUN: opt < %s -asan -asan-detect-invalid-pointer-cmp -S \
+; RUN:     | FileCheck %s --check-prefixes=CMP,NOSUB,ALL
+; RUN: opt < %s -asan -asan-detect-invalid-pointer-sub -S \
+; RUN:     | FileCheck %s --check-prefixes=SUB,NOCMP,ALL
+; RUN: opt < %s -asan -asan-detect-invalid-pointer-pair -S \
+; RUN:     | FileCheck %s --check-prefixes=CMP,SUB,ALL
+; Support instrumentation of invalid pointer pair detection.
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+
+define i32 @mycmp(i8* %p, i8* %q) sanitize_address {
+; ALL-LABEL: @mycmp
+; NOCMP-NOT: call void @__sanitizer_ptr_cmp
+; CMP: [[P:%[0-9A-Za-z]+]] = ptrtoint i8* %p to i64
+; CMP: [[Q:%[0-9A-Za-z]+]] = ptrtoint i8* %q to i64
+  %x = icmp ule i8* %p, %q
+; CMP: call void @__sanitizer_ptr_cmp(i64 [[P]], i64 [[Q]])
+  %y = zext i1 %x to i32
+  ret i32 %y
+}
+
+define i32 @mysub(i8* %p, i8* %q) sanitize_address {
+; ALL-LABEL: @mysub
+; NOSUB-NOT: call void @__sanitizer_ptr_sub
+; SUB: [[P:%[0-9A-Za-z]+]] = ptrtoint i8* %p to i64
+; SUB: [[Q:%[0-9A-Za-z]+]] = ptrtoint i8* %q to i64
+  %x = ptrtoint i8* %p to i64
+  %y = ptrtoint i8* %q to i64
+  %z = sub i64 %x, %y
+; SUB: call void @__sanitizer_ptr_sub(i64 [[P]], i64 [[Q]])
+  %w = trunc i64 %z to i32
+  ret i32 %w
+}