This is an archive of the discontinued LLVM Phabricator instance.

[llvm][IRBuilder] Introspection for CreateAlignmentAssumption*() functions
ClosedPublic

Authored by lebedev.ri on Nov 15 2018, 10:25 AM.

Details

Summary

Clang calls these functions to produce IR for assume-aligned attributes.
I would like to teach UBSAN to verify these assumptions.
For that, i need to access the final pointer on which the check is performed,
and the actual icmp that does the check.

The alternative to this would be to fully re-implement this in clang.

Diff Detail

Repository
rL LLVM

Event Timeline

hfinkel added inline comments.
include/llvm/IR/IRBuilder.h
2126

Please add comments here documenting the use of this struct and the meaning of the fields.

lebedev.ri marked an inline comment as done.

Document the AlignmentAssumptionIntrospection struct.

hfinkel added inline comments.Nov 16 2018, 9:14 AM
include/llvm/IR/IRBuilder.h
2127

Don't mention Clang specifics in IRBuilder's documentation. That's not relevant here.

2131

This is a question?

2132

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?

lebedev.ri added inline comments.Nov 16 2018, 9:24 AM
include/llvm/IR/IRBuilder.h
2132

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.

lebedev.ri marked 2 inline comments as done.

Reword the comments as suggested.

hfinkel added inline comments.Nov 16 2018, 10:26 AM
include/llvm/IR/IRBuilder.h
2132

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).

lebedev.ri added inline comments.Nov 16 2018, 10:35 AM
include/llvm/IR/IRBuilder.h
2132
  • 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?

hfinkel added inline comments.Nov 16 2018, 11:35 AM
include/llvm/IR/IRBuilder.h
2132

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.

Address review notes: don't store the actual pointer, just the icmp.

lebedev.ri marked 5 inline comments as done.Nov 16 2018, 2:31 PM
hfinkel accepted this revision.Nov 26 2018, 7:22 PM

LGTM

This revision is now accepted and ready to land.Nov 26 2018, 7:22 PM

LGTM

Thank you for the review!

Rebased, NFC.

Rebased before commit, NFC.