This is an archive of the discontinued LLVM Phabricator instance.

[GlobalOpt] Fix a miscompile when evaluating struct initializers.
ClosedPublic

Authored by jroelofs on Jul 12 2021, 12:55 PM.

Details

Summary

The bug was that evaluateBitcastFromPtr attempts a narrowing to a struct's 0th element of a store that covers other elements. While this is okay on the load side, applying it to stores causes us to miss the writes to the additionally covered elements.

rdar://79503568

Diff Detail

Event Timeline

jroelofs created this revision.Jul 12 2021, 12:55 PM
jroelofs requested review of this revision.Jul 12 2021, 12:55 PM
Herald added a project: Restricted Project. · View Herald TranscriptJul 12 2021, 12:55 PM
Gerolf added a subscriber: Gerolf.Jul 14 2021, 10:43 AM

This is straightforward. LGTM.

efriedma added inline comments.
llvm/lib/Transforms/Utils/Evaluator.cpp
374

"proposed bitcast load" doesn't make sense. Do you mean to say "The conversion is illegal if the stored type is narrower than the type of the memory location"?

The code would probably be easier to understand if MutatedMemory was represented using a tuple of "(GlobalVariable, Offset, Size)", or something like that, instead of trying to do a delicate dance with ConstantExprs. But you don't need to do that rewrite here.

379

This is a weird way to write Val->getType().

aeubanks added inline comments.Jul 14 2021, 11:12 AM
llvm/lib/Transforms/Utils/Evaluator.cpp
180

ConstantLoad?

372–382

Can you make this uppercase?

llvm/test/Transforms/GlobalOpt/store-struct-element.ll
3

is this necessary?

jroelofs added inline comments.Jul 14 2021, 11:41 AM
llvm/lib/Transforms/Utils/Evaluator.cpp
180

I thought renaming would help clarify what the callback is expected to do. Here in some sense it behaves kind of like a constexpr load would, returning a constant if it can evaluate a load from a constant pointer, and nullptr if it cannot.

374

For context, I meant "proposed bitcast load" in the sense that evaluateBitcastFromPtr making queries to its callback first for the whole struct, then for that struct's first element, etc. is a "proposal" with nullptr return being a rejection, and the evaluation of the callback itself being a sort of constexpr-through-a-bitcast. Agreed that the wording is strange; I'll reword.

The code would probably be easier to understand...

I was thinking the same, but wanted to keep the fix simple.

jroelofs updated this revision to Diff 358697.Jul 14 2021, 12:26 PM

review feedback

jroelofs marked 4 inline comments as done.Jul 14 2021, 12:29 PM
jroelofs added inline comments.
llvm/lib/Transforms/Utils/Evaluator.cpp
180

Just noticed you meant the comment needed updating after renaming it TryLoad.

jroelofs updated this revision to Diff 358698.Jul 14 2021, 12:30 PM

clang-format

This revision is now accepted and ready to land.Jul 14 2021, 2:29 PM
This revision was landed with ongoing or failed builds.Jul 14 2021, 3:45 PM
This revision was automatically updated to reflect the committed changes.
jroelofs added inline comments.Jul 14 2021, 6:49 PM
llvm/lib/Transforms/Utils/Evaluator.cpp
374

The code would probably be easier to understand if MutatedMemory was represented using a tuple of "(GlobalVariable, Offset, Size)", or something like that, instead of trying to do a delicate dance with ConstantExprs. But you don't need to do that rewrite here.

I'm beginning to think a representational change is *necessary* in order to fix all the issues here. Consider this, for example:

https://llvm.godbolt.org/z/4jhn9PGdW