[PATCH] D144982: Fix -fsplit-lto-unit with ifuncs

[Daniel Kiss via Phabricator via llvm-commits]

danielkiss added a comment.

In D144982#4171702 <https://reviews.llvm.org/D144982#4171702>, @tejohnson wrote:

> In D144982#4170950 <https://reviews.llvm.org/D144982#4170950>, @danielkiss wrote:
>
>> In D144982#4169480 <https://reviews.llvm.org/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 <https://github.com/llvm/llvm-project/blob/bc3e492323f34eb92f49897d22468aef2d831d5a/llvm/test/tools/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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D144982/new/

https://reviews.llvm.org/D144982