Page MenuHomePhabricator

[ThinLTO] Fix lazy-loading of MDString instruction attachments
ClosedPublic

Authored by mehdi_amini on Jan 19 2017, 1:46 PM.

Details

Summary

CFI is using intrinsics that takes MDString as arguments, and this
was broken during lazy-loading of metadata.

Diff Detail

Repository
rL LLVM

Event Timeline

mehdi_amini created this revision.Jan 19 2017, 1:46 PM

Nominate for LLVM 4.0 at the same time.

pcc edited edge metadata.Jan 19 2017, 4:16 PM

Unfortunately this doesn't seem to entirely fix the CFI issue. With this patch applied I get these assertion failures for the attached test case (llvm-lto2 @resolution.txt -o /tmp/foo):

invalid local scope
!18644 = distinct !DILexicalBlock(scope: null, file: !5600, line: 137, column: 2)
invalid local scope
!18685 = distinct !DILexicalBlock(scope: null, file: !5600, line: 164, column: 2)
llvm-lto2: [...]/include/llvm/Support/Casting.h:95: static bool llvm::isa_impl_cl<llvm::DILocalScope, const llvm::Metadata *>::doit(const From *) [To = llvm::DILocalScope, From = const llvm::Metadata *]: Assertion `Val && "isa<> used on a null pointer"' failed.
Aborted (core dumped)

If I pass -disable-ondemand-mds-loading or if I revert this patch I get the "expected" behaviour ("expected" because of incomplete support for CFI on trunk):

Can't get register for value!
UNREACHABLE executed at [...]/lib/CodeGen/SelectionDAG/SelectionDAGBuilder.cpp:1202!
pcc added a comment.Jan 19 2017, 5:38 PM


Here's a reproducer that does not use CFI.

pcc added a comment.Jan 19 2017, 9:32 PM

Reduced reproducer.

To reproduce: PATH=/path/to/llvm/build/bin ./run-lto2.sh

Fix the test case reported by pcc: obviously adding a PlaceholderQueue requires to flush it at some point...

I'll add an assert in the PlaceholderQueue destructor in a separate commit.

Assertions added in r292597.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
502 ↗(On Diff #85110)

This was missing ^^

pcc accepted this revision.Jan 20 2017, 12:03 PM

Thanks, I was able to build chrome successfully with CFI + debug info with this patch applied. LGTM (with nits) and approved for 4.0.

llvm/lib/Bitcode/Reader/MetadataLoader.cpp
488 ↗(On Diff #85110)

Is this function used any more?

1746 ↗(On Diff #85110)

Should this function be renamed to match?

This revision is now accepted and ready to land.Jan 20 2017, 12:03 PM

Address pcc feedback, update patch before commiting

mehdi_amini marked 3 inline comments as done.Jan 20 2017, 12:24 PM
This revision was automatically updated to reflect the committed changes.