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

[Erich Keane via Phabricator via llvm-commits]

erichkeane added a comment.

In D112349#3112315 <https://reviews.llvm.org/D112349#3112315>, @v.g.vassilev wrote:

> In D112349#3111927 <https://reviews.llvm.org/D112349#3111927>, @erichkeane wrote:
>
>> In D112349#3111873 <https://reviews.llvm.org/D112349#3111873>, @ibookstein wrote:
>>
>>> And how is Cling expecting CFE to deal with partial knowledge situations at the implementation level? To deal with exactly the non-local cases that the current violations address?
>>> If there's no prescriptive way forward to dealing with these cases (so they're tech debt without a remediation plan), then as far as I'm concerned this case sits exactly under the same tech-debt umbrella of the existing violations and the way forward is using the same violating solution for this case too. 
>>> We definitely shouldn't block the IR verification indefinitely on this impasse. Once an acceptable solution is found, this will also be part of that refactor.
>>
>> My understanding is that the REPL setup is that the 'IR' just needs to be in a state where it doesn't require reverts/rewrites later, so that we can do partial-back-end-code-generation as we go along.  That said, I'm not positive as to the implications.  I'm just repeating the discussion the CFE code-owner made at the time.
>>
>> IMO, the 'acceptable' solution is to have a way to forward-declare an ifunc in IR so that we can fill in the resolver later on.  From your description earlier, it seems that this would work as we could emit it as an 'unknown symbol' (as if it was an undefined function declaration), and would be completely implementable in the CFE.
>>
>> So it would change your plan from earlier to:
>>
>> When processing cpu_specific, emit the ifunc "x.ifunc", with no resolver;
>> When processing cpu_dispatch:
>>
>>   Get/Create the ifunc, then pull up the resolver.
>>   If the resolver is null (as it should be), create one and update the 'ifunc'.
>>   Generate said resolver.
>>   
>
> Speaking of the incremental compilation case, we can experiment with the clang-repl binary. I am not sure about the details of this discussion but here is an example that works today:
>
>   llvm/build/bin/clang-repl 
>   clang-repl> __attribute__((cpu_specific(ivybridge))) void single_version(void){}
>   clang-repl> void useage() {single_version();}
>   clang-repl> quit
>
> What would it be a good example to check if the incremental compilation case is covered?

FWIW, the comments above are about not making it 'worse' at least/adding more wo

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

> I feel like we're getting lost in the weeds here.
>
> At the time a bitcode module is finalized, it is supposed to be in a valid state. 
> 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.
>
> There are two separate issues here which we need to decouple:
>
> - IFuncs with undefined resolvers were never valid, the verification just happened to be missing. They also misbehave, as demonstrated by my example above where a TU contains both cpu_specific and a usage, but no cpu_dispatch. Therefore, we'd not be making anything worse by plugging the current hole in the verification of GlobalIFunc and keeping cpu_specific/cpu_dispatch working, using the same method we use to handle aliases or ifuncs that come after declarations, i.e. cpu_specific emits plain function declarations, cpu_dispatch upgrades them via takeName+RAUW+eraseFromParent if they already exist. We'll have made the verification more robust, fixed a bug when there's a TU where there's cpu_specific + usage but no cpu_dispatch, and not have incurred more tech debt by doing so (since that tech debt already exists, and a solution would need to address all these cases in exactly the same way).
> - Clang-repl/CGM needs infrastructure for dealing with this sort of 'backtracking' in incremental compilation use-cases (declaration gets upgraded to alias, declaration gets upgraded to ifunc). If it needs IR changes for that, then they should be designed, agreed upon, and integrated. We're not going to have a decision made in this closed PR discussion that either GlobalAliases or GlobalIFuncs support a declaration state all of a sudden. Such decisions could have cross-cutting ramifications in that there would all of a sudden be 3 ways to represent things that are equivalent to/get lowered to function declarations, rather than one. Limiting ourselves to not using the existing solution for these use-cases/bugs in normal compilation with the hope that it will ease the creation of a solution for incremental compilation of the same use-cases is self-defeating.
>
> As for clang-repl, I've so far tried the following; I get the sense that I'm poking around in less well-specified places (asserts and null-deref crashes):
>
>   itay ~/llvm-project/build (main)> bin/clang-repl
>   clang-repl> #include <stdio.h>
>   clang-repl> __attribute__((cpu_dispatch(generic))) void a(void);
>   clang-repl> __attribute__((cpu_specific(generic))) void a(void) { puts("hi"); }
>   In file included from <<< inputs >>>:1:
>   input_line_2:1:55: warning: body of cpu_dispatch function will be ignored [-Wfunction-multiversion]
>   __attribute__((cpu_specific(generic))) void a(void) { puts("hi"); }
>                                                         ^
>   clang-repl> auto b = (a(), 5);
>   clang-repl: /home/itay/llvm-project/llvm/lib/CodeGen/AsmPrinter/AsmPrinter.cpp:1683: void llvm::AsmPrinter::emitGlobalIFunc(llvm::Module &, const llvm::GlobalIFunc &): Assertion `GI.hasLocalLinkage() && "Invalid ifunc linkage"' failed.
>   fish: 'bin/clang-repl' terminated by signal SIGABRT (Abort)
>   itay ~/llvm-project/build (main) [SIGABRT]> bin/clang-repl
>   clang-repl> extern int g_a __attribute__((alias("g_b")));
>   In file included from <<< inputs >>>:1:
>   input_line_0:1:31: error: alias must point to a defined variable or function
>   extern int g_a __attribute__((alias("g_b")));
>                                 ^
>   fish: 'bin/clang-repl' terminated by signal SIGSEGV (Address boundary error)
>   itay ~/llvm-project/build (main) [SIGSEGV]> bin/clang-repl
>   clang-repl> void foo(void) __attribute__((ifunc("foo_resolver")));
>   In file included from <<< inputs >>>:1:
>   input_line_1:1:31: error: ifunc must point to a defined function
>   void foo(void) __attribute__((ifunc("foo_resolver")));
>                                 ^
>   fish: 'bin/clang-repl' terminated by signal SIGSEGV (Address boundary error)
>   itay ~/llvm-project/build (main) [SIGSEGV]> bin/clang-repl
>   clang-repl> void foo(void);
>   clang-repl> void bar(void) __attribute__((alias("foo")));
>   In file included from <<< inputs >>>:1:
>   input_line_1:1:31: error: alias must point to a defined variable or function
>   void bar(void) __attribute__((alias("foo")));
>                                 ^
>   fish: 'bin/clang-repl' terminated by signal SIGSEGV (Address boundary error)
>   itay ~/llvm-project/build (main) [SIGSEGV]> bin/clang-repl
>   clang-repl> void foo(void); void bar(void) __attribute__((alias("foo"))); void foo(void) {}
>   In file included from <<< inputs >>>:1:
>   input_line_0:1:47: error: alias must point to a defined variable or function
>   void foo(void); void bar(void) __attribute__((alias("foo"))); void foo(void) {}
>                                                 ^
>   fish: 'bin/clang-repl' terminated by signal SIGSEGV (Address boundary error)
>   itay ~/llvm-project/build (main) [SIGSEGV]> bin/clang-repl
>   clang-repl> void foo(void) {}
>   clang-repl> void bar(void) __attribute__((alias("foo")));
>   In file included from <<< inputs >>>:1:
>   input_line_1:1:31: error: alias must point to a defined variable or function
>   void bar(void) __attribute__((alias("foo")));
>                                 ^
>   fish: 'bin/clang-repl' terminated by signal SIGSEGV (Address boundary error)

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.

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


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112349