[LTO] Don't internalize declarations

[inglorion]

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.

[tejohnson]

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]

but declarations cannot be internal.

Do you mean that the IR verifier would fail?

[inglorion]

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]

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

[inglorion]

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]

check for error message in test

[pcc]

llvm/lib/LTO/LTO.cpp

874

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

llvm/test/LTO/X86/pr38046.ll

20

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]

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]

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

[tejohnson]

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

llvm/lib/LTO/LTO.cpp

874–875

s/and/any/?

[inglorion]

put error message check back in

[inglorion]

fixed comment

[pcc]

LGTM

llvm/lib/LTO/LTO.cpp

for (const auto &R : GlobalResolutions) {
       if (!R.second.isPrevailingIRSymbol())
         continue;
       if (R.second.Partition != 0 &&
           R.second.Partition != GlobalResolution::External)
         continue;
 
       GlobalValue *GV =
           RegularLTO.CombinedModule->getNamedValue(R.second.IRName);
-      // Ignore symbols defined in other partitions.
-      if (!GV || GV->hasLocalLinkage())
+      // Ignore declarations and symbols defined in other partitions.
+      if (!GV || GV->hasLocalLinkage() || GV->isDeclaration())
         continue;
       GV->setUnnamedAddr(R.second.UnnamedAddr ? GlobalValue::UnnamedAddr::Global
                                               : GlobalValue::UnnamedAddr::None);
       if (R.second.Partition == 0)
         GV->setLinkage(GlobalValue::InternalLinkage);
     }
 
     if (Conf.PostInternalizeModuleHook &&

llvm/test/LTO/X86/Inputs/pr38046.m.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+declare i32 @foo()
+
+define i32 @main(i32 %argc, i8** %argv) {
+  %call = call i32 @foo()
+  ret i32 %call
+}

llvm/test/LTO/X86/pr38046.ll

+; RUN: rm -fr %t
+; RUN: mkdir -p %t
+; RUN: opt -module-summary -o %t/t.o %s
+; RUN: opt -o %t/m.o %S/Inputs/pr38046.m.ll
+; RUN: cd %t
+; RUN: llvm-lto2 run -save-temps -o m m.o t.o \
+; RUN:   -r=m.o,main,plx \
+; RUN:   -r=m.o,foo,l \
+; RUN:   -r=t.o,foo,pl \
+; RUN:   -r=t.o,get,pl
+; RUN: llvm-dis m.0.2.internalize.bc > /dev/null 2>llvm-dis.stderr || true
+; RUN: FileCheck -allow-empty %s < llvm-dis.stderr
+
+; CHECK-NOT: Global is external, but doesn't have external or weak linkage
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+define i32 @foo() !dbg !17 {
+  call void @llvm.dbg.value(metadata i32 ()* @get, metadata !22, metadata !DIExpression()), !dbg !24
+  ret i32 0, !dbg !25
+}
+
+define i32 @get() !dbg !26 {
+  ret i32 0, !dbg !27
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!llvm.module.flags = !{!12, !13, !14, !15}
+!llvm.ident = !{!16}
+
+!0 = distinct !DICompileUnit(language: DW_LANG_C_plus_plus, file: !1, producer: "clang version 7.0.0 ", isOptimized: true, runtimeVersion: 0, emissionKind: FullDebug, enums: !{}, retainedTypes: !3, globals: !{})
+!1 = !DIFile(filename: "t.cc", directory: "/tmp/t")
+!2 = !{}
+!3 = !{!4}
+!4 = !DIBasicType(name: "int", size: 32, encoding: DW_ATE_signed)
+!5 = !DIFile(filename: "./t.h", directory: "/tmp/t")
+!6 = !DISubprogram(name: "get", linkageName: "get", scope: !0, file: !5, line: 3, type: !7, isLocal: false, isDefinition: false, scopeLine: 3, flags: DIFlagPublic | DIFlagPrototyped | DIFlagStaticMember, isOptimized: true)
+!7 = !DISubroutineType(types: !8)
+!8 = !{!4}
+!9 = !DISubprogram(name: "foo", linkageName: "foo", scope: !0, file: !5, line: 4, type: !10, isLocal: false, isDefinition: false, scopeLine: 4, flags: DIFlagPublic | DIFlagPrototyped, isOptimized: true)
+!10 = !DISubroutineType(types: !11)
+!11 = !{!4}
+!12 = !{i32 2, !"Dwarf Version", i32 4}
+!13 = !{i32 2, !"Debug Info Version", i32 3}
+!14 = !{i32 1, !"wchar_size", i32 4}
+!15 = !{i32 1, !"ThinLTO", i32 0}
+!16 = !{!"clang version 7.0.0 "}
+!17 = distinct !DISubprogram(name: "foo", linkageName: "foo", scope: !0, file: !1, line: 5, type: !10, isLocal: false, isDefinition: true, scopeLine: 5, flags: DIFlagPrototyped, isOptimized: true, unit: !0, declaration: !9, retainedNodes: !{})
+!18 = distinct !DISubprogram(name: "do", linkageName: "do", scope: !5, file: !5, line: 10, type: !19, isLocal: false, isDefinition: true, scopeLine: 10, flags: DIFlagPrototyped, isOptimized: true, unit: !0, templateParams: !{}, retainedNodes: !21)
+!19 = !DISubroutineType(types: !20)
+!20 = !{!4}
+!21 = !{!22}
+!22 = !DILocalVariable(name: "get", scope: !18, file: !5, line: 11, type: !4)
+!23 = distinct !DILocation(line: 6, column: 21, scope: !17)
+!24 = !DILocation(line: 11, column: 14, scope: !18, inlinedAt: !23)
+!25 = !DILocation(line: 12, column: 1, scope: !17)
+!26 = distinct !DISubprogram(name: "get", linkageName: "get", scope: !0, file: !1, line: 14, type: !7, isLocal: false, isDefinition: true, scopeLine: 14, flags: DIFlagPrototyped, isOptimized: true, unit: !0, declaration: !6, retainedNodes: !2)
+!27 = !DILocation(line: 15, column: 3, scope: !26)