Fix behavior of ifuncs with 'used' extern "C" static functions

[erichkeane]

Summary

We expect that extern "C" static functions to be usable in things like
inline assembly, as well as ifuncs:
See the bug report here: https://github.com/llvm/llvm-project/issues/54549

However, we were diagnosing this as 'not defined', because the
ifunc's attempt to look up its resolver would generate a declared IR
function.

Additionally, as background, the way we allow these static extern "C"
functions to work in inline assembly is by making an alias with the C
mangling in MOST situations to the version we emit with
internal-linkage/mangling.

The problem here was multi-fold: First- We generated the alias after the
ifunc was checked, so the function by that name didn't exist yet.
Second, the ifunc's generation caused a symbol to exist under the name
of the alias already (the declared function above), which suppressed the
alias generation.

This patch fixes all of this by moving the checking of ifuncs/CFE aliases
until AFTER we have generated the extern-C alias. Then, it does a
'fixup' around the GlobalIFunc to make sure we correct the reference.

[aaron.ballman]

CodeGen is not my wheelhouse, so I've added @rjmccall in case he spots something I've missed. This seems reasonable to me though.

clang/lib/CodeGen/CodeGenModule.cpp

6351

6458

[erichkeane]

Do the two comment fixups aaron suggested.

[tahonermann]

clang/lib/CodeGen/CodeGenModule.cpp

6352

An example of why the names might not match would be helpful here.

6356–6358

This comment doesn't state what the challenge is when there is more than one use. Is the concern that there may be multiple ifuncs that have the same associated resolver? What might cause that to happen? (presumably multiple functions declared with the ifunc attribute that name the same resolver).

A test that exercises this case seems appropriate.

clang/test/SemaCXX/externc-ifunc-resolver.cpp

9

There is considerable subtlety between these two tests and, unfortunately, the diagnostic isn't particularly helpful.

If I'm following correctly, the other test passes because an unmangled name is specified for the ifunc attribute and that is resolved against the emitted alias.

In this test, the same code is present as the other test, but with an additional declaration that doesn't seem relevant; one would not expect lookup for "resolve_foo" in the ifunc attribute to include the NS namespace. Is the error generated because two aliases for "resolve_foo" are emitted? If so, that seems like something that should be diagnosed somewhere. Or perhaps an alias should not be emitted for used functions defined at namespace scope.

[erichkeane]

clang/lib/CodeGen/CodeGenModule.cpp

6356–6358

I think the 'multiple ifuncs' isn't actually an issue, it is more that "I have no idea what else could use this/create this". Thinking now, the multiple 'ifuncs' seems completely doable now. I'll update the patch to handle multiples here.

clang/test/SemaCXX/externc-ifunc-resolver.cpp

9

If I'm following correctly, the other test passes because an unmangled name is specified for the ifunc attribute and that is resolved against the emitted alias.

Yes, this is correct.

There is considerable subtlety between these two tests and, unfortunately, the diagnostic isn't particularly helpful.

Yeah, this is unfortunate. However, when it is diagnosed causes some problems in that we don't have the real name of the declaration available anymore. I'm not intending to improve the quality of the diagnostic here though.

Is the error generated because two aliases for "resolve_foo" are emitted?

It is because two aliases WOULD be emitted, so we choose not to.

If so, that seems like something that should be diagnosed somewhere.

Maybe? Probably? I don't see the 'used' alias functionality as a part of this patch, I'm trying to keep that behavior as much as it possibly exists now.

Or perhaps an alias should not be emitted for used functions defined at namespace scope.

This we cannot do without breaking code that depends on it already.

[erichkeane]

Going through @tahonermann 's comments to attempt to allow multiple ifunc references, I ended up down the rabbit hole.

Turns out there are ways for there to be a ConstantExpr bitcast in the way that needs to be corrected (the 'first' one is never created this way, as the type is derived directly from the ifunc's type). I did a significant refactor to attempt to make this as clean as possible.

Note that the diagnostics for ifuncs are still bad, but I make no attempts to fix that here.

[erichkeane]

Ping!

[aaron.ballman]

FWIW, I'm seeing a precommit CI failure on Windows:

Failed Tests (1):
  Clang :: SemaCXX/externc-ifunc-resolver.cpp

May as well also fix up the clang-format issues in the review.

clang/lib/CodeGen/CodeGenModule.cpp

6352

Name appears to be entirely unused?

[erichkeane]

Applied clang-format, removed 'Name' as Aaron found is no longer used, fixed the Sema test to not run on Windows triple, where ifunc is not available

[erichkeane]

fix a typo in the 'triple' .

[tahonermann]

clang/lib/CodeGen/CodeGenModule.cpp

6381–6382

This comment can only be understood in the context of this commit with the FIXME comment noted in the relevant test case. I suggest:

// This user is one we don't know how to handle, so fail redirection. This
// will result in an ifunc retaining a resolver name that will ultimately fail
// to be resolved to a defined function.

6424–6437

I suggest moving the declaration of ExistingElem after the break on !Val given that the former is not relevant if Val is null.

I think the !Val check is worth a comment here. I suggest:

// If Val is null, that implies that there were multiple declarations that each
// had a claim to the unmangled name. In this case, generation of the alias
// is suppressed. See CodeGenModule::MaybeHandleStaticInExternC().

The lack of checking for an existing element with the desired alias name was a pre-existing oversight, yes?

6430–6431

This comment seems inconsistent with the code. If there are no ifuncs involved, then why call CheckAndReplaceExternCIFuncs()?

clang/lib/CodeGen/CodeGenModule.h

1579–1581

Grammar is off in the last sentence.

The comment doesn't really explain this function's purpose. I suggest:

/// Helper function for EmitStaticExternCAliases() to redirect ifuncs that have a resolver
/// name that matches 'Elem' to instead resolve to the name of 'CppFunc'. This
/// redirection is necessary in cases where 'Elem' has a name that will be emitted as
/// an alias of the name bound to 'CppFunc'; ifuncs may not reference aliases. Redirection
/// is only performed if 'Elem' is only used by ifuncs in which case, 'Elem' is destroyed..
/// 'true' is returned If redirection is successful, and 'false' is returned otherwise.

clang/test/SemaCXX/externc-ifunc-resolver.cpp

10–14

Thanks for adding this comment; it is helpful.

[erichkeane]

Fix all the things @tahonermann found.

clang/lib/CodeGen/CodeGenModule.cpp

6424–6437

SGTM, thanks!

6430–6431

Woops! Artifact of a previous implementation of this. Replaced the comment.

[tahonermann]

clang/lib/CodeGen/CodeGenModule.cpp

6426

You retyped my suggested comment instead of copy/paste? Or is the "hte" just intended to test my attention to detail? 😜

clang/lib/CodeGen/CodeGenModule.h

1579–1581

The parent comment is marked as done, but no change appears to have been applied.

[erichkeane]

clang/lib/CodeGen/CodeGenModule.cpp

6426

Ugh, even worse. I read your suggestion and was like, "I don't like that, it reads funny, let me try my own version". THEN, I typed in MY version of it just to find out I'd basically typed the exact wording(I put the 'See...' line earlier), so I just copy/pasted the change. For the worse... yay me.

clang/lib/CodeGen/CodeGenModule.h

1579–1581

Woops, forgot to git-add the file! Done again!

[tahonermann]

Looks good to me! I left one final comment about a double period that was my fault.

clang/lib/CodeGen/CodeGenModule.h

1584

Double period; that was my badness that you dutifully copy pasted :)

clang/lib/CodeGen/CodeGenModule.h

private:
   void emitAtAvailableLinkGuard();
 
   /// Emit the llvm.used and llvm.compiler.used metadata.
   void emitLLVMUsed();
 
   /// Emit the link options introduced by imported modules.
   void EmitModuleLinkOptions();
 
+  /// Helper function for EmitStaticExternCAliases() to redirect ifuncs that
+  /// have a resolver name that matches 'Elem' to instead resolve to the name of
+  /// 'CppFunc'. This redirection is necessary in cases where 'Elem' has a name
+  /// that will be emitted as an alias of the name bound to 'CppFunc'; ifuncs
+  /// may not reference aliases. Redirection is only performed if 'Elem' is only
+  /// used by ifuncs in which case, 'Elem' is destroyed. 'true' is returned if
+  /// redirection is successful, and 'false' is returned otherwise.
+  bool CheckAndReplaceExternCIFuncs(llvm::GlobalValue *Elem,
+                                    llvm::GlobalValue *CppFunc);
+
   /// Emit aliases for internal-linkage declarations inside "C" language
   /// linkage specifications, giving them the "expected" name where possible.
   void EmitStaticExternCAliases();
 
   void EmitDeclMetadata();
 
   /// Emit the Clang version as llvm.ident metadata.
   void EmitVersionIdentMetadata();

clang/lib/CodeGen/CodeGenModule.cpp

static void setVisibilityFromDLLStorageClass(const clang::LangOptions &LO,
   }
 }
 
 void CodeGenModule::Release() {
   EmitDeferred();
   EmitVTablesOpportunistically();
   applyGlobalValReplacements();
   applyReplacements();
-  checkAliases();
   emitMultiVersionFunctions();
   EmitCXXGlobalInitFunc();
   EmitCXXGlobalCleanUpFunc();
   registerGlobalDtorsWithAtExit();
   EmitCXXThreadLocalInitFunc();
   if (ObjCRuntime)
     if (llvm::Function *ObjCInitFunction = ObjCRuntime->ModuleInitFunction())
       AddGlobalCtor(ObjCInitFunction);
getModule().setProfileSummary(
         llvm::ProfileSummary::PSK_Instr);
     if (PGOStats.hasDiagnostics())
       PGOStats.reportDiagnostics(getDiags(), getCodeGenOpts().MainFileName);
   }
   EmitCtorList(GlobalCtors, "llvm.global_ctors");
   EmitCtorList(GlobalDtors, "llvm.global_dtors");
   EmitGlobalAnnotations();
   EmitStaticExternCAliases();
+  checkAliases();
   EmitDeferredUnusedCoverageMappings();
   CodeGenPGO(*this).setValueProfilingFlag(getModule());
   if (CoverageMapping)
     CoverageMapping->emit();
   if (CodeGenOpts.SanitizeCfiCrossDso) {
     CodeGenFunction(*this).EmitCfiCheckFail();
     CodeGenFunction(*this).EmitCfiCheckStub();
   }
static void EmitGlobalDeclMetadata(CodeGenModule &CGM,
 
   // TODO: should we report variant information for ctors/dtors?
   llvm::Metadata *Ops[] = {llvm::ConstantAsMetadata::get(Addr),
                            llvm::ConstantAsMetadata::get(GetPointerConstant(
                                CGM.getLLVMContext(), D.getDecl()))};
   GlobalMetadata->addOperand(llvm::MDNode::get(CGM.getLLVMContext(), Ops));
 }
 
+bool CodeGenModule::CheckAndReplaceExternCIFuncs(llvm::GlobalValue *Elem,
+                                                 llvm::GlobalValue *CppFunc) {
+  // Store the list of ifuncs we need to replace uses in.
+  llvm::SmallVector<llvm::GlobalIFunc *> IFuncs;
+  // List of ConstantExprs that we should be able to delete when we're done
+  // here.
+  llvm::SmallVector<llvm::ConstantExpr *> CEs;
+
+  // First make sure that all users of this are ifuncs (or ifuncs via a
+  // bitcast), and collect the list of ifuncs and CEs so we can work on them
+  // later.
+  for (llvm::User *User : Elem->users()) {
+    // Users can either be a bitcast ConstExpr that is used by the ifuncs, OR an
+    // ifunc directly. In any other case, just give up, as we don't know what we
+    // could break by changing those.
+    if (auto *ConstExpr = dyn_cast<llvm::ConstantExpr>(User)) {
+      if (ConstExpr->getOpcode() != llvm::Instruction::BitCast)
+        return false;
+
+      for (llvm::User *CEUser : ConstExpr->users()) {
+        if (auto *IFunc = dyn_cast<llvm::GlobalIFunc>(CEUser)) {
+          IFuncs.push_back(IFunc);
+        } else {
+          return false;
+        }
+      }
+      CEs.push_back(ConstExpr);
+    } else if (auto *IFunc = dyn_cast<llvm::GlobalIFunc>(User)) {
+      IFuncs.push_back(IFunc);
+    } else {
+      // This user is one we don't know how to handle, so fail redirection. This
+      // will result in an ifunc retaining a resolver name that will ultimately
+      // fail to be resolved to a defined function.
+      return false;
+    }
+  }
+
+  // Now we know this is a valid case where we can do this alias replacement, we
+  // need to remove all of the references to Elem (and the bitcasts!) so we can
+  // delete it.
+  for (llvm::GlobalIFunc *IFunc : IFuncs)
+    IFunc->setResolver(nullptr);
+  for (llvm::ConstantExpr *ConstExpr : CEs)
+    ConstExpr->destroyConstant();
+
+  // We should now be out of uses for the 'old' version of this function, so we
+  // can erase it as well.
+  Elem->eraseFromParent();
+
+  for (llvm::GlobalIFunc *IFunc : IFuncs) {
+    // The type of the resolver is always just a function-type that returns the
+    // type of the IFunc, so create that here. If the type of the actual
+    // resolver doesn't match, it just gets bitcast to the right thing.
+    auto *ResolverTy =
+        llvm::FunctionType::get(IFunc->getType(), /*isVarArg*/ false);
+    llvm::Constant *Resolver = GetOrCreateLLVMFunction(
+        CppFunc->getName(), ResolverTy, {}, /*ForVTable*/ false);
+    IFunc->setResolver(Resolver);
+  }
+  return true;
+}
+
 /// For each function which is declared within an extern "C" region and marked
 /// as 'used', but has internal linkage, create an alias from the unmangled
 /// name to the mangled name if possible. People expect to be able to refer
 /// to such functions with an unmangled name from inline assembly within the
 /// same translation unit.
 void CodeGenModule::EmitStaticExternCAliases() {
   if (!getTargetCodeGenInfo().shouldEmitStaticExternCAliases())
     return;
   for (auto &I : StaticExternCValues) {
     IdentifierInfo *Name = I.first;
     llvm::GlobalValue *Val = I.second;
-    if (Val && !getModule().getNamedValue(Name->getName()))
+
+    // If Val is null, that implies there were multiple declarations that each
+    // had a claim to the unmangled name. In this case, generation of the alias
+    // is suppressed. See CodeGenModule::MaybeHandleStaticInExterC.
+    if (!Val)
+      break;
+
+    llvm::GlobalValue *ExistingElem =
+        getModule().getNamedValue(Name->getName());
+
+    // If there is either not something already by this name, or we were able to
+    // replace all uses from IFuncs, create the alias.
+    if (!ExistingElem || CheckAndReplaceExternCIFuncs(ExistingElem, Val))
       addCompilerUsedGlobal(llvm::GlobalAlias::create(Name->getName(), Val));
   }
 }
 
 bool CodeGenModule::lookupRepresentativeDecl(StringRef MangledName,
                                              GlobalDecl &Result) const {
   auto Res = Manglings.find(MangledName);
   if (Res == Manglings.end())
     return false;
   Result = Res->getValue();
   return true;
 }
 
 /// Emits metadata nodes associating all the global values in the
 /// current module with the Decls they came from.  This is useful for
 /// projects using IR gen as a subroutine.
 ///
 /// Since there's currently no way to associate an MDNode directly
 /// with an llvm::GlobalValue, we create a global named metadata
 /// with the name 'clang.global.decl.ptrs'.
 void CodeGenModule::EmitDeclMetadata() {
   llvm::NamedMDNode *GlobalMetadata = nullptr;
 
   for (auto &I : MangledDeclNames) {
     llvm::GlobalValue *Addr = getModule().getNamedValue(I.second);
     // Some mangled names don't necessarily have an associated GlobalValue
     // in this module, e.g. if we mangled it for DebugInfo.
     if (Addr)
       EmitGlobalDeclMetadata(*this, GlobalMetadata, I.first, Addr);
   }

clang/test/CodeGenCXX/externc-ifunc-resolver.cpp

+// RUN: %clang_cc1 -triple x86_64-linux-gnu -emit-llvm -o - %s | FileCheck %s
+
+extern "C" {
+__attribute__((used)) static void *resolve_foo() { return 0; }
+__attribute__((ifunc("resolve_foo"))) char *foo();
+__attribute__((ifunc("resolve_foo"))) void foo2(int);
+__attribute__((ifunc("resolve_foo"))) char foo3(float);
+__attribute__((ifunc("resolve_foo"))) char foo4(float);
+}
+
+// CHECK: @resolve_foo = internal alias i8* (), i8* ()* @_ZL11resolve_foov
+// CHECK: @foo = ifunc i8* (), bitcast (i8* ()* @_ZL11resolve_foov to i8* ()* ()*)
+// CHECK: @foo2 = ifunc void (i32), bitcast (i8* ()* @_ZL11resolve_foov to void (i32)* ()*)
+// CHECK: @foo3 = ifunc i8 (float), bitcast (i8* ()* @_ZL11resolve_foov to i8 (float)* ()*)
+// CHECK: @foo4 = ifunc i8 (float), bitcast (i8* ()* @_ZL11resolve_foov to i8 (float)* ()*)
+// CHECK: define internal noundef i8* @_ZL11resolve_foov()

clang/test/SemaCXX/externc-ifunc-resolver.cpp

+// RUN: %clang_cc1 -emit-llvm-only -triple x86_64-linux-gnu -verify %s
+
+extern "C" {
+__attribute__((used)) static void *resolve_foo() { return 0; }
+namespace NS {
+__attribute__((used)) static void *resolve_foo() { return 0; }
+} // namespace NS
+
+// FIXME: This diagnostic is pretty confusing, the issue is that the existence
+// of the two functions suppresses the 'alias' creation, and thus the ifunc
+// resolution via the alias as well. In the future we should probably find
+// some way to improve this diagnostic (likely by diagnosing when we decide
+// this case suppresses alias creation).
+__attribute__((ifunc("resolve_foo"))) void foo(); // expected-error{{ifunc must point to a defined function}}
+}
+