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

[Itay Bookstein via Phabricator via llvm-commits]

ibookstein added a comment.

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)


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112349