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@-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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
