[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

12 ↗

(On Diff #157195)

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 ↗

(On Diff #157397)

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

llvm/test/LTO/X86/pr38046.ll

20 ↗

(On Diff #157397)

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 ↗

(On Diff #157625)

s/and/any/?

[inglorion]

put error message check back in

[inglorion]

fixed comment

[pcc]

LGTM

llvm/trunk/lib/LTO/LTO.cpp

for (const auto &R : GlobalResolutions) {
         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())
+      // Also skip declarations, which are not allowed to have internal linkage.
+      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/trunk/test/LTO/X86/pr38046.ll

+; 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 2>%t.dis.stderr || true
+; RUN: FileCheck -allow-empty %s < %t.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() {
+  call void @llvm.dbg.value(metadata i32 ()* @get, metadata !7, metadata !DIExpression()), !dbg !DILocation(scope: !6)
+  ret i32 0
+}
+
+define i32 @get() {
+  ret i32 0
+}
+
+declare void @llvm.dbg.value(metadata, metadata, metadata)
+
+!llvm.dbg.cu = !{!0}
+!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)