[PATCH] D67122: [UBSan][clang][compiler-rt] Applying non-zero offset to nullptr is undefined behaviour

[Vedant Kumar via Phabricator via cfe-commits]

vsk added inline comments.


================
Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:177
      ``-fsanitize=undefined``.
+  -  ``-fsanitize=pointer-offsetting``: Enables ``nullptr-and-nonzero-offset``
+     and ``pointer-overflow``.
----------------
Why does this need to be a separate sanitizer, as opposed to being a part of -fsanitize=pointer-overflow? Is there a need for a new group?


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4544
 
-  // If the GEP has already been reduced to a constant, leave it be.
-  if (isa<llvm::Constant>(GEPVal))
-    return GEPVal;
-
-  // Only check for overflows in the default address space.
-  if (GEPVal->getType()->getPointerAddressSpace())
-    return GEPVal;
+  // Was the GEP has already been reduced to a constant?
+  if (isa<llvm::Constant>(GEPVal)) {
----------------
`s/has//`, `s/been//`


================
Comment at: clang/lib/CodeGen/CodeGenFunction.h:4072
+  std::pair<llvm::Value * /*TotalOffset*/, llvm::Value * /*OffsetOverflows*/>
+  EmitGEPOffsetInBytes(llvm::Value *BasePtr, llvm::Value *GEPVal);
+
----------------
Why does this need to be a method in CodeGenFunction? This seems too tied to the work done in EmitCheckedInBoundsGEP to warrant inclusion here.


================
Comment at: compiler-rt/lib/ubsan/ubsan_handlers.cpp:715
+         "applying non-zero offset to non-null pointer %0 producing null "
+         "pointer is undefined behaviour")
+        << (void *)Base;
----------------
I don't think these diagnostics generally need to repeat the fact that there is UB: that seems implicit.

How about: 'non-zero offset %0 applied to null pointer', and 'non-zero offset %0 applied to non-null pointer %1 produced null'?


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D67122/new/

https://reviews.llvm.org/D67122