This is an archive of the discontinued LLVM Phabricator instance.

[GlobalOpt][Evaluator] Rewrite global ctor evaluation (fixes PR51879)
ClosedPublic

Authored by nikic on Dec 10 2021, 8:48 AM.

Details

Summary

Global ctor evaluation currently models memory as a map from Constant * to Constant *. For this to be correct, it is required that there is only a single Constant * referencing a given memory location. The Evaluator tries to ensure this by imposing certain limitations that could result in ambiguities (by limiting types, casts and GEP formats), but ultimately still fails, as can be seen in https://github.com/llvm/llvm-bugzilla-archive/issues/51879. The approach is fundamentally fragile and will get more so with opaque pointers.

My original thought was to instead store memory for each global as an offset => value representation. However, we also need to make sure that we can actually rematerialize the modified global initializer into a Constant in the end, which may not be possible if we allow arbitrary writes.

What this patch does instead is to represent globals as a MutableValue, which is either a Constant * or a MutableAggregate *. The mutable aggregate exists to allow efficient mutation of individual aggregate elements, as mutating an element on a Constant would require interning a new constant. When a write to the Constant * is made, it is converted into a MutableAggregate * as needed.

I believe this should make the evaluator more robust, compatible with opaque pointers, and a bit simpler as well.

Diff Detail

Event Timeline

nikic created this revision.Dec 10 2021, 8:48 AM
nikic requested review of this revision.Dec 10 2021, 8:48 AM
Herald added a project: Restricted Project. · View Herald TranscriptDec 10 2021, 8:48 AM

Elegant solution, I like it!

llvm/lib/Transforms/Utils/Evaluator.cpp
190

The lifetime of MV (or even possibly this) ends here, and I think that makes it UB to write to Val below. Maybe the dtor's body needs to be outlined & called here explicitly instead of the dtor itself?

nikic updated this revision to Diff 393557.Dec 10 2021, 11:44 AM

Don't manually call destructor.

nikic marked an inline comment as done.Dec 10 2021, 11:45 AM
nikic added inline comments.
llvm/lib/Transforms/Utils/Evaluator.cpp
190

Not sure what the exact rules here are, but it was definitely dubious... I've added a clear() method now, which is called here and in the dtor.

nikic marked an inline comment as done.Dec 17 2021, 3:10 AM

ping :)

nikic updated this revision to Diff 397020.Jan 3 2022, 1:22 AM

Rebase & Ping :)

jroelofs accepted this revision.Jan 3 2022, 8:59 AM

Looks like rebasing dropped the addition of clear(). LGTM with that and a linter-appeasing clang-format.

This revision is now accepted and ready to land.Jan 3 2022, 8:59 AM
nikic updated this revision to Diff 397080.Jan 3 2022, 9:10 AM

Restore changes lost in rebase, oops.

This revision was landed with ongoing or failed builds.Jan 4 2022, 12:33 AM
This revision was automatically updated to reflect the committed changes.
haowei added subscribers: phosek, paulkirth, haowei.EditedJan 4 2022, 11:08 AM

We are seeing assertion errors in some of Fuchsia's clang builders after this change was landed, the error message is:

clang++: llvm/lib/IR/Constants.cpp:2281: static llvm::Constant *llvm::ConstantExpr::getBitCast(llvm::Constant *, llvm::Type *, bool): Assertion `CastInst::castIsValid(Instruction::BitCast, C, DstTy) && "Invalid constantexpr bitcast!"' failed.

Looks like the assertion error happens in LLVM's own codebase. I filed github issue: https://github.com/llvm/llvm-project/issues/52994 and attached a reproducer.

Could you take a look, and if it takes a long time to fix, could you revert this change please?

dyung added a subscriber: dyung.Jan 5 2022, 3:32 AM

Hi, one of our internal tests is hitting an assertion failure after your change. I have put the details in PR53002 (https://github.com/llvm/llvm-project/issues/53002), can you take a look?