[PATCH] D54588: [llvm][IRBuilder] Introspection for CreateAlignmentAssumption*() functions

[Hal Finkel via Phabricator via llvm-commits]

hfinkel added inline comments.


================
Comment at: include/llvm/IR/IRBuilder.h:2132
+    /// of the offset value) is the alignment check performed?
+    Value *PtrIntValue = nullptr;
+    /// The alignment check itself (`i1`), which is then used in the assumption.
----------------
lebedev.ri wrote:
> hfinkel wrote:
> > Also, it's not clear to me why you need this. A sanitizer should get the both the original pointer and the offset to give maximum information to the user. No?
> > A sanitizer should get the both the original pointer and the offset
> 
> Exactly, *both*. In clang, i know the original pointer, the offset (may be 0), and the alignment.
> 
> But the alignment check is performed not on the original pointer, but on `f(pointer, offset)`,
> which is in this case `(((uintptr_t)pointer) - offset)`. So if i don't capture the `PtrIntValue`
> (which *is* `(((uintptr_t)pointer) - offset)`), i will need to re-calculate it elsewhere,
> and ensure that both of the calculations stay in sync.
> 
> Seems best to re-use the actual existing pre-computed values.
> 
> Please see D54590 for the actual compiler-rt tests that show the actual diag output.
Having an interface where you're required to pass a, b, and (a-b), seems a bit gratuitous. If we change what the alignment assumption means, or add new parameters, you'll need to update the ubsan interface anyway (because you'll have more things to display to the user).


Repository:
  rL LLVM

https://reviews.llvm.org/D54588