[PATCH] D116861: [UBSan] Fix incorrect alignment reported when global new returns an offset pointer

[Reid Kleckner via Phabricator via cfe-commits]

rnk added a subscriber: rjmccall.
rnk added a comment.

I think the main thing here is that the diagnostic is incorrect: It claims that the type requires `__STDC_DEFAULT_NEW_ALIGNMENT__` (16) byte alignment, but it doesn't, it only requires 8. That's confusing.

The next question is, does LLVM exploit `__STDCPP_DEFAULT_NEW_ALIGNMENT__` for optimization purposes? IMO, UBSan should agree with LLVM: it is supposed to aid in the early detection of UB, rather than having to figure that out later after optimizations have run. If LLVM doesn't exploit it, I don't see a need to report an error. After some experimentation, I have convinced myself that LLVM doesn't know or exploit allocator alignment:
https://godbolt.org/z/oo6rzKn3c

Finally, we should ask ourselves, is it worth adding a special diagnostic to check that all operator new calls return aligned pointers? I think the answer is no. Users are more likely to heed sanitizer errors when they point to actual, practical issues rather than hypothetical ones about more highly aligned types allocated at other call sites.

Let me ask @rjmccall for a second opinion, since he wrote the `Address` alignment-tracking overhaul code.



================
Comment at: compiler-rt/test/ubsan/TestCases/TypeCheck/global-new-alignment.cpp:33
+int main() {
+  // CHECK-NOT: runtime error: constructor call on misaligned address [[PTR:0x[0-9a-f]*]] for type 'Param', which requires 16 byte alignment
+  Param *p = new Param;
----------------
I suggest simplifying this to `CHECK-NOT: runtime error`, since no errors are expected. This is mostly to improve test debuggability anyway, the test will fail if it exits non-zero.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D116861