[ThinLTO] Remove functions resolved to available_externally from comdats

[tejohnson]

Summary

thinLTOResolveWeakForLinkerModule needs to drop any preempted weak symbols
that were converted to available_externally from comdats, otherwise we
will get a verification failure (since available_externally is a
declaration for the linker, and no declarations can be in a comdat).

[mehdi_amini]

Do we need gold for the test here or could it be written with an LLVM tool?

[twoh]

BTW, it seems that the original assertion failure (PR28760) doesn't happen even with r274523. Originally IRLinker::linkGlobalValueProto created a new GlobalValue with ".%d" appended to its name, but for the given example code the function returns nullptr because DoneLinkBodies is already true. Could you please double check?

[tejohnson]

In D23015#502330, @mehdi_amini wrote:

Do we need gold for the test here or could it be written with an LLVM tool?

Let me see if I can trigger without gold's symbol resolution

In D23015#502688, @twoh wrote:

BTW, it seems that the original assertion failure (PR28760) doesn't happen even with r274523. Originally IRLinker::linkGlobalValueProto created a new GlobalValue with ".%d" appended to its name, but for the given example code the function returns nullptr because DoneLinkBodies is already true. Could you please double check?

I haven't tried at r274523, only at head as of yesterday. The test case included here definitely creates a GV with .%d appended (it wouldn't pass if it didn't), and it fails with the assertion if I restore that. Are you saying that the test case included here is not working when the patch is applied to r274523, or that it doesn't provoke the problem at that older revision? Or you cannot reproduce your original failure at r274523? If your original failure is reproducible at a recent revision, is it fixed by the current patch?

[twoh]

@tejohnson The test code surely generates GV with ".%d" appended, but it doesn't trigger the assertion failure even without this patch, both for r274523 and r277147. Let me try again with more recent version.

My original failure cannot be reproduced by the recent version(r277147) because other issue hides the original failure. However, once I apply this patch to r274523 then it works (linking fails with another error but it definitely makes more progress). I'm just concerning the test code may target wrong.

[tejohnson]

In D23015#502735, @twoh wrote:

@tejohnson The test code surely generates GV with ".%d" appended, but it doesn't trigger the assertion failure even without this patch, both for r274523 and r277147. Let me try again with more recent version.

Hmm, it does for me and I am at r277267. If I put the assert back it fails with that assert.

My original failure cannot be reproduced by the recent version(r277147) because other issue hides the original failure.

Is this the -g failure? Can you try without -g?

However, once I apply this patch to r274523 then it works (linking fails with another error but it definitely makes more progress). I'm just concerning the test code may target wrong.

[twoh]

@tejohnson Sorry, it was my bad. I wrote the makefile by myself and swapped the position of t.o and t2.o. Once I position them right the assertion fails.

If I try it without -g, it fails with the same error with r274523.

[tejohnson]

In D23015#502773, @twoh wrote:

@tejohnson Sorry, it was my bad. I wrote the makefile by myself and swapped the position of t.o and t2.o. Once I position them right the assertion fails.

If I try it without -g, it fails with the same error with r274523.

Ok, great. Let me know if the link succeeds with this patch and with -g removed.

[tejohnson]

In D23015#502700, @tejohnson wrote:

In D23015#502330, @mehdi_amini wrote:

Do we need gold for the test here or could it be written with an LLVM tool?

Let me see if I can trigger without gold's symbol resolution

Can't trigger with llvm-lto. The gold-plugin uses the IRLinker to move the module being compiled (in that ThinLTO backend) into an empty module, indicating the ValuesToLink based on its symbol resolution decisions. Which is why we get f1 linked in only as a declaration for %t2.o, which means we need to create a local copy of it for the use via the alias, resulting in the naming conflict with the declaration. In llvm-lto, we simply use the module being compiled with a thinlto-action as is.

[mehdi_amini]

OK, let me backtrack, is the original issue coming from the fact that gold is starting with an empty module?
Then why not changing this?

[tejohnson]

In D23015#502970, @mehdi_amini wrote:

OK, let me backtrack, is the original issue coming from the fact that gold is starting with an empty module?
Then why not changing this?

We use gold's symbol resolution to determine which values to link in. So in the example, it knows that f1 is a preempted symbol in %t2.o and doesn't need to include its definition. I don't think we should need to change that.

[mehdi_amini]

In D23015#503013, @tejohnson wrote:

In D23015#502970, @mehdi_amini wrote:

OK, let me backtrack, is the original issue coming from the fact that gold is starting with an empty module?
Then why not changing this?

We use gold's symbol resolution to determine which values to link in. So in the example, it knows that f1 is a preempted symbol in %t2.o and doesn't need to include its definition. I don't think we should need to change that.

I see...
I still does not think that's a great thing: using the IRMover just to get a module where some symbols are pruned seems quite sketchy to me.

Here we accept that the module can have renamed symbols from what is in the index, but that should be an invariant or we'll run in trouble at some point. So I'm not convinced the fix is right at this point.

I dug a bit more, I'm still trying to follow how the bug happens: the IRMover is forcing @f1 (in Inputs/thinlto_alias2.ll) to be linked to satisfy the alias @a2. And since there is already a declaration for @f1 when it does so, it is supposed to create a local copy (i.e. it will be an *internal* function), IRMover.cpp ~line 930.

Now the code you are patching is a callback for the internalize utility, and IIUC it is not called on internal function (which I expect to be the case here), as I read InternalizePass::shouldPreserveGV.

[tejohnson]

In D23015#503045, @mehdi_amini wrote:

In D23015#503013, @tejohnson wrote:

In D23015#502970, @mehdi_amini wrote:

OK, let me backtrack, is the original issue coming from the fact that gold is starting with an empty module?
Then why not changing this?

We use gold's symbol resolution to determine which values to link in. So in the example, it knows that f1 is a preempted symbol in %t2.o and doesn't need to include its definition. I don't think we should need to change that.

I see...
I still does not think that's a great thing: using the IRMover just to get a module where some symbols are pruned seems quite sketchy to me.

Here we accept that the module can have renamed symbols from what is in the index, but that should be an invariant or we'll run in trouble at some point. So I'm not convinced the fix is right at this point.

I decided I agree, see below.

I dug a bit more, I'm still trying to follow how the bug happens: the IRMover is forcing @f1 (in Inputs/thinlto_alias2.ll) to be linked to satisfy the alias @a2. And since there is already a declaration for @f1 when it does so, it is supposed to create a local copy (i.e. it will be an *internal* function), IRMover.cpp ~line 930.

Correct.

Now the code you are patching is a callback for the internalize utility, and IIUC it is not called on internal function (which I expect to be the case here), as I read InternalizePass::shouldPreserveGV.

It has been promoted though (by renameModuleForThinLTO), so it is not internal at this point (which is why the CHECK in the new test case looks like "CHECK: define hidden i32 @f1.1.llvm.0"

But after I sent this last night I realized that we probably should not have the gold-plugin drop any preempted symbols at this point. The reason is that we are losing optimization opportunities:

1. Loss of inlining opportunity for the dropped preempted linkonce_odr: In this case the weak_odr can't be inlined anyway, but if it was linkonce_odr, we wouldn't be able to inline it if it was preempted (we would have to import it back in, but the import decisions will be made when we still have the linkonce_odr definition and the calls wouldn't be external, but in any case there isn't any point in dropping only to import for inlining later).
2. Loss of internalization for the local copy - in this case we are checking to see if we can internalize the promoted symbol, which we would be able to do in this case if we could locate its summary. So we lose this opportunity.

I have made a change to the gold-plugin to keep the preempted symbols as well. However, this exposed another issue: when we keep the preempted linkonce in the comdat, thinLTOResolveWeakForLinkerModule comes along and changes it to available_externally. We then get a verification failure about a declaration in a comdat.

Interestingly, we have a fix for this in renameModuleForThinLTO, which drops any available_externally from comdats. It is meant to clean up after importing, but if I reorder renameModuleForThinLTO and thinLTOResolveWeakForLinkerModule in gold-plugin, it fixes this particular issue. I managed to reproduce this issue with an llvm-lto test case, since ProcessThinLTOModule() in ThinLTOCodeGenerator.cpp has the same ordering. It is also fixed by flipping the ordering between these two. Interestingly, and probably completely accidentally, ThinLTOCodeGenerator::promote() already has the desirable ordering. However, I don't like the subtle ordering requirement between these two calls. Perhaps it is better to explicitly strip any new available_externally symbols from comdats in thinLTOResolveWeakForLinkerModule. Hmm, I think I like that approach better. WDYT? Let me do that, will do so and clean up and add the new test cases to the patches later today.

[tejohnson]

Change to avoid dropping preempted symbols early.

[tejohnson]

Ping.

[mehdi_amini]

Sorry missed the update, will try to get to it today.

[mehdi_amini]

LGTM, Thanks!

[tejohnson]

I discovered while merging this patch with head on Friday that the Resolution-based LTO API I committed for pcc at r278338 already fixes this issue in the same way.

However, the following aspect is still an issue:

---

In D23015#503394, @tejohnson wrote:

I have made a change to the gold-plugin to keep the preempted symbols as well. However, this exposed another issue: when we keep the preempted linkonce in the comdat, thinLTOResolveWeakForLinkerModule comes along and changes it to available_externally. We then get a verification failure about a declaration in a comdat.

Interestingly, we have a fix for this in renameModuleForThinLTO, which drops any available_externally from comdats. It is meant to clean up after importing, but if I reorder renameModuleForThinLTO and thinLTOResolveWeakForLinkerModule in gold-plugin, it fixes this particular issue. I managed to reproduce this issue with an llvm-lto test case, since ProcessThinLTOModule() in ThinLTOCodeGenerator.cpp has the same ordering. It is also fixed by flipping the ordering between these two. Interestingly, and probably completely accidentally, ThinLTOCodeGenerator::promote() already has the desirable ordering. However, I don't like the subtle ordering requirement between these two calls. Perhaps it is better to explicitly strip any new available_externally symbols from comdats in thinLTOResolveWeakForLinkerModule. Hmm, I think I like that approach better. WDYT? Let me do that, will do so and clean up and add the new test cases to the patches later today.

---

It turns out that with pcc's changes we get the fortuitous ordering of renameModuleForThinLTO after thinLTOResolveWeakForLinkerModule in the new lto::thinBackend, so the issue is not exposed via the gold-plugin. It is however, still an issue in llvm-lto (and I realized in looking into this that I had forgotten to add my new llvm-lto based test case to this patch).

I am going to change this patch to only include this particular fix, along with the llvm-lto based test, and submit it.

[tejohnson]

Update to only include fix and test case for dropping available_externally
from comdats.

[mehdi_amini]

LGTM.

[tejohnson]

Rename test to something more appropriate before committing.

llvm/trunk/lib/Transforms/IPO/FunctionImport.cpp

auto updateLinkage = [&](GlobalValue &GV) {
     if (GS == DefinedGlobals.end())
       return;
     auto NewLinkage = GS->second->linkage();
     if (NewLinkage == GV.getLinkage())
       return;
     DEBUG(dbgs() << "ODR fixing up linkage for `" << GV.getName() << "` from "
                  << GV.getLinkage() << " to " << NewLinkage << "\n");
     GV.setLinkage(NewLinkage);
+    // Remove functions converted to available_externally from comdats,
+    // as this is a declaration for the linker, and will be dropped eventually.
+    // It is illegal for comdats to contain declarations.
+    auto *GO = dyn_cast_or_null<GlobalObject>(&GV);
+    if (GO && GO->isDeclarationForLinker() && GO->hasComdat()) {
+      assert(GO->hasAvailableExternallyLinkage() &&
+             "Expected comdat on definition (possibly available external)");
+      GO->setComdat(nullptr);
+    }
   };
 
   // Process functions and global now
   for (auto &GV : TheModule)
     updateLinkage(GV);
   for (auto &GV : TheModule.globals())
     updateLinkage(GV);
   for (auto &GV : TheModule.aliases())

llvm/trunk/test/ThinLTO/X86/Inputs/linkonce_resolution_comdat.ll

+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$c2 = comdat any
+
+define linkonce_odr i32 @f(i8*) unnamed_addr comdat($c2) {
+    ret i32 41
+}
+
+define i32 @g() {
+    %i = call i32 @f(i8* null)
+    ret i32 %i
+}

llvm/trunk/test/ThinLTO/X86/linkonce_resolution_comdat.ll

+; This test ensures that we drop the preempted copy of @f from %t2.bc from its
+; comdat after making it available_externally. If not we would get a
+; verification error.
+; RUN: opt -module-summary %s -o %t1.bc
+; RUN: opt -module-summary %p/Inputs/linkonce_resolution_comdat.ll -o %t2.bc
+; RUN: llvm-lto -thinlto-action=run %t1.bc %t2.bc -exported-symbol=f -exported-symbol=g -disable-inlining
+
+; RUN: llvm-nm -o - < %t1.bc.thinlto.o | FileCheck %s --check-prefix=NM1
+; NM1: W f
+
+; RUN: llvm-nm -o - < %t2.bc.thinlto.o | FileCheck %s --check-prefix=NM2
+; f() would have been turned into available_externally since it is preempted,
+; and inlined into g()
+; NM2-NOT: f
+
+target datalayout = "e-m:e-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+$c1 = comdat any
+
+define linkonce_odr i32 @f(i8*) unnamed_addr comdat($c1) {
+    ret i32 43
+}