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

[Erich Keane via Phabricator via cfe-commits]

erichkeane added a comment.

Explanations make sense to me, I'm generally in favor with the 1 concern.



================
Comment at: clang/lib/CodeGen/CodeGenModule.cpp:397
 
+    bool IsIFunc = isa<llvm::GlobalIFunc>(Alias);
     llvm::Constant *Aliasee =
----------------
ibookstein wrote:
> 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 :)
The CFE (particularly when doing checks, but even when generating code) should be doing as much based on the AST rather than the IR, as it makes us a bit more fragile/dependent on the form of the IR. So typcially we try to avoid determining types/etc from IR.

Note we break this rule a bunch, but it is currently burning us on opaque-ptr for example.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D112868