[PATCH] D112349: [Verifier] Add verification logic for GlobalIFuncs

[Erich Keane via Phabricator via llvm-commits]

erichkeane added a comment.



In D112349#3104479 <https://reviews.llvm.org/D112349#3104479>, @ibookstein wrote:

> Hmm. When I try to compile an object file where the resolver is a declaration, both clang-13, clang-14, and gcc-9.3 complain that the ifunc must point to a defined function:
>
>   void *foo_resolver();
>   void foo(void) __attribute__((ifunc("foo_resolver")));
>
> clang (13 and 14) complain:
>
>   obj.c:2:31: error: ifunc must point to a defined function
>   void foo(void) __attribute__((ifunc("foo_resolver")));
>                                 ^
>   1 error generated.
>
> gcc 9.3.0 complains:
>
>   obj.c:3:6: error: ‘foo’ aliased to undefined symbol ‘foo_resolver’
>       3 | void foo(void) __attribute__((ifunc("foo_resolver")));
>         |      ^~~
>
> I realize that the fact that frontends reject this doesn't necessarily mean that the IR they would have hypothetically produced is invalid, I'm just wondering about the semantics.
> Drawing some parallels to GlobalAliases, you'll see that they also check that the aliasee is a definition rather than a declaration (`Verifier::visitAliaseeSubExpr`), which was the reason I added the same check to `Verifier::visitGlobalIFunc`.

My understanding is the frontend's semantic rules are/were different from the IR rules, which is why we implemented it that way.

> Should an object file be produced with an UND ifunc symbol in that case? Wouldn't it be more correct to emit a plain old function declaration? (see `llvm/test/Linker/ifunc.ll` for behavior of linking an ifunc on top of a function declaration, should work correctly).

I'm not sure what you mean here?  Are you suggesting that an undefined resolver should instead just implement an undefined 'function' for the .ifunc?  This doesn't seem right?  Wouldn't that cause ODR issues?

> At the very least to alleviate the breakage I think we can just rip out the `Assert(!Resolver->isDeclarationForLinker(), "...")` from the Verifier, but I have a feeling that this is not the right long-term solution.

I guess I still don't understand what the practical limitation that requires ifuncs to have a defined resolver?  The resolver is just a normal function, so it seems to me that allowing them to have normal linking rules makes sense?  I personally think this is the least obtrusive change; this patch is adding a limitation that didn't exist previously unnecessarily.

> The cpu_specific/cpu_dispatch attributes are completely new to me, so bear with me if I'm misunderstanding; wouldn't the following implementation both provide the correct semantics and avoid an ifunc-with-an-undefined-resolver situation?
>
> - The cpu_specific attribute emits
>   1. A Function Declaration with a computed name "x.ifunc"

It just seems odd I guess to name a function .ifunc, and not have it be an ifunc?  What does our linker think about that?

>   1. A single Function with the cpu-specific body
>   2. Multiple GlobalAliases with computed names whose aliasee is the function from 2)
> - The cpu_dispatch attribute emits a strongly-defined non-interposable ifunc with the same computed name "x.ifunc", and a hidden defined resolver. Both IR linking and regular linking should resolve the plain function-delcaration-references to the ifunc properly.

I'm not sure what you mean by 'non-interposable'?  We are intentionally forming the .ifunc to have the same linkage as the original function, so anything we do that would break that is not acceptable.  Additionally, I'd hope that this wouldn't be an ABI break?  Additionally, the way the CFE generates these calls would require a pretty massive overhaul, since we'd have to "know" when to replace those in cases where the cpu-specific and cpu-dispatch are in the same TU.  Previously we did something similar for attribute-target multiversioning, but the idea of doing a replace-all-uses was considered unacceptable by the CFE code owners.

In D112349#3105292 <https://reviews.llvm.org/D112349#3105292>, @ibookstein wrote:

> Note that (as the examples demonstrate) clang self-verifies and checks among other things that ifuncs that it emits point to definitions; this happens in `CodeGenModule::checkAliases()`.
> I haven't read the cpu_specific/cpu_dispatch-related code in CodeGenModule yet, but I'm guessing that it doesn't register the generated aliases/ifuncs into the `CodeGenModule::Aliases` vector for deferred verification, which is why this didn't trigger the same error that my ifunc example did so far.

Thats correct, these aren't 'aliases' or 'ifuncs' as far as the CFE is concerned; they are multiversioned functions.  That 'Aliases' and 'ifunc' list in the CFE are the AST-constructs of those, not the IR constructs, so there is no reason to put the multiversioned thinks in that list, since they are implementation details.  Emitting an error "invalid alias!"/etc for


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112349