[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:
> > 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).
> * If i only pass a, then i will not be able to display the actual alignment (or it will lie when the offset is non-zero, but it won't even be able to detect that since b wasn't passed).
> * If i pass a and b, then two implementations (computing (a-b)) will need to be kept in sync. Without breaking legacy support once/if something changes, not that it is likely to change.
> * The only alternative i see is to play it dumb and not display any pointer at all (and do not display the actual alignment).
> 
> I agree that it isn't The Best Interface Ever, but from what i see,
> it's the one providing the most info without duplication.
> 
> Do you have any more concrete suggestions?
You should pass a and b. Yes, you'll need to recompute (a-b). But you need to keep these in sync anyway.

My point is: If we change something (e.g., add some additional parameter somehow), such that p != a-b, you'll need to change the interface anyway (i.e., you'll need to add a _foo_v2 ubsan function, or similar), so that you can continue to display accurate and useful information to the user.


Repository:
  rL LLVM

https://reviews.llvm.org/D54588