[ThinLTO] Handle empty summaries during internalization

[tejohnson]

Summary

Fix a problem I discovered when testing a fix for PR30610. The
callback MustPreserveGV in llvm::thinLTOInternalizeModule will assert if
there is no summary for a GV. We were handling this in the LTO API by
guarding the invocation of thinLTOInternalizeModule, but it was exposed
by llvm-lto which uses the legacy libLTO interface. Moved the guard into
thinLTOInternalizeModule and add a test.

[tejohnson]

Actually, the description of the test is wrong - the test I added ensures that we still get this correct for llvm-lto2 (the new LTO API). Let me add one for llvm-lto which probably needs a -thinlto-action=internalize case.

[tejohnson]

Add an llvm-lto based test as well

[mehdi_amini]

The model I have in mind with the thin-link is that we operate on the combined index which represent all the files we're processing accurately. The linker provides us with the external references (among other informations).

As I mentioned to you in person the other day, this model is broken by modules with empty summaries: some of the combined index transformations may be wrong (miscompile) because of that.

We need to rework a bit the model for these modules...

lib/Transforms/IPO/FunctionImport.cpp

554

Can you get there with an empty module? (As a module that does not define any IR Global?)

[tejohnson]

In D25345#564860, @mehdi_amini wrote:

The model I have in mind with the thin-link is that we operate on the combined index which represent all the files we're processing accurately. The linker provides us with the external references (among other informations).

As I mentioned to you in person the other day, this model is broken by modules with empty summaries: some of the combined index transformations may be wrong (miscompile) because of that.

We need to rework a bit the model for these modules...

Yes I was thinking about this some more after our conversation Wed. I think the right way to go is to do something like we do now with the HasSection flag which prevents importing any references - we can either add another bit or generalize that one - then set it for all GVs. That way they can be in the reference graph but not be imported.

But this change is needed for now to keep the current model working and to be able to commit D25359 which depends on this.

lib/Transforms/IPO/FunctionImport.cpp

554

Sure, but then we wouldn't call back into MustPreserveGV so it is fine to exit here early.

[mehdi_amini]

In D25345#565040, @tejohnson wrote:

But this change is needed for now to keep the current model working

"Keep the current model working" isn't exact: I believe it is broken since the recent addition of "false empty summaries" for module.
So have the current model working again now, you would need to revert this.

[tejohnson]

In D25345#565108, @mehdi_amini wrote:

In D25345#565040, @tejohnson wrote:

But this change is needed for now to keep the current model working

"Keep the current model working" isn't exact: I believe it is broken since the recent addition of "false empty summaries" for module.
So have the current model working again now, you would need to revert this.

I was referring to the model of preventing importing and renaming by suppressing the summary. Which has other issues as discussed, but this change at least makes the current handling uniform.

[tejohnson]

Ping. The fix in D25359 (which you already LGTM'd) depends on this change to make the handling of empty summaries consistent between llvm-lto and llvm-lto2 (i.e. ThinLTOCodeGenerator and the new LTO API).

The other alternative is to commit D25359 without any llvm-lto based tests, but that seems less than ideal.

[mehdi_amini]

Sorry, forgot about this patch. My main concern last week was that I fear we're piling-up on top of a fragile foundation: that'd be building technical debt. So I was trying to narrow down how to make sure we're not on a slippery slope with the current situation.

I was referring to the model of preventing importing and renaming by suppressing the summary.

I don't know what you referred to?

[tejohnson]

In D25345#567655, @mehdi_amini wrote:

Sorry, forgot about this patch. My main concern last week was that I fear we're piling-up on top of a fragile foundation: that'd be building technical debt. So I was trying to narrow down how to make sure we're not on a slippery slope with the current situation.

I was referring to the model of preventing importing and renaming by suppressing the summary.

I don't know what you referred to?

I'm referring to the model where we invoke llvm::moduleCanBeRenamedForThinLTO() to guard building a summary when we can't rename (e.g. due to inline assembly, or with D25359, due to module level assembly)

[mehdi_amini]

But my issue is not what moduleCanBeRenamedForThinLTO() does right now, but rather what do we do when it returns "false". These modules previously didn't go through the ThinLTO pipeline, so it was fine. Now they have an empty summary and they go to the thin-link. This seems an issue to me, and I rather have a proper answer design-wise rather than patching over-and-over as bugs are reported.

[tejohnson]

In D25345#567682, @mehdi_amini wrote:

But my issue is not what moduleCanBeRenamedForThinLTO() does right now, but rather what do we do when it returns "false". These modules previously didn't go through the ThinLTO pipeline, so it was fine. Now they have an empty summary and they go to the thin-link. This seems an issue to me, and I rather have a proper answer design-wise rather than patching over-and-over as bugs are reported.

Note that the bug that is being fixes is not due to this model. We are just using the same model to address it as with inline assembly.

Agree that a better long term solution is needed soon, but in the meantime it would be good to be able to fix the module assembly issue now, since we already use this approach for inline assembly, and also address the difference in how this case is being handled here in internalization.

lib/LTO/LTOBackend.cpp

Error lto::thinBackend(Config &Conf, unsigned Task, AddStreamFn AddStream,
 
   renameModuleForThinLTO(Mod, CombinedIndex);
 
   thinLTOResolveWeakForLinkerModule(Mod, DefinedGlobals);
 
   if (Conf.PostPromoteModuleHook && !Conf.PostPromoteModuleHook(Task, Mod))
     return Error();
 
-  if (!DefinedGlobals.empty())
-    thinLTOInternalizeModule(Mod, DefinedGlobals);
 
   if (Conf.PostInternalizeModuleHook &&
       !Conf.PostInternalizeModuleHook(Task, Mod))
     return Error();
 
   auto ModuleLoader = [&](StringRef Identifier) {
     assert(Mod.getContext().isODRUniquingDebugTypes() &&
            "ODR Type uniquing should be enabled on the context");

lib/Transforms/IPO/FunctionImport.cpp

for (auto &GV : TheModule.globals())
     updateLinkage(GV);
   for (auto &GV : TheModule.aliases())
     updateLinkage(GV);
 }
 
 /// Run internalization on \p TheModule based on symmary analysis.
 void llvm::thinLTOInternalizeModule(Module &TheModule,
                                     const GVSummaryMapTy &DefinedGlobals) {
+  // Skip this if there are no GV summaries. Either there are no
+  // defined GVs to internalize, or we had an empty summary (e.g.
+  // due to inline or module-level assembly that prevents us from
+  // being able to rename local references). If we don't bail out here
+  // then we would get an assert below in the MustPreserveGV callback.
+  if (DefinedGlobals.empty())
+    return;
+
   // Parse inline ASM and collect the list of symbols that are not defined in
   // the current module.
   StringSet<> AsmUndefinedRefs;
   object::IRObjectFile::CollectAsmUndefinedRefs(
       Triple(TheModule.getTargetTriple()), TheModule.getModuleInlineAsm(),
       [&AsmUndefinedRefs](StringRef Name, object::BasicSymbolRef::Flags Flags) {
         if (Flags & object::BasicSymbolRef::SF_Undefined)
           AsmUndefinedRefs.insert(Name);

test/Transforms/FunctionImport/Inputs/inlineasm.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
 @myvar = internal constant i8 1, align 1
 @llvm.used = appending global [1 x i8*] [i8* @myvar], section "llvm.metadata"
 
 define void @foo(i64* %v) #0 {
 entry:
   %v.addr = alloca i64*, align 8
   store i64* %v, i64** %v.addr, align 8
   %0 = load i64*, i64** %v.addr, align 8
   call void asm sideeffect "movzbl     myvar(%rip), %eax\0A\09movq %rax, $0\0A\09", "=*m,~{eax},~{dirflag},~{fpsr},~{flags}"(i64* %0) #1
   ret void
 }

test/Transforms/FunctionImport/inlineasm.ll

 ; Do setup work for all below tests: generate bitcode and combined index
 ; RUN: opt -module-summary %s -o %t.bc
 ; RUN: opt -module-summary %p/Inputs/inlineasm.ll -o %t2.bc
 ; RUN: llvm-lto -thinlto -o %t3 %t.bc %t2.bc
 
+; RUN: llvm-lto -thinlto-action=internalize -thinlto-index %t3.thinlto.bc %t2.bc -o -  --exported-symbol=main | llvm-dis -o - | FileCheck %s --check-prefix=INTERNALIZE
+; INTERNALIZE: define void @foo(i64* %v)
+
+; RUN: llvm-lto2 -o %t3 %t.bc %t2.bc -r=%t.bc,main,plx -r=%t.bc,foo,l -r=%t2.bc,foo,pl
+; RUN: llvm-nm %t3.1 | FileCheck %s --check-prefix=NM
+; NM: T foo
+
 ; Attempt the import now, ensure below that file containing inline assembly
 ; is not imported from. Otherwise we would need to promote its local variable
 ; used in the inline assembly, which would not see the rename.
 ; RUN: opt -function-import -summary-file %t3.thinlto.bc %t.bc -S 2>&1 | FileCheck %s --check-prefix=CHECK
 
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
 define i32 @main() #0 {
 entry:
   %f = alloca i64, align 8
   call void @foo(i64* %f)
   ret i32 0
 }
 
 ; CHECK: declare void @foo(i64*)
 declare void @foo(i64*) #1