[CodeGen] Diagnose and reject non-function ifunc resolvers

[ibookstein]

Summary

Signed-off-by: Itay Bookstein <[e-mail]>

[ibookstein]

clang-format

[aaron.ballman]

One thing that surprises me is that the test changes are in Sema but the code changes are in CodeGen. I see that this test actually emits LLVM IR. Should it be moved (as an NFC change)?

[erichkeane]

clang/lib/CodeGen/CodeGenModule.cpp

320–321

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?

398

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.

clang/test/Sema/attr-ifunc.c

16

Did we lose this previous error? We still want that to happen, right?

[ibookstein]

clang/lib/CodeGen/CodeGenModule.cpp

320–321

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")));

398

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 :)

clang/test/Sema/attr-ifunc.c

16

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

[erichkeane]

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

clang/lib/CodeGen/CodeGenModule.cpp

398

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.

[ibookstein]

Changed to using D->hasAttr<IFuncAttr>() and passing that
bool into the checkAliasedGlobal function.

[ibookstein]

Nicer param order for checkAliasedGlobal..

[ibookstein]

clang-format + fastforward rebase

[MaskRay]

In D112868#3105942, @aaron.ballman wrote:

One thing that surprises me is that the test changes are in Sema but the code changes are in CodeGen. I see that this test actually emits LLVM IR. Should it be moved (as an NFC change)?

Echo this question. Otherwise the new behavior looks good to me.

[ibookstein]

move attr-ifunc.c test to codegen

[ibookstein]

move test from Sema to CodeGen (trying to make arcanist send multiple
separate commits..?)

[ibookstein]

rebase fix after adding parent NFC commits

[MaskRay]

I assume that dancing with GlobalValue makes this difficult to implement in lib/Sema.

[ibookstein]

Yeah, it's real ugly from a CFE/LLVM separation POV. I also can't avoid seeing it as a duplication of the verifier's logic. But emitting diagnoses is better than asserting/crashing...
For the recursion, maybe an equivalent of getAliaseeObject on GlobalDecl-s could work? For the rest - not steeped in the code enough to say...

clang/lib/CodeGen/CodeGenModule.cpp

for (auto &I : GlobalValReplacements) {
 
     GV->replaceAllUsesWith(C);
     GV->eraseFromParent();
   }
 }
 
 // This is only used in aliases that we created and we know they have a
 // linear structure.
 static const llvm::GlobalValue *getAliasedGlobal(const llvm::GlobalValue *GV) {
-  llvm::SmallPtrSet<const llvm::GlobalValue *, 4> Visited;
-  for (;;) {
-    if (!GV || !Visited.insert(GV).second)
-      return nullptr;
-
-    const llvm::Constant *C;
-    if (auto *GA = dyn_cast<llvm::GlobalAlias>(GV))
-      C = GA->getAliasee();
-    else if (auto *GI = dyn_cast<llvm::GlobalIFunc>(GV))
-      C = GI->getResolver();
-    else
-      return GV;
 
-    GV = dyn_cast<llvm::GlobalValue>(C->stripPointerCasts());
-  }
+  const auto *AliaseeGV = dyn_cast<llvm::GlobalValue>(C->stripPointerCasts());
+  if (!AliaseeGV)
+    return nullptr;
+
+  const llvm::GlobalValue *FinalGV = AliaseeGV->getAliaseeObject();
+  if (FinalGV == GV)
+    return nullptr;
+
+  return FinalGV;
+
+static bool checkAliasedGlobal(bool IsIFunc,
+                               DiagnosticsEngine &Diags,
+                               SourceLocation Location,
+                               const llvm::GlobalValue *Alias,
+                               const llvm::GlobalValue *&GV) {
+  GV = getAliasedGlobal(Alias);
+  if (!GV) {
+    Diags.Report(Location, diag::err_cyclic_alias) << IsIFunc;
+    return false;
+  }
+
+  if (GV->isDeclaration()) {
+    Diags.Report(Location, diag::err_alias_to_undefined) << IsIFunc << IsIFunc;
+    return false;
+  }
+
+  if (IsIFunc) {
+    // Check resolver function type.
+    const auto *F = dyn_cast<llvm::Function>(GV);
+    if (!F) {
+      Diags.Report(Location, diag::err_alias_to_undefined)
+          << IsIFunc << IsIFunc;
+      return false;
+    }
+
+    llvm::FunctionType *FTy = F->getFunctionType();
+    if (!FTy->getReturnType()->isPointerTy()) {
+      Diags.Report(Location, diag::err_ifunc_resolver_return);
+      return false;
+    }
+  }
+
+  return true;
 }
 
 void CodeGenModule::checkAliases() {
   // Check if the constructed aliases are well formed. It is really unfortunate
   // that we have to do this in CodeGen, but we only construct mangled names
   // and aliases during codegen.
   bool Error = false;
   DiagnosticsEngine &Diags = getDiags();
   for (const GlobalDecl &GD : Aliases) {
     const auto *D = cast<ValueDecl>(GD.getDecl());
     SourceLocation Location;
     bool IsIFunc = D->hasAttr<IFuncAttr>();
     if (const Attr *A = D->getDefiningAttr())
       Location = A->getLocation();
     else
       llvm_unreachable("Not an alias or ifunc?");
+
     StringRef MangledName = getMangledName(GD);
     llvm::GlobalValue *Alias = GetGlobalValue(MangledName);
-    const llvm::GlobalValue *GV = getAliasedGlobal(Alias);
-    if (!GV) {
+    const llvm::GlobalValue *GV = nullptr;
+    if (!checkAliasedGlobal(IsIFunc, Diags, Location, Alias, GV)) {
       Error = true;
-      Diags.Report(Location, diag::err_cyclic_alias) << IsIFunc;
-    } else if (GV->isDeclaration()) {
-      Error = true;
-      Diags.Report(Location, diag::err_alias_to_undefined)
-          << IsIFunc << IsIFunc;
-    } else if (IsIFunc) {
-      // Check resolver function type.
-      llvm::FunctionType *FTy = dyn_cast<llvm::FunctionType>(
-          GV->getType()->getPointerElementType());
-      assert(FTy);
-      if (!FTy->getReturnType()->isPointerTy())
-        Diags.Report(Location, diag::err_ifunc_resolver_return);
+      continue;
     }
 
     llvm::Constant *Aliasee =
         IsIFunc ? cast<llvm::GlobalIFunc>(Alias)->getResolver()
                 : cast<llvm::GlobalAlias>(Alias)->getAliasee();
 
     llvm::GlobalValue *AliaseeGV;
     if (auto CE = dyn_cast<llvm::ConstantExpr>(Aliasee))
       AliaseeGV = cast<llvm::GlobalValue>(CE->getOperand(0));
     else
       AliaseeGV = cast<llvm::GlobalValue>(Aliasee);

clang/test/Sema/attr-ifunc.c

 // RUN: %clang_cc1 -triple x86_64-windows -fsyntax-only -verify %s
 // RUN: %clang_cc1 -triple x86_64-linux -fsyntax-only -verify -emit-llvm-only -DCHECK_ALIASES %s
 // RUN: %clang_cc1 -triple x86_64-linux -fsyntax-only -verify -emit-llvm-only %s
 
 #if defined(_WIN32)
 void foo() {}
 void bar() __attribute__((ifunc("foo")));
 //expected-warning@-1 {{unknown attribute 'ifunc' ignored}}
 
 #else
 #if defined(CHECK_ALIASES)
 void* f1_ifunc();
 void f1() __attribute__((ifunc("f1_ifunc")));
 //expected-error@-1 {{ifunc must point to a defined function}}
 
-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")));
 //expected-error@-1 {{ifunc definition is part of a cycle}}
 
 void* f3_a() __attribute__((ifunc("f3_b")));
 //expected-warning@-1 {{ifunc will always resolve to f3_c even if weak definition of f3_b is overridden}}
 void* f3_b() __attribute__((weak, alias("f3_c")));
 void* f3_c() { return 0; }
 
 void f4_ifunc() {}
 void f4() __attribute__((ifunc("f4_ifunc")));
 //expected-error@-1 {{ifunc resolver function must return a pointer}}
 
+int f5_resolver_gvar;
+void f5() __attribute__((ifunc("f5_resolver_gvar")));
+// expected-error@-1 {{ifunc must point to a defined function}}
+
+void *f6_resolver_resolver() { return 0; }
+void *f6_resolver() __attribute__((ifunc("f6_resolver_resolver")));
+void f6() __attribute__((ifunc("f6_resolver")));
+// expected-error@-1 {{ifunc must point to a defined function}}
+
 #else
 void f1a() __asm("f1");
 void f1a() {}
 //expected-note@-1 {{previous definition is here}}
 void f1() __attribute__((ifunc("f1_ifunc")));
 //expected-error@-1 {{definition with same mangled name 'f1' as another definition}}
 void* f1_ifunc() { return 0; }
 
 void* f6_ifunc(int i);
 void __attribute__((ifunc("f6_ifunc"))) f6() {}
 //expected-error@-1 {{definition 'f6' cannot also be an ifunc}}
 
 #endif
 #endif