Page MenuHomePhabricator

[BasicAA] Fix order in which we pass MemoryLocations to alias()
ClosedPublic

Authored by aeubanks on May 10 2022, 10:23 AM.

Details

Summary

D98718 caused the order of Values/MemoryLocations we pass to alias() to
be significant due to storing the offset in the PartialAlias case. But
some callers weren't audited and were still passing swapped arguments,
causing the returned PartialAlias offset to be negative in some
cases. For example, the newly added unittests would return -1
instead of 1.

Fixes #55343, a miscompile.

Diff Detail

Event Timeline

aeubanks created this revision.May 10 2022, 10:23 AM
aeubanks requested review of this revision.May 10 2022, 10:23 AM
Herald added a project: Restricted Project. · View Herald TranscriptMay 10 2022, 10:23 AM

looks like this isn't the only time this sort of thing has come up: D115927

nikic accepted this revision.May 10 2022, 11:24 AM

LGTM

This revision is now accepted and ready to land.May 10 2022, 11:24 AM
asbirlea accepted this revision.May 10 2022, 11:36 AM
This revision was landed with ongoing or failed builds.May 10 2022, 12:09 PM
This revision was automatically updated to reflect the committed changes.
arichardson added inline comments.
llvm/include/llvm/IR/IRBuilder.h
544

Would it be possible to drop the default zero argument for new APIs? Defaulting to AS0 instead of explicitly passing address spaces often causes assertion errors for targets such as AVR or the downstream CHERI-RISC-V and Arm Morello targets.

544

o

aeubanks added inline comments.May 13 2022, 10:46 AM
llvm/include/llvm/IR/IRBuilder.h
544

As somebody unfamiliar with those targets, what would be the right thing to pass in various cases?
And if you're interested in dropping the default argument, I think it would be better to do that all at once for all APIs.

Is it possible to add in-tree testing for the sort of thing you're interested in without upstreaming an entire target?