[PATCH] D54589: [clang][UBSan] Sanitization for alignment assumptions.

[Roman Lebedev via Phabricator via cfe-commits]

lebedev.ri added inline comments.


================
Comment at: docs/UndefinedBehaviorSanitizer.rst:198
+assume-aligned-like attributes), `object-size``, and ``vptr`` checks do not
+apply to pointers to types with the ``volatile`` qualifier
 
----------------
rjmccall wrote:
> Is there a reason for this exception?
Are you asking about the LHS of the diff, or about adding an exception to that for this sanitizer?
I'm adding an exception here because i don't know what should be done here.
Does it make sense to emit an assumptions for volatile pointers, but do not sanitize these assumptions?


================
Comment at: lib/CodeGen/CGBuiltin.cpp:1895
 
-    EmitAlignmentAssumption(PtrValue, Alignment, OffsetValue);
+    EmitAlignmentAssumption(PtrValue, Ptr, {/*The expr loc is sufficient.*/},
+                            Alignment, OffsetValue);
----------------
rjmccall wrote:
> Is this `{}`-initializing a `SourceLocation`?  Please use `SourceLocation()` instead and put the comment before it.
> Is this `{}`-initializing a `SourceLocation`?
Yes

Ok.


================
Comment at: lib/CodeGen/CodeGenFunction.cpp:2467
+    llvm::Value *OffsetValue, llvm::Value *TheCheck,
+    llvm::Instruction *Assumption) {
+  assert(Assumption && isa<llvm::CallInst>(Assumption) &&
----------------
rjmccall wrote:
> What's the deal with the two different source locations?
The first one points to the source-location of this alignment assumption.
The second one *may* point to the location where the alignment was specified.
See e.g. "test/ubsan/TestCases/Pointer/alignment-assumption-attribute-align_value-on-lvalue.cpp" in https://reviews.llvm.org/D54590#change-jI44M13yrBNo


Repository:
  rC Clang

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

https://reviews.llvm.org/D54589