[local-bounds] Ignore volatile operations

[vitalybuka]

Summary

-fsanitize=local-bounds is very similar to `object-size` and
should also ignore volatile pointers.
https://clang.llvm.org/docs/UndefinedBehaviorSanitizer.html#volatile

[vitalybuka]

ping

[rsmith]

Code change looks fine, and it makes sense to me to apply the same rules to both the local-bounds and the object-size sanitizer, since they're checking the same thing. Please also update the documentation you linked to, to indicate that the local-bounds sanitizer also ignores volatile accesses.

[vitalybuka]

doc

[vitalybuka]

done

[vitalybuka]

updated doc

[rsmith]

clang/docs/UndefinedBehaviorSanitizer.rst

176–179

While we're here we should mention that local-bounds isn't part of undefined.

[vitalybuka]

add =undefined note

clang/docs/UndefinedBehaviorSanitizer.rst

 Available checks are:
 
   -  ``-fsanitize=alignment``: Use of a misaligned pointer or creation
      of a misaligned reference. Also sanitizes assume_aligned-like attributes.
   -  ``-fsanitize=bool``: Load of a ``bool`` value which is neither
      ``true`` nor ``false``.
   -  ``-fsanitize=builtin``: Passing invalid values to compiler builtins.
   -  ``-fsanitize=bounds``: Out of bounds array indexing, in cases
-     where the array bound can be statically determined.
+     where the array bound can be statically determined. The check includes
+     ``-fsanitize=array-bounds`` and ``-fsanitize=local-bounds``. Note that
+     ``-fsanitize=local-bounds`` is not included in ``-fsanitize=undefined``.
   -  ``-fsanitize=enum``: Load of a value of an enumerated type which
      is not in the range of representable values for that enumerated
      type.
   -  ``-fsanitize=float-cast-overflow``: Conversion to, from, or
      between floating-point types which would overflow the
      destination. Because the range of representable values for all
      floating-point types supported by Clang is [-inf, +inf], the only
      cases detected are conversions from floating point to integer types.
-  ``-fsanitize=vla-bound``: A variable-length array whose bound
      does not evaluate to a positive value.
   -  ``-fsanitize=vptr``: Use of an object whose vptr indicates that it is of
      the wrong dynamic type, or that its lifetime has not begun or has ended.
      Incompatible with ``-fno-rtti``. Link must be performed by ``clang++``, not
      ``clang``, to make sure C++-specific parts of the runtime library and C++
      standard libraries are present.
 
 You can also use the following check groups:
   -  ``-fsanitize=undefined``: All of the checks listed above other than
      ``float-divide-by-zero``, ``unsigned-integer-overflow``,
-     ``implicit-conversion``, and the ``nullability-*`` group of checks.
+     ``implicit-conversion``, ``local-bounds`` and the ``nullability-*`` group
+     of checks.
   -  ``-fsanitize=undefined-trap``: Deprecated alias of
      ``-fsanitize=undefined``.
   -  ``-fsanitize=implicit-integer-truncation``: Catches lossy integral
      conversions. Enables ``implicit-signed-integer-truncation`` and
      ``implicit-unsigned-integer-truncation``.
   -  ``-fsanitize=implicit-integer-arithmetic-value-change``: Catches implicit
      conversions that change the arithmetic value of the integer. Enables
      ``implicit-signed-integer-truncation`` and ``implicit-integer-sign-change``.
You can also use the following check groups:
   -  ``-fsanitize=nullability``: Enables ``nullability-arg``,
      ``nullability-assign``, and ``nullability-return``. While violating
      nullability does not have undefined behavior, it is often unintentional,
      so UBSan offers to catch it.
 
 Volatile
 --------
 
-The ``null``, ``alignment``, ``object-size``, and ``vptr`` checks do not apply
+The ``null``, ``alignment``, ``object-size``, ``local-bounds``, and ``vptr`` checks do not apply
 to pointers to types with the ``volatile`` qualifier.
 
 Minimal Runtime
 ===============
 
 There is a minimal UBSan runtime available suitable for use in production
 environments. This runtime has a small attack surface. It only provides very
 basic issue logging and deduplication, and does not support

llvm/lib/Transforms/Instrumentation/BoundsChecking.cpp

static bool addBoundsChecking(Function &F, TargetLibraryInfo &TLI,
 
   // check HANDLE_MEMORY_INST in include/llvm/Instruction.def for memory
   // touching instructions
   SmallVector<std::pair<Instruction *, Value *>, 4> TrapInfo;
   for (Instruction &I : instructions(F)) {
     Value *Or = nullptr;
     BuilderTy IRB(I.getParent(), BasicBlock::iterator(&I), TargetFolder(DL));
     if (LoadInst *LI = dyn_cast<LoadInst>(&I)) {
+      if (!LI->isVolatile())
         Or = getBoundsCheckCond(LI->getPointerOperand(), LI, DL, TLI,
                                 ObjSizeEval, IRB, SE);
     } else if (StoreInst *SI = dyn_cast<StoreInst>(&I)) {
+      if (!SI->isVolatile())
         Or = getBoundsCheckCond(SI->getPointerOperand(), SI->getValueOperand(),
                                 DL, TLI, ObjSizeEval, IRB, SE);
     } else if (AtomicCmpXchgInst *AI = dyn_cast<AtomicCmpXchgInst>(&I)) {
-      Or = getBoundsCheckCond(AI->getPointerOperand(), AI->getCompareOperand(),
+      if (!AI->isVolatile())
+        Or =
+            getBoundsCheckCond(AI->getPointerOperand(), AI->getCompareOperand(),
                                DL, TLI, ObjSizeEval, IRB, SE);
     } else if (AtomicRMWInst *AI = dyn_cast<AtomicRMWInst>(&I)) {
-      Or = getBoundsCheckCond(AI->getPointerOperand(), AI->getValOperand(), DL,
-                              TLI, ObjSizeEval, IRB, SE);
+      if (!AI->isVolatile())
+        Or = getBoundsCheckCond(AI->getPointerOperand(), AI->getValOperand(),
+                                DL, TLI, ObjSizeEval, IRB, SE);
     }
     if (Or)
       TrapInfo.push_back(std::make_pair(&I, Or));
   }
 
   // Create a trapping basic block on demand using a callback. Depending on
   // flags, this will either create a single block for the entire function or
   // will create a fresh block every time it is called.

llvm/test/Instrumentation/BoundsChecking/simple.ll

 ; CHECK-NEXT: icmp ult i64 {{.*}}, 32
 ; CHECK-NEXT: icmp ult i64 {{.*}}, 4
 ; CHECK-NEXT: or i1
 ; CHECK: trap
   store i32 3, i32* %idx, align 4
   ret void
 }
 
+; CHECK: @store_volatile
+define void @store_volatile(i64 %x) nounwind {
+  %1 = tail call i8* @calloc(i64 4, i64 %x)
+  %2 = bitcast i8* %1 to i32*
+  %idx = getelementptr inbounds i32, i32* %2, i64 8
+; CHECK-NOT: trap
+  store volatile i32 3, i32* %idx, align 4
+  ret void
+}
+
 ; CHECK: @f4
 define void @f4(i64 %x) nounwind {
   %1 = tail call i8* @realloc(i8* null, i64 %x) nounwind
   %2 = bitcast i8* %1 to i32*
   %idx = getelementptr inbounds i32, i32* %2, i64 8
 ; CHECK: trap
   %3 = load i32, i32* %idx, align 4
   ret void
; CHECK: br label
   %3 = load i8, i8 addrspace(1)* %2, align 4
   ret void
 }
 
 ; CHECK: @f12
 define i64 @f12(i64 %x, i64 %y) nounwind {
   %1 = tail call i8* @calloc(i64 1, i64 %x)
 ; CHECK: mul i64 %y, 8
+; CHECK: trap
   %2 = bitcast i8* %1 to i64*
   %3 = getelementptr inbounds i64, i64* %2, i64 %y
   %4 = load i64, i64* %3, align 8
   ret i64 %4
 }
 
+; CHECK: @load_volatile
+define i64 @load_volatile(i64 %x, i64 %y) nounwind {
+  %1 = tail call i8* @calloc(i64 1, i64 %x)
+; CHECK-NOT: trap
+  %2 = bitcast i8* %1 to i64*
+  %3 = getelementptr inbounds i64, i64* %2, i64 %y
+  %4 = load volatile i64, i64* %3, align 8
+  ret i64 %4
+}
+
 ; PR17402
 ; CHECK-LABEL: @f13
 define void @f13() nounwind {
 entry:
   br label %alive
 
 dead:
   ; Self-refential GEPs can occur in dead code.
   %incdec.ptr = getelementptr inbounds i32, i32* %incdec.ptr, i64 1
   ; CHECK: %incdec.ptr = getelementptr inbounds i32, i32* %incdec.ptr
   %l = load i32, i32* %incdec.ptr
   br label %alive
 
 alive:
   ret void
 }