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

[Itay Bookstein via Phabricator via cfe-commits]

ibookstein added a comment.

> 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).

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
2. Solve using RAUW/finalization
3. Solve using yet-unproposed non-RAUW solution (which solves all of the above use-cases simultaneously because they're essentially the same)

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.
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.
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?


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112349