This is an archive of the discontinued LLVM Phabricator instance.

[LTO] Don't internalize declarations
ClosedPublic

Authored by inglorion on Jul 24 2018, 10:54 PM.

Details

Summary

Some links were failing with "Global is external, but doesn't have
external or weak linkage!" in ThinLTO builds with debug
information. This happened when we elide the body of a global that is
referenced by debug info. This results in a declaration, which we
would then internalize - but declarations cannot be internal. This
change avoids the problem by not internalizing these declarations.

Fixes PR38046.

Event Timeline

inglorion created this revision.Jul 24 2018, 10:54 PM

Code change looks fine, but please reduce the test case. - there's a lot of stuff there that likely isn't needed to reproduce.

llvm/test/LTO/X86/pr38046.ll
13

I guess llvm-dis will fail without the patch. Still, would be good to add a check for the declaration that was being internalized

but declarations cannot be internal.

Do you mean that the IR verifier would fail?

but declarations cannot be internal.

Do you mean that the IR verifier would fail?

Yes. And this causes the Chrome ThinLTO build to fail on Windows.

inglorion updated this revision to Diff 157392.Jul 25 2018, 4:26 PM

Thanks, @tejohnson! I made the test case smaller.

Ah, just saw your comment about adding a check. Yes, I was checking that llvm-dis succeeds; I'll check for the error message instead to make it clearer what we're testing.

inglorion updated this revision to Diff 157397.Jul 25 2018, 4:39 PM

check for error message in test

inglorion marked an inline comment as done.Jul 25 2018, 4:40 PM
pcc added inline comments.Jul 26 2018, 1:16 PM
llvm/lib/LTO/LTO.cpp
874–876

I would explain in the comment why we are ignoring declarations here.

llvm/test/LTO/X86/pr38046.ll
21

If this just needs to be an llvm.dbg.value referring to get I think this can still be made simpler. See llvm/test/CodeGen/Generic/dbg_value.ll which appears to be the smallest test case in the tree that uses llvm.dbg.value.

inglorion updated this revision to Diff 157625.Jul 26 2018, 7:02 PM
inglorion marked 2 inline comments as done.

Per @pcc's suggestions:

Updated the comment.

Simplified the testcase. There is still a bit more metadata in there
than in dbg_value.ll, notably !llvm.module.flags and its
descendants. Without that, the broken module error does not reproduce.

Hmm, seems like I lost checking the error message along the way.

LGTM with one minor comment nit below. See if pcc has any more comments though.

llvm/lib/LTO/LTO.cpp
874

s/and/any/?

inglorion updated this revision to Diff 157627.Jul 26 2018, 7:08 PM

put error message check back in

inglorion updated this revision to Diff 157629.Jul 26 2018, 7:09 PM

fixed comment

inglorion marked an inline comment as done.Jul 26 2018, 7:09 PM
pcc accepted this revision.Jul 26 2018, 7:53 PM

LGTM

This revision is now accepted and ready to land.Jul 26 2018, 7:53 PM
This revision was automatically updated to reflect the committed changes.