This is an archive of the discontinued LLVM Phabricator instance.

Differential D49777

[LTO] Don't internalize declarations
ClosedPublic

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

Details

Reviewers
pcc
tejohnson
Commits
rGeae4742d81ef: [LTO] Don't internalize declarations
rL338100: [LTO] Don't internalize declarations
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.

Diff Detail

Event Timeline

inglorion created this revision.Jul 24 2018, 10:54 PM
Herald added subscribers: dexonsmith, steven_wu, JDevlieghere and 3 others. · View Herald TranscriptJul 24 2018, 10:54 PM
tejohnson added a comment.Jul 25 2018, 6:53 AM

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

mehdi_amini added a comment.Jul 25 2018, 2:00 PM

but declarations cannot be internal.

Do you mean that the IR verifier would fail?

inglorion added a comment.Jul 25 2018, 4:25 PM
In D49777#1175774, @mehdi_amini wrote:

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.

Harbormaster completed remote builds in B20715: Diff 157392.Jul 25 2018, 4:26 PM
inglorion added a comment.Jul 25 2018, 4:34 PM

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

Harbormaster completed remote builds in B20716: Diff 157397.Jul 25 2018, 4:39 PM
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.

inglorion added a comment.Jul 26 2018, 7:03 PM

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

tejohnson added a comment.Jul 26 2018, 7:06 PM

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

Harbormaster completed remote builds in B20766: Diff 157627.Jul 26 2018, 7:08 PM
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
Closed by commit rL338100: [LTO] Don't internalize declarations (authored by inglorion). · Explain WhyJul 26 2018, 10:41 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents

PathSize
llvm/
lib/
LTO/
5 lines
test/
LTO/
X86/
31 lines

Diff 157625

llvm/lib/LTO/LTO.cpp

Show First 20 Lines • Show All 865 Lines • ▼ Show 20 Lines for (const auto &R : GlobalResolutions) {
if (!R.second.isPrevailingIRSymbol()) if (!R.second.isPrevailingIRSymbol())
continue; continue;
if (R.second.Partition != 0 && if (R.second.Partition != 0 &&
R.second.Partition != GlobalResolution::External) R.second.Partition != GlobalResolution::External)
continue; continue;
GlobalValue *GV = GlobalValue *GV =
RegularLTO.CombinedModule->getNamedValue(R.second.IRName); RegularLTO.CombinedModule->getNamedValue(R.second.IRName);
// Ignore symbols defined in other partitions. // Ignore and symbols defined in other partitions.
tejohnsonUnsubmitted
Done
ReplyInline Actions

s/and/any/?

tejohnson: s/and/any/?
if (!GV || GV->hasLocalLinkage()) // Also skip declarations, which are not allowed to have internal linkage.
if (!GV || GV->hasLocalLinkage() || GV->isDeclaration())
pccUnsubmitted
Done
ReplyInline Actions

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

pcc: I would explain in the comment why we are ignoring declarations here.
continue; continue;
GV->setUnnamedAddr(R.second.UnnamedAddr ? GlobalValue::UnnamedAddr::Global GV->setUnnamedAddr(R.second.UnnamedAddr ? GlobalValue::UnnamedAddr::Global
: GlobalValue::UnnamedAddr::None); : GlobalValue::UnnamedAddr::None);
if (R.second.Partition == 0) if (R.second.Partition == 0)
GV->setLinkage(GlobalValue::InternalLinkage); GV->setLinkage(GlobalValue::InternalLinkage);
} }
if (Conf.PostInternalizeModuleHook && if (Conf.PostInternalizeModuleHook &&
▲ Show 20 Lines • Show All 372 Lines • Show Last 20 Lines

llvm/test/LTO/X86/pr38046.ll

  • This file was added.
; RUN: opt -module-summary -o %t.o %s
; RUN: llvm-lto2 run -save-temps -o %t.lto.o %t.o \
; RUN: -r=%t.o,foo,plx \
; RUN: -r=%t.o,get,pl
; RUN: llvm-dis %t.lto.o.0.2.internalize.bc > /dev/null
target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
target triple = "x86_64-unknown-linux-gnu"
define i32 @foo() {
call void @llvm.dbg.value(metadata i32 ()* @get, metadata !7, metadata !DIExpression()), !dbg !DILocation(scope: !6)
ret i32 0
}
tejohnsonUnsubmitted
Done
ReplyInline Actions

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

tejohnson: I guess llvm-dis will fail without the patch. Still, would be good to add a check for the…
define i32 @get() {
ret i32 0
}
declare void @llvm.dbg.value(metadata, metadata, metadata)
!llvm.dbg.cu = !{!0}
pccUnsubmitted
Done
ReplyInline Actions

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.

pcc: If this just needs to be an `llvm.dbg.value` referring to `get` I think this can still be made…
!llvm.module.flags = !{!2, !3, !4, !5}
!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1)
!1 = !DIFile(filename: "t.cc", directory: "/tmp/t")
!2 = !{i32 2, !"Dwarf Version", i32 4}
!3 = !{i32 2, !"Debug Info Version", i32 3}
!4 = !{i32 1, !"wchar_size", i32 4}
!5 = !{i32 1, !"ThinLTO", i32 0}
!6 = distinct !DISubprogram(unit: !0)
!7 = !DILocalVariable(name: "get", scope: !6)