Fix -fsplit-lto-unit with ifuncs

[danielkiss]

Summary

ifuncs can't take part of the whole-program devirtualization so no need them to be copied to the merged module.
The corresponding resolver function also kept out which caused the crash.

Fixes #60962 #57870

[tejohnson]

I'm a little confused - did we create an illegal split module by cloning the ifunc to the regular LTO split module? Should the handling in CloneModule (particularly with its use of the ShouldCloneDefinition callback) be changed so that ifunc are not cloned if their resolver is not cloned?

llvm/test/ThinLTO/X86/ifunc_splitlto.ll

30

Is the second !2 supposed to be !3? Otherwise there is a circular metadata reference and no use of !3.

[danielkiss]

In D144982#4169480, @tejohnson wrote:

I'm a little confused - did we create an illegal split module by cloning the ifunc to the regular LTO split module? Should the handling in CloneModule (particularly with its use of the ShouldCloneDefinition callback) be changed so that ifunc are not cloned if their resolver is not cloned?

simplifyExternals turns the resolver to external. Externals won't be in the module summary which will be referenced by the ifunc.

[tejohnson]

In D144982#4170950, @danielkiss wrote:

In D144982#4169480, @tejohnson wrote:

I'm a little confused - did we create an illegal split module by cloning the ifunc to the regular LTO split module? Should the handling in CloneModule (particularly with its use of the ShouldCloneDefinition callback) be changed so that ifunc are not cloned if their resolver is not cloned?

simplifyExternals turns the resolver to external. Externals won't be in the module summary which will be referenced by the ifunc.

Right, simplifyExternals ensures that any symbols cloned into the merged Module as declarations are either dropped if there are no references or made external if there are. But this is not specific to resolver functions.

Ah, I see that the latest version of the patch has moved the new ifunc handling into simplifyExternals, and only drops the ifunc if the resolver func is not defined in the module. This is better as it isn't unconditional.

What I was asking was whether we can handle this directly in CloneModule via the callback, instead of removing the ifunc later. I.e. should CloneModule be changed to invoke ShouldCloneDefinition on the ifunc resolver function before deciding whether to clone over each ifunc definition? Because it generally seems incorrect to create a module with an ifunc with a resolver function not defined in that module (i.e. not just when splitting the ThinLTO module, but any time we invoke CloneModule with a callback that doesn't simply return true for everything).

However, what would happen if one of the functions that we do decide to clone itself references/calls an ifunc? In that case with your patch or my proposed change you will I think end up with an bad module since you are removing the ifunc completely and not replacing it with anything, so you would have a reference to a nonexistent symbol. For aliases CloneModule handles this by converting the alias into either a function or variable external declaration. I'm not sure what the correct behavior for an ifunc is?

[danielkiss]

In D144982#4171702, @tejohnson wrote:

In D144982#4170950, @danielkiss wrote:

In D144982#4169480, @tejohnson wrote:

I'm a little confused - did we create an illegal split module by cloning the ifunc to the regular LTO split module? Should the handling in CloneModule (particularly with its use of the ShouldCloneDefinition callback) be changed so that ifunc are not cloned if their resolver is not cloned?

simplifyExternals turns the resolver to external. Externals won't be in the module summary which will be referenced by the ifunc.

Right, simplifyExternals ensures that any symbols cloned into the merged Module as declarations are either dropped if there are no references or made external if there are. But this is not specific to resolver functions.

Ah, I see that the latest version of the patch has moved the new ifunc handling into simplifyExternals, and only drops the ifunc if the resolver func is not defined in the module. This is better as it isn't unconditional.

What I was asking was whether we can handle this directly in CloneModule via the callback, instead of removing the ifunc later. I.e. should CloneModule be changed to invoke ShouldCloneDefinition on the ifunc resolver function before deciding whether to clone over each ifunc definition? Because it generally seems incorrect to create a module with an ifunc with a resolver function not defined in that module (i.e. not just when splitting the ThinLTO module, but any time we invoke CloneModule with a callback that doesn't simply return true for everything).

I think it is even more complicated because it is valid to call the resolver directly so copying the resolver might not mean anything for the ifunc. (see baz here: https://godbolt.org/z/PWzYoEv6z)

If the CloneModule handles this than we break the llvm-reduce/remove-ifunc-program-addrspace.ll.
I need to investigate why the ifunc_remove_as1_resolver_casted_in_1 won't see the resolver (name of it becomes empty) while cloning module.

However, what would happen if one of the functions that we do decide to clone itself references/calls an ifunc? In that case with your patch or my proposed change you will I think end up with an bad module since you are removing the ifunc completely and not replacing it with anything, so you would have a reference to a nonexistent symbol. For aliases CloneModule handles this by converting the alias into either a function or variable external declaration. I'm not sure what the correct behavior for an ifunc is?

Intuitively ifuncs would be external declaration for users as they are lowered today but that won't work for platforms where the resolver would be called in place(instead of PLTs on Linux/elf).
Probably in these cases we need to carry the resolver if the ifuncs need to be copied.

[danielkiss]

let's remove ifuncs if not used.

[danielkiss]

Last version fixes the Chromium and others crashes. Generic solution for CloneModule is conceptually more complex and probably orthogonal to this change.

[tejohnson]

In D144982#4178747, @danielkiss wrote:

In D144982#4171702, @tejohnson wrote:

In D144982#4170950, @danielkiss wrote:

In D144982#4169480, @tejohnson wrote:

I'm a little confused - did we create an illegal split module by cloning the ifunc to the regular LTO split module? Should the handling in CloneModule (particularly with its use of the ShouldCloneDefinition callback) be changed so that ifunc are not cloned if their resolver is not cloned?

simplifyExternals turns the resolver to external. Externals won't be in the module summary which will be referenced by the ifunc.

Right, simplifyExternals ensures that any symbols cloned into the merged Module as declarations are either dropped if there are no references or made external if there are. But this is not specific to resolver functions.

Ah, I see that the latest version of the patch has moved the new ifunc handling into simplifyExternals, and only drops the ifunc if the resolver func is not defined in the module. This is better as it isn't unconditional.

What I was asking was whether we can handle this directly in CloneModule via the callback, instead of removing the ifunc later. I.e. should CloneModule be changed to invoke ShouldCloneDefinition on the ifunc resolver function before deciding whether to clone over each ifunc definition? Because it generally seems incorrect to create a module with an ifunc with a resolver function not defined in that module (i.e. not just when splitting the ThinLTO module, but any time we invoke CloneModule with a callback that doesn't simply return true for everything).

I think it is even more complicated because it is valid to call the resolver directly so copying the resolver might not mean anything for the ifunc. (see baz here: https://godbolt.org/z/PWzYoEv6z)

If the CloneModule handles this than we break the llvm-reduce/remove-ifunc-program-addrspace.ll.
I need to investigate why the ifunc_remove_as1_resolver_casted_in_1 won't see the resolver (name of it becomes empty) while cloning module.

I suppose it is because it is valid to copy just the resolver as you mentioned, and I guess in general CloneModule isn't trying to split the module in 2.

However, what would happen if one of the functions that we do decide to clone itself references/calls an ifunc? In that case with your patch or my proposed change you will I think end up with an bad module since you are removing the ifunc completely and not replacing it with anything, so you would have a reference to a nonexistent symbol. For aliases CloneModule handles this by converting the alias into either a function or variable external declaration. I'm not sure what the correct behavior for an ifunc is?

Intuitively ifuncs would be external declaration for users as they are lowered today but that won't work for platforms where the resolver would be called in place(instead of PLTs on Linux/elf).
Probably in these cases we need to carry the resolver if the ifuncs need to be copied.

Can we at least add an assert for now to the split module code that we don't end up with either split module needing the ifunc but not containing its resolver?

[tejohnson]

lgtm

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

203

nit: its not it's in this context.

[danielkiss]

Thanks!

llvm/lib/Transforms/IPO/ThinLTOBitcodeWriter.cpp

for (Function &F : llvm::make_early_inc_range(M)) {
     F.replaceAllUsesWith(ConstantExpr::getBitCast(NewF, F.getType()));
     F.eraseFromParent();
   }
 
   for (GlobalVariable &GV : llvm::make_early_inc_range(M.globals())) {
     if (GV.isDeclaration() && GV.use_empty()) {
       GV.eraseFromParent();
       continue;
     }
   }
 }
 
 static void
 filterModule(Module *M,
              function_ref<bool(const GlobalValue *)> ShouldKeepDefinition) {
   std::vector<GlobalValue *> V;
   for (GlobalValue &GV : M->global_values())
std::unique_ptr<Module> MergedM(
         if (auto *GVar =
                 dyn_cast_or_null<GlobalVariable>(GV->getAliaseeObject()))
           return HasTypeMetadata(GVar);
         return false;
       }));
   StripDebugInfo(*MergedM);
   MergedM->setModuleInlineAsm("");
 
+  // Remove ifuncs from MergedM due the corresponding functions are also
+  // kept out from MergeM.
+  SmallVector<GlobalIFunc *, 8> IFuncsToErease;
+  for (auto &IF : MergedM->ifuncs())
+    IFuncsToErease.push_back(&IF);
+  for (auto &IF : IFuncsToErease)
+    MergedM->eraseIFunc(IF);
+
   // Clone any llvm.*used globals to ensure the included values are
   // not deleted.
   cloneUsedGlobalVariables(M, *MergedM, /*CompilerUsed*/ false);
   cloneUsedGlobalVariables(M, *MergedM, /*CompilerUsed*/ true);
 
   for (Function &F : *MergedM)
     if (!F.isDeclaration()) {
       // Reset the linkage of all functions eligible for virtual constant

llvm/test/ThinLTO/X86/ifunc_splitlto.ll

+; regresstion test for https://github.com/llvm/llvm-project/issues/60962
+; RUN: opt -thinlto-bc -thinlto-split-lto-unit -o %t %s
+; RUN: llvm-modextract -b -n 0 -o - %t | llvm-dis | FileCheck --check-prefix=M0 %s
+; RUN: llvm-modextract -b -n 1 -o - %t | llvm-dis | FileCheck --check-prefix=M1 %s
+
+target datalayout = "e-m:e-p270:32:32-p271:32:32-p272:64:64-i64:64-f80:128-n8:16:32:64-S128"
+target triple = "x86_64-unknown-linux-gnu"
+
+@i = ifunc ptr (ptr, i64), ptr @hoge
+
+@g = constant i8 1, !type !0
+@assoc = private constant i8 2, !associated !1
+
+define ptr @hoge() !type !2 {
+bb:
+  ret ptr null
+}
+
+; M0: @g = external constant
+; M0: @i = ifunc ptr (ptr, i64), ptr @hoge
+; M0: define ptr @hoge()
+; M0-NOT: @assoc
+; M1: @g = constant i8 1
+; M1: @assoc = private constant i8 2
+; M1-NOT: @i = ifunc ptr (ptr, i64), ptr @hoge
+; M1-NOT: define ptr @hoge()
+
+!0 = !{i32 0, !"typeid"}
+!1 = !{ptr @g}
+!2 = !{i64 0, !2}
+!3 = distinct !{}