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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

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

>> To me the 'not in the weeds' part is, "How do I get CPU-dispatch/CPU-specific to work without RAUW, since that is offensive to the CFE code owner?  Additionally, I found that some of the examples without the defined resolver actually DO work in my downstream, though I don't know what changes we make to make that happen.  So adding this limitation in actually breaks my downstream.
>
> Regarding the examples that do work, I've provided explanations as to how they partially sort-of-work, but that the general semantics are broken (the 'connection' to the resolver is 'severed' in each translation unit that has it as a declaration + all references from within that TU are borked).

As I mentioned, my downstream actually DOES work, I believe our code-generator has some changes that are making this work, but I haven't found the actual commit yet.  I suspect it is some level of changes that emit ifuncs with undefined resolvers differently.

> The problems that the CFE currently uses RAUW to solve are //fundamental// to the specified behavior of the **language features** that use it, so a solution for these problems needs to arise from the CFE. Imagine the following incremental compilation use-case:
>
>   > extern int x; // x is a GlobalVariable declaration (initializer == null).
>   > int y; // y is a GlobalVariable definition (initializer = constant 0).
>   > extern int x __attribute__((alias("y"))); // x needs to transform into a GlobalAlias now.
>   >
>   > extern void *my_memcpy(void *dest, void *src, size_t n); // my_memcpy is a Function declaration
>   > void *my_memcpy(void *dest, void *src, unsigned long n) __attribute__((ifunc("my_memcpy_resolver"))); // my_memcpy_resolver is a Function declaration, my_memcpy is a GlobalIFunc with declaration for a resolver - whoops, incremental module is invalid at this point
>   > static void *my_memcpy_impl(void *dest, void *src, unsigned long n) { return 0; }
>   > static void *my_memcpy_resolver(void) { return &my_memcpy_impl; } // Now my_memcpy ifunc has defined resolver
>   >
>   > void CpuSpecific(void); // CpuSpecific is a Function declaration
>   > void Foo(void) { CpuSpecific(); } // Foo calls a function declaration
>   > __attribute__((cpu_specific(generic))) void CpuSpecific(void) { puts("generic"); } // We still don't know whether cpu_dispatch will be in this TU or not. Do we upgrade CpuSpecific from a function declaration to a definition? to an ifunc? Bind directly to this body?
>   > __attribute__((cpu_dispatch(generic))) void CpuSpecific(void); // Now we know that we have to upgrade it to an ifunc, and how the resolver should look. But it's already a Function (declaration).
>
> All the above //need// to work in non-incremental compilation, and the only existing way for them to work right now is RAUW or a module finalization step. If the CFE code owner(s) find that offensive but proposes no alternative, what is one to do? There are 3 possible states here:
>
> 1. Remain broken

As mentioned above, not necessarily always broken, my downstream works with your examples of how its broken.  Breaking this is IMO, not acceptable.

> 2. Solve using RAUW/finalization

This is unacceptable to the Clang CFE maintainer, so I believe this is a non-starter.

> 3. Solve using yet-unproposed non-RAUW solution (which solves all of the above use-cases simultaneously because they're essentially the same)

My preference is clearly this one.  If we had a way to create the ifunc in the CFE with NO resolver (that is, a forward-declaration of an ifunc!) that could be emitted as-if it were a normal function call (which is what you suggested to happen in the CFE, so this is just suggesting it later), it solves my problem.  As far as I can tell, this is what my downstream is doing (except the key-off here is whether the resolver is not defined, not null).  Can we discuss an actual solution as a part of this discussion?

> It is better to treat (2) as a way out of (1) which doesn't increase the cost of (3), than to just stay at (1) until (3) happens. As it currently stands, the somewhat similar issue of calling a declared-but-undefined function in the REPL is currently embodied as a JIT session error, with failure to materialize symbols. Perhaps a valid solution for the work-in-progress aliases and ifuncs is to transform them into declarations in the JIT module until they have definitions for their aliasees/resolvers. But we won't know without more context. I think it might be more effective that I'll write a patch up which does use RAUW together with a test for the breakage we discussed and we'll continue the discussion with the CFE code owner(s) there.
>
>>> The LLVM bitcode verifier does not consider GlobalAliases which have either a null or an undefined aliasee to be valid. The same should have held for GlobalIFuncs and their resolvers since their inception, and the fact that it were not so is an oversight.
>>
>> Citation Needed here.  Is there a design document that specifies this, or is this your opinion?  If its the latter, the implementation was the documentation, so this is still a breaking change.
>
> I'm assuming that you're talking about GlobalIFuncs, since for GlobalAliases you'll see both constraints encoded in the `Verifier::visitGlobalAlias` and `Verifier::visitAliaseeSubExpr` functions.
> As for GlobalIFunc, I have no design document for the LLVM-IR-level representation of the feature, but:
>
> 1. @MaskRay has provided reference for the object-format constraint itself (`Requirement (a): Resolver must be defined in the same translation unit as the implementations.`). Bitcode Modules correspond to TUs - so it stands to reason that the same restriction apply to them, unless behavior is implemented to bridge the gap.
> 2. I've demonstrated that compiling a bitcode module containing an ifunc with an undefined resolver emits broken code for all usages of the ifunc-with-no-resolver, where I can probably massage that into a crash with a small bit of additional work (since it ends up calling the resolver rather than calling the return value of the resolver). This applies to current use-cases of cpu_specific in translation units that don't contain the corresponding cpu_dispatch, which I understand is part of the documented usage model of cpu_specific/cpu_dispatch. So it's already subtly broken, and in reference to (1), this means that behavior to bridge the gap between what the IR ostensibly allows and what the object format allows is **not** implemented.
> 3. CFE already diagnoses this issue for plain ifuncs and aliases.

So its important to note here that a 'language level' ifunc is not a perfect match to the IR-level ifunc, which is not a perfect match to an object-file ifunc.  These are 3 different features that map closely, but not exactly.

> 4. I've drawn significant parallels between aliases and ifuncs at the object file level, and extrapolated the reasoning for why aliases have to point at defined objects to the reasoning why ifuncs have to point at defined objects.
>
> So, when I say "should have held" I am indeed expressing my opinion. But I do believe that I've backed it up substantially, and that from a design perspective adding that restriction to the IR really is the correct decision.

I appreciate the explanation.

> Calling the implementation the documentation doesn't ring right with me - is the broken behavior I referred to in (2) a compatibility constraint now? Are the bizarre disallowed-by-the-specification undefined STT_GNU_IFUNC symbols emitted by cpu_specific-without-cpu_dispatch a compatibility constraint now?

In absence of a spec, the implement-is-the-spec just by nature.  As I mentioned, my downstream DOES have this work, so this is a compatibility constraint (and we as a project tend to at least try to work with downstreams by not breaking them this aggressively). That said, I am, and have been willing to work with you to make the reasonable changes here.  I'm trying to get us to a solution that doesn't break my users and allows you to recommit your patch.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112349