rsmith added a comment. Looks fine to me with some doc improvements.
================ Comment at: clang/docs/ReleaseNotes.rst:64-66 + non-zero offset to ``nullptr`` (or making non-``nullptr`` a ``nullptr``, + by subtracting pointer's integral value from the pointer itself; in C, also, + applying *any* (even zero) offset to ``nullptr``) is undefined behaviour. ---------------- The parenthetical here makes this awkward to read. Also, I don't think we need to say which LLVM revision this happened in. How about: "In both C and C++ (C17 6.5.6p8, C++ [expr.add]), pointer arithmetic is only permitted within arrays. In particular, the behavior of a program is not defined if it adds a non-zero offset (or in C, any offset) to a null pointer, or that forms a null pointer by subtracting an integer from a non-null pointer, and the LLVM optimizer now uses those guarantees for transformations. This may lead to unintended behavior in code that performs these operations. The Undefined Behavior Sanitizer `-fsanitize=pointer-overflow` check has been extended to detect these cases, so that code relying on them can be detected and fixed." ================ Comment at: clang/docs/ReleaseNotes.rst:242 -- ... +- * ``pointer-overflow`` check was extended added to catch the cases where + a non-zero offset being applied, either to a ``nullptr``, or the result ---------------- Add "The" to the start of this bullet. ================ Comment at: clang/docs/ReleaseNotes.rst:243-244 +- * ``pointer-overflow`` check was extended added to catch the cases where + a non-zero offset being applied, either to a ``nullptr``, or the result + of applying of the offset is a ``nullptr``. + As per C++ Standard ``[expr.add]`` that is undefined behaviour. ---------------- "[...] where a non-zero offset is applied to a null pointer, or the result of applying the offset is a null pointer." ================ Comment at: clang/docs/ReleaseNotes.rst:245 + of applying of the offset is a ``nullptr``. + As per C++ Standard ``[expr.add]`` that is undefined behaviour. + ---------------- I don't think we really need to say this is undefined behavior here. ================ Comment at: clang/docs/UndefinedBehaviorSanitizer.rst:133 - ``-fsanitize=pointer-overflow``: Performing pointer arithmetic which - overflows. + overflows; applying a non-zero (in C++; in C - any) offset to either a + non-``nullptr``, or pointer becoming ``nullptr`` after applying the offset. ---------------- Simplify this a bit: "Performing pointer arithmetic which overflows, or where either the old or new pointer value is a null pointer (or in C, when they both are)." ================ Comment at: clang/lib/CodeGen/CGExprScalar.cpp:4657 + Builder.GetInsertBlock()->getParent(), PtrTy->getPointerAddressSpace()); + // Check for overflows unless the GEP got constant-folded, + // and only in the default address space ---------------- If we want to split out the "constant folded" case to avoid issuing too many sanitizer traps on bogus but common patterns, we should have another sanitizer group to re-enable those diagnostics for the constant-folded cases. (I'm fine with not doing that in this patch, though.) Repository: rG LLVM Github Monorepo CHANGES SINCE LAST ACTION https://reviews.llvm.org/D67122/new/ https://reviews.llvm.org/D67122 _______________________________________________ cfe-commits mailing list cfe-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
