[PATCH] D112868: [Sema] Diagnose and reject non-function ifunc resolvers

[Itay Bookstein via Phabricator via cfe-commits]

ibookstein added inline comments.


================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:320
 // linear structure.
 static const llvm::GlobalValue *getAliasedGlobal(const llvm::GlobalValue *GV) {
+  const llvm::Constant *C;
----------------
erichkeane wrote:
> Can you explain a bit better how this change here works?  The previous version appears to have done quite a bit of work in the loop to look through aliases/ifuncs, and it seems we don't do that anymore?  
Previously this function would have drilled through an arbitrary ifunc->alias->ifunc->alias interleaving structure. This is sort of peculiar since for alias->ifunc it would return the resolver (rather than the aliasee, which is the ifunc itself), and for ifunc->ifunc (which is invalid anyway) it would return the second ifunc's resolver.
In this change I want it to stop at the first non-alias object it encounters, and for ifuncs diagnose when the type of such an object is incorrect.
That is exactly what the `GlobalValue::getAliaseeObject()` function does - it pierces through aliases in a loop.

In essence, in an alias->ifunc->alias->... chain there should always be 0 or 1 ifuncs, and if there's an ifunc the last object should be a resolver function (and not a global variable or an ifunc).
If you were to try the following code snippet, you'll see that it asserts/crashes clang prior to this patch:
```
int g_var;
void foo(void) __attribute__((ifunc("g_var")));
```



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:397
 
+    bool IsIFunc = isa<llvm::GlobalIFunc>(Alias);
     llvm::Constant *Aliasee =
----------------
erichkeane wrote:
> What is the purpose of changing this from checking the declaration to the IR?  It seems that this is something we should be able to catch at the AST level and not have to revert to IR checks like this.
Inside `checkAliasedGlobal` I wanted to save on a parameter (`D`) since then I would just be passing it redundantly because the information is already available on the `Alias`. Here I wanted the check to be an exact copy of the check inside `checkAliasedGlobal` for consistency. I don't mind changing that at at all, I guess I just don't see why I should prefer one over the other :)


================
Comment at: clang/test/Sema/attr-ifunc.c:16
 
-void* f2_a() __attribute__((ifunc("f2_b")));
-//expected-error at -1 {{ifunc definition is part of a cycle}}
+void *f2_a() __attribute__((alias("f2_b")));
 void* f2_b() __attribute__((ifunc("f2_a")));
----------------
erichkeane wrote:
> Did we lose this previous error?  We still want that to happen, right?
One of the two errors (the one on `f2_b`) is enough, IMO.
There's some lopsidedness in this 'cycle' diagnosis because I consider the ifunc 'hop' to be special - it is no longer treated as a plain alias to loop into.
This case is the `if (FinalGV == GV)` in the `getAliasedGlobal` implementation above. It's also possible to diagnose this as "resolver of ifunc cannot be ifunc" (if I remove that condition).


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112868