Page MenuHomePhabricator

Make setInitializer() assert that the entire initializer is usable.
Needs ReviewPublic

Authored by arnt on Apr 12 2019, 6:42 AM.

Details

Summary

Until now, it was possible to set an initilizer for e.g. a struct that
contained a global constant from some other module. The verifier would
complain, but the verifier's complaint doesn't point very accurately at
the problem.

Diff Detail

Event Timeline

arnt created this revision.Apr 12 2019, 6:42 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 12 2019, 6:42 AM
arnt added a comment.Apr 12 2019, 6:43 AM

I picked reviewers by taking the last two people who changed the file, hope you don't mind.

You probably want to wrap that entire function into #ifndef NDEBUG

llvm/lib/IR/Globals.cpp
386–387

The style is usually

for(int I = 0, N = V->getNumOperands(); I != N; ++I)
  assertThatAllGlobalsAreIn(V->getOperand(--n), Correct);
386–387

well, s/--n/n/

arnt updated this revision to Diff 194876.Apr 12 2019, 7:17 AM

updated following comments from lebedev.ri; thanks

arnt marked 2 inline comments as done.May 14 2019, 5:15 AM

Ping?

arnt updated this revision to Diff 206821.Jun 27 2019, 4:12 AM

Rebased to this morning's LLVM.

No changes were necessary.

arnt added a reviewer: pete.Jun 27 2019, 4:19 AM

pete: I took the liberty of adding you as reviewer since you made the most recent change to that function. 2015-06-02. Not exactly yesterday.

efriedma added inline comments.Jun 27 2019, 6:58 PM
llvm/lib/IR/Globals.cpp
386

Please clang-format this.

Can you check for ConstantAggregate here, instead of checking ConstantStruct and ConstantArray separately? Would it also make sense to check for ConstantExpr?

Can you use a for-range loop here? (for (auto *Op : V->operands()))

arnt updated this revision to Diff 207257.Jul 1 2019, 3:24 AM

Add unit test, plus some nonfunctional changes.

arnt marked 2 inline comments as done.Jul 1 2019, 3:29 AM

I like colon-for.

llvm/lib/IR/Globals.cpp
386

Oooh, I can call constants() now. Someone has added that. (Might be a while ago, this isn't exactly new code. ;-)

I changed it to test ConstantAggregate, and added a passing unit test involving ConstantExpr.

efriedma added inline comments.Jul 1 2019, 11:28 AM
llvm/unittests/IR/VerifierTest.cpp
228

Are you sure this actually does what you want? ConstantExpr::get() does basic constant folding.

arnt updated this revision to Diff 211433.Jul 24 2019, 12:23 AM
arnt marked an inline comment as done.

best take care of self-referential initialisers.

arnt updated this revision to Diff 211434.Jul 24 2019, 12:48 AM
arnt marked 2 inline comments as done.

More conservative testing, as half-suggested by efriedma.

I don't think you actually need a recursion guard; you only recurse on ConstantAggregates, and they can't refer to themselves circularly. But maybe it's a good idea anyway to avoid exponential compile-time in certain edge cases.

llvm/unittests/IR/VerifierTest.cpp
237

This test *still* isn't testing what you want; verification fails due to the other global variable.