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

[Richard Smith - zygoloid via Phabricator via llvm-commits]

rsmith added inline comments.


================
Comment at: clang/docs/ReleaseNotes.rst:63
 
+* As per C++ and C Standards (C++: ``[expr.add]``; C17: 6.5.6p8), applying
+  non-zero offset to ``nullptr`` (or making non-``nullptr`` a ``nullptr``,
----------------
In C, even adding 0 to a null pointer is undefined. Did this change in C17?


================
Comment at: clang/docs/ReleaseNotes.rst:68
+  (`D66608 <https://reviews.llvm.org/D66608>`_ ``[InstCombine] icmp eq/ne (gep
+  inbounds P, Idx..), null -> icmp eq/ne P, null``) LLVM middle-end uses those
+  guarantees for transformations. If the source contains such UB's, said code
----------------
LLVM -> the LLVM


================
Comment at: clang/docs/ReleaseNotes.rst:69
+  inbounds P, Idx..), null -> icmp eq/ne P, null``) LLVM middle-end uses those
+  guarantees for transformations. If the source contains such UB's, said code
+  may now be miscompiled. Undefined Behaviour Sanitizer
----------------
"undefined behavior", like (eg) "code", is generally treated as an uncountable noun, so this should be "If the source contains such undefined behavior, said code [...]" rather than "If the source contains such undefined behavior*s*, said code [...]".


================
Comment at: clang/docs/ReleaseNotes.rst:238
 
-- ...
+- * ``pointer-overflow`` check was extended added to catch the cases where
+    a non-zero offset being applied, either to a ``nullptr``, or the result
----------------
Reusing this group seems a little surprising, since the new checks don't seem to have anything to do with overflow. Is the general idea that this warning identifies places where pointer artihmetic leaves the complete object (where, for now, we only catch the case where it wraps around the address space or leaves / reaches a hypothetical size-0 object at the null address)?


================
Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4659
+  // and only in the default address space
+  bool ShallPerformOverflowCheck =
+      !isa<llvm::Constant>(GEPVal) && PtrTy->getPointerAddressSpace() == 0;
----------------
Remove the `Shall`s here. They don't add anything.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122