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

[Roman Lebedev via Phabricator via llvm-commits]

lebedev.ri added inline comments.


================
Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:177
      ``-fsanitize=undefined``.
+  -  ``-fsanitize=pointer-offsetting``: Enables ``nullptr-and-nonzero-offset``
+     and ``pointer-overflow``.
----------------
vsk wrote:
> 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?
That ties in with the question i'm asking myself in last update:
https://reviews.llvm.org/D67122#1656418
I.e., would it be okay to produce *all* these checks for **every** `gep inbounds` we codegen, indiscriminantly,
as opposed to only always producing this `nullptr-and-nonzero-offset`?


================
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;
----------------
vsk wrote:
> 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'?
> I don't think these diagnostics generally need to repeat the fact that there is UB: that seems implicit.

Sure, ok.

> 'non-zero offset %0 applied to non-null pointer %1 produced null'?

I don't want to spell any more info that it already does, it isn't really useful //here//.
Also, we only receive the base pointer and the computed pointer.
We don't know the actual offset, we may have done `ptr - intptr_t(ptr)`,
or maybe it was `ptr + (0-intptr_t(ptr))`.
(Sure, we could receive more info, but i'm not sure worth it here.)


================
Comment at: llvm/docs/ReleaseNotes.rst:59
+  ``getelementptr inbounds`` can not change the null status of a pointer,
+  meaning it can not produce non-null pointer given null base pointer, and
+  likewise given non-null base pointer it can not produce null pointer; if it
----------------
xbolva00 wrote:
> lebedev.ri wrote:
> > xbolva00 wrote:
> > > lebedev.ri wrote:
> > > > xbolva00 wrote:
> > > > > Please add a reference to C/C++ standard that this is a UB.
> > > > It is irrelevant what semantics C/C++ standard has/doesn't have for whatever C/C++ construct,
> > > > this is talking about LLVM IR.
> > > Ok, so Clang docs?
> > > 
> > > “The compiler now optimizes ... since ... is a UB according to ... Use UBSan to catch these cases.”
> > ... see `clang/docs/ReleaseNotes.rst` then?
> > Those words were present in the first revision of the differential already,
> > https://reviews.llvm.org/D67122?id=218504#change-RNUd6wICb9iD
> Oh, sorry. 
NP :)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D67122