[PATCH] D139163: Utils: Add utility pass to lower ifuncs

[John McCall via Phabricator via llvm-commits]

rjmccall added a comment.

In D139163#3992262 <https://reviews.llvm.org/D139163#3992262>, @dexonsmith wrote:

> In D139163#3991248 <https://reviews.llvm.org/D139163#3991248>, @MoritzS wrote:
>
>> To answer your questions in the comments about what to do about resolvers with arguments: At least glibc always calls ifunc resolvers without any arguments. It just reads the address of the resolver function from the ELF file, casts it to a pointer to a void-returning function with no arguments and calls it.
>>
>> So, I agree: Resolvers with arguments should not be allowed.
>
> @rjmccall, I'm trying to remember whether Mach-O has an ifunc-like thing that takes an argument.

I don't know that we support ifuncs of any kind, but I'll ask.



================
Comment at: llvm/lib/Transforms/Utils/ModuleUtils.cpp:352-361
+    // We don't know what to pass to a resolver function taking arguments
+    //
+    // FIXME: Is this even valid? clang and gcc don't complain but this
+    // probably should be invalid IR. We could just pass through undef.
+    if (!std::empty(ResolvedFunction->getFunctionType()->params())) {
+      LLVM_DEBUG(dbgs() << "Not lowering ifunc resolver function "
+                        << ResolvedFunction->getName() << " with parameters\n");
----------------
dexonsmith wrote:
> This is where ifuncs with arguments are handled. @rjmccall, thoughts on the FIXME?
If possible, I agree that we should treat this as invalid and diagnose in the frontend.  However, it's possible that the current leniency puts us in a compatibility bind.  What would we do if the frontend is forced to accept resolver functions with parameters?  Can we substitute a thunk in the frontend that passes undefined arguments, or would that break semantics somehow?

Also, please fix this code to say "ResolverFunction" instead of "ResolvedFunction".  That is an extremely important difference.


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

https://reviews.llvm.org/D139163