[CodeGen] Set attributes on resolvers emitted after ifuncs

[MaskRay]

Visiting the ifunc calls GetOrCreateLLVMFunction with
NotForDefinition while visiting the resolver calls
GetOrCreateLLVMFunction with ForDefinition.

When an ifunc is emitted before its resolver, the ForDefinition call
does not call SetFunctionAttributes, because the function prematurely
returns due to (Entry->getValueType() == Ty) and
llvm::GlobalIFunc::getResolverFunctionType(DeclTy).

This leads to missing !kcfi_type with -fsanitize=kcfi.

extern void ifunc0(void) __attribute__ ((ifunc("resolver0")));
void *resolver0(void) { return 0; } // SetFunctionAttributes not called

extern void ifunc1(void) __attribute__ ((ifunc("resolver1")));
static void *resolver1(void) { return 0; } // SetFunctionAttributes not called

extern void ifunc2(void) __attribute__ ((ifunc("resolver2")));
static void *resolver2(void*) { return 0; }

Ensure SetFunctionAttributes is called by calling
GetOrCreateLLVMFunction with a dummy non-function type. Now that the
F->takeName(Entry) code path may be taken, the
DisableSanitizerInstrumentation code
(https://reviews.llvm.org/D150262) should be moved to checkAliases,
when the resolver function is finalized.

[llvmbot]

@llvm/pr-subscribers-clang-codegen

@llvm/pr-subscribers-clang

Author: Fangrui Song (MaskRay)

Changes

When an ifunc is emitted before its resolver, the resolver is created by
GetOrCreateLLVMFunction without ever calling SetFunctionAttributes.
The causes missing !kcfi_type with -fsanitize=kcfi.

extern void ifunc0(void) __attribute__ ((ifunc("resolver0")));
void *resolver0(void) { return 0; } // SetFunctionAttributes not called

extern void ifunc1(void) __attribute__ ((ifunc("resolver1")));
static void *resolver1(void) { return 0; } // SetFunctionAttributes not called

extern void ifunc2(void) __attribute__ ((ifunc("resolver2")));
static void *resolver2(void*) { return 0; }

This is because GetOrCreateLLVMFunction, while might also be called
with ForDefinition, does not continue after (Entry->getValueType() == Ty).

To ensure that SetFunctionAttributes is called, call
GetOrCreateLLVMFunction with a dummy non-function type. Now the
F->takeName(Entry) code path may be taken, the
DisableSanitizerInstrumentation code
(https://reviews.llvm.org/D150262) should be moved to checkAliases,
when the resolver function is finalized.

---

Full diff: https://github.com/llvm/llvm-project/pull/98832.diff

3 Files Affected:

• (modified) clang/lib/CodeGen/CodeGenModule.cpp (+12-7)
• (modified) clang/test/CodeGen/ifunc.c (+4-5)
• (modified) clang/test/CodeGen/kcfi.c (+10-10)

diff --git a/clang/lib/CodeGen/CodeGenModule.cpp b/clang/lib/CodeGen/CodeGenModule.cpp
index 08cfa694cfb81..250041d380cb1 100644
--- a/clang/lib/CodeGen/CodeGenModule.cpp
+++ b/clang/lib/CodeGen/CodeGenModule.cpp
@@ -721,6 +721,11 @@ void CodeGenModule::checkAliases() {
           cast<llvm::GlobalAlias>(Alias)->setAliasee(Aliasee);
       }
     }
+    // ifunc resolvers are usually implemented to run before sanitizer
+    // initialization. Disable instrumentation to prevent the ordering issue.
+    if (IsIFunc)
+      cast<llvm::Function>(Aliasee)->addFnAttr(
+          llvm::Attribute::DisableSanitizerInstrumentation);
   }
   if (!Error)
     return;
@@ -6106,11 +6111,14 @@ void CodeGenModule::emitIFuncDefinition(GlobalDecl GD) {
 
   Aliases.push_back(GD);
 
+  // The resolver might not be visited yet. Specify a dummy non-function type to
+  // indicate IsIncompleteFunction. Either the type is ignored (if the resolver
+  // was emitted or will be eagerly emitted) or the whole function will replaced
+  // (if the resolver will be inserted into DeferredDeclsToEmit).
+  llvm::Constant *Resolver = GetOrCreateLLVMFunction(
+      IFA->getResolver(), llvm::Type::getVoidTy(getLLVMContext()), {},
+      /*ForVTable=*/false);
   llvm::Type *DeclTy = getTypes().ConvertTypeForMem(D->getType());
-  llvm::Type *ResolverTy = llvm::GlobalIFunc::getResolverFunctionType(DeclTy);
-  llvm::Constant *Resolver =
-      GetOrCreateLLVMFunction(IFA->getResolver(), ResolverTy, {},
-                              /*ForVTable=*/false);
   llvm::GlobalIFunc *GIF =
       llvm::GlobalIFunc::create(DeclTy, 0, llvm::Function::ExternalLinkage,
                                 "", Resolver, &getModule());
@@ -6134,9 +6142,6 @@ void CodeGenModule::emitIFuncDefinition(GlobalDecl GD) {
     Entry->eraseFromParent();
   } else
     GIF->setName(MangledName);
-  if (auto *F = dyn_cast<llvm::Function>(Resolver)) {
-    F->addFnAttr(llvm::Attribute::DisableSanitizerInstrumentation);
-  }
   SetCommonAttributes(GD, GIF);
 }
 
diff --git a/clang/test/CodeGen/ifunc.c b/clang/test/CodeGen/ifunc.c
index b049739daf2aa..58a00ada687cb 100644
--- a/clang/test/CodeGen/ifunc.c
+++ b/clang/test/CodeGen/ifunc.c
@@ -58,12 +58,11 @@ extern void hoo(int) __attribute__ ((ifunc("hoo_ifunc")));
 // CHECK: call i32 @foo(i32
 // CHECK: call void @goo()
 
-// SAN: define internal nonnull {{(noundef )?}}ptr @foo_ifunc() #[[#FOO_IFUNC:]] {
-
 // SAN: define {{(dso_local )?}}noalias {{(noundef )?}}ptr @goo_ifunc() #[[#GOO_IFUNC:]] {
 
-// SAN: define {{(dso_local )?}}noalias {{(noundef )?}}ptr @hoo_ifunc() #[[#HOO_IFUNC:]] {
+// SAN: define {{(dso_local )?}}noalias {{(noundef )?}}ptr @hoo_ifunc() #[[#GOO_IFUNC]] {
+
+// SAN: define internal {{(noundef )?}}nonnull ptr @foo_ifunc() #[[#FOO_IFUNC:]] {
 
-// SAN-DAG: attributes #[[#FOO_IFUNC]] = {{{.*}} disable_sanitizer_instrumentation {{.*}}
 // SAN-DAG: attributes #[[#GOO_IFUNC]] = {{{.*}} disable_sanitizer_instrumentation {{.*}}
-// SAN-DAG: attributes #[[#HOO_IFUNC]] = {{{.*}} disable_sanitizer_instrumentation {{.*}}
+// SAN-DAG: attributes #[[#FOO_IFUNC]] = {{{.*}} disable_sanitizer_instrumentation {{.*}}
diff --git a/clang/test/CodeGen/kcfi.c b/clang/test/CodeGen/kcfi.c
index c29429f644ba1..622843cedba50 100644
--- a/clang/test/CodeGen/kcfi.c
+++ b/clang/test/CodeGen/kcfi.c
@@ -33,16 +33,6 @@ int call(fn_t f) {
   return f();
 }
 
-#ifndef __cplusplus
-// C: define internal ptr @resolver1() #[[#]] {
-int ifunc1(int) __attribute__((ifunc("resolver1")));
-static void *resolver1(void) { return 0; }
-
-// C: define internal ptr @resolver2() #[[#]] {
-static void *resolver2(void) { return 0; }
-long ifunc2(long) __attribute__((ifunc("resolver2")));
-#endif
-
 // CHECK-DAG: define internal{{.*}} i32 @{{f3|_ZL2f3v}}(){{.*}} !kcfi_type ![[#TYPE]]
 static int f3(void) { return 1; }
 
@@ -58,6 +48,16 @@ static int f5(void) { return 2; }
 // CHECK-DAG: declare !kcfi_type ![[#TYPE]]{{.*}} i32 @{{f6|_Z2f6v}}()
 extern int f6(void);
 
+#ifndef __cplusplus
+// C: define internal ptr @resolver1() #[[#]] !kcfi_type ![[#]] {
+int ifunc1(int) __attribute__((ifunc("resolver1")));
+static void *resolver1(void) { return 0; }
+
+// C: define internal ptr @resolver2() #[[#]] !kcfi_type ![[#]] {
+static void *resolver2(void) { return 0; }
+long ifunc2(long) __attribute__((ifunc("resolver2")));
+#endif
+
 int test(void) {
   return call(f1) +
          __call((fn_t)f2) +

[efriedma-quic]

Not what I was expecting, but this works, sure. LGTM.

[efriedma-quic]

Suggested change

	// not be emitted).
	// not been emitted).