ThinLTOBitcodeWriter: handle aliases first in filterModule

[inglorion]

Summary

This change fixes a "local linkage requires default visibility" assert when attempting to build LLVM with ThinLTO on Windows.

[mehdi_amini]

Do you have a test case?

[inglorion]

The test for this change is in D31633.

[mehdi_amini]

I see the clang test. We try to keep our testing at the IR level though. Is there anything specific that prevents it in this case?

[inglorion]

@mehdi_amini: This is reduced from a C++ program that triggered the problem. The compiler failed before outputing IR, which is why it is C++. I can try to see if I can create some IR that will reproduce the problem, but it will have to wait until tomorrow.

[inglorion]

Thanks, @pcc for helping me look into this and realizing that doing the alias processing first fixes the problem. I've also included a small change to use GA-> instead of I->, which prevents us from using the iterator in an invalid state and hitting another assert.

[inglorion]

Added LLVM IR test.

[pcc]

test/ThinLTO/X86/promote-internals.ll

1 ↗

(On Diff #94118)

This is a test of the ThinLTOBitcodeWriter so it should live in test/Transforms/ThinLTOBitcodeWriter.

2 ↗

(On Diff #94118)

Please use llvm-modextract and llvm-dis to check that the IR is as expected.

13–21 ↗

(On Diff #94118)

Do you need these functions? I think you should be able to trigger the bug without them if you give @al external linkage.

[inglorion]

addressed @pcc's comments

[inglorion]

test/ThinLTO/X86/promote-internals.ll

13–21 ↗

(On Diff #94118)

Doing that leads to a different error ("GlobalValue not found in index"). That error is also addressed by this patch (specifically, by the change from I-> to GA->), but can be triggered with or without the change to when we process aliases. For that reason, I've elected to keep the functions in the test - but I can remove them if you really want me to.

[pcc]

test/ThinLTO/X86/promote-internals.ll

13–21 ↗

(On Diff #94118)

If I make just the I-> to GA-> change locally I still get the "GlobalValue not found in index" assertion failure with your test case with the functions removed. I would just remove them because the assertion failure is being caused by the same underlying issue.

[pcc]

test/Transforms/ThinLTOBitcodeWriter/promote-internals.ll

3

Please also check the contents of both modules here.

[inglorion]

Made changes requested by @pcc

[pcc]

LGTM

test/Transforms/ThinLTOBitcodeWriter/promote-internals.ll

1

Nit: rename filter-alias.ll

lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

if (GV.isDeclaration() && GV.use_empty()) {
       GV.eraseFromParent();
       continue;
     }
   }
 }
 
 void filterModule(
     Module *M, function_ref<bool(const GlobalValue *)> ShouldKeepDefinition) {
-  for (Function &F : *M) {
-    if (ShouldKeepDefinition(&F))
-      continue;
-
-    F.deleteBody();
-    F.setComdat(nullptr);
-    F.clearMetadata();
-  }
-
-  for (GlobalVariable &GV : M->globals()) {
-    if (ShouldKeepDefinition(&GV))
-      continue;
-
-    GV.setInitializer(nullptr);
-    GV.setLinkage(GlobalValue::ExternalLinkage);
-    GV.setComdat(nullptr);
-    GV.clearMetadata();
-  }
-
   for (Module::alias_iterator I = M->alias_begin(), E = M->alias_end();
        I != E;) {
     GlobalAlias *GA = &*I++;
     if (ShouldKeepDefinition(GA))
       continue;
 
     GlobalObject *GO;
-    if (I->getValueType()->isFunctionTy())
+    if (GA->getValueType()->isFunctionTy())
       GO = Function::Create(cast<FunctionType>(GA->getValueType()),
                             GlobalValue::ExternalLinkage, "", M);
     else
       GO = new GlobalVariable(
           *M, GA->getValueType(), false, GlobalValue::ExternalLinkage,
           (Constant *)nullptr, "", (GlobalVariable *)nullptr,
           GA->getThreadLocalMode(), GA->getType()->getAddressSpace());
     GO->takeName(GA);
     GA->replaceAllUsesWith(GO);
     GA->eraseFromParent();
   }
+
+  for (Function &F : *M) {
+    if (ShouldKeepDefinition(&F))
+      continue;
+
+    F.deleteBody();
+    F.setComdat(nullptr);
+    F.clearMetadata();
+  }
+
+  for (GlobalVariable &GV : M->globals()) {
+    if (ShouldKeepDefinition(&GV))
+      continue;
+
+    GV.setInitializer(nullptr);
+    GV.setLinkage(GlobalValue::ExternalLinkage);
+    GV.setComdat(nullptr);
+    GV.clearMetadata();
+  }
 }
 
 void forEachVirtualFunction(Constant *C, function_ref<void(Function *)> Fn) {
   if (auto *F = dyn_cast<Function>(C))
     return Fn(F);
   if (isa<GlobalValue>(C))
     return;
   for (Value *Op : C->operands())

test/Transforms/ThinLTOBitcodeWriter/promote-internals.ll

+; RUN: opt -thinlto-bc -o %t %s
+; RUN: llvm-modextract -n 0 -o - %t | llvm-dis | FileCheck --check-prefix=CHECK0 %s
+; RUN: llvm-modextract -n 1 -o - %t | llvm-dis | FileCheck --check-prefix=CHECK1 %s
+; CHECK0: @al = external global i8*
+; CHECK1: @al = unnamed_addr alias i8*,
+
+target datalayout = "e-m:w-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-pc-windows-msvc19.0.24215"
+
+$al = comdat any
+
+@anon = private unnamed_addr constant { [1 x i8*] } { [1 x i8*] [i8* null] }, comdat($al), !type !0
+
+@al = external unnamed_addr alias i8*, getelementptr inbounds ({ [1 x i8*] }, { [1 x i8*] }* @anon, i32 0, i32 0, i32 1)
+
+!0 = !{i64 8, !"?AVA@@"}