[llvm] 286ab11 - Reapply "[llvm][CFI] Do not canonicalize COFF functions in a comdat (#139962)"

[Leonard Chan via llvm-commits]

Author: Leonard Chan
Date: 2025-05-16T15:38:22-07:00
New Revision: 286ab11dc65de8175e2bce69fd7e36acd3be7ce0

URL: https://github.com/llvm/llvm-project/commit/286ab11dc65de8175e2bce69fd7e36acd3be7ce0
DIFF: https://github.com/llvm/llvm-project/commit/286ab11dc65de8175e2bce69fd7e36acd3be7ce0.diff

LOG: Reapply "[llvm][CFI] Do not canonicalize COFF functions in a comdat (#139962)"

This reapplies 33684ac9be4892579f63a8e2b67080419426cf98 with appropriate
requires on tests.

COFF requires that a function exists with the same name as a comdat. Not
having this key function results in `LLVM ERROR: Associative COMDAT
symbol '...' does not exist.` CFI by default will attempt to
canonicalize a function by appending `.cfi` to its name which allows
external references to refer to the new canonical alias, but it does not
change the comdat name. We can change the comdat name since symbol and
comdat resolution occurs before LTO, so we already know which symbols
are prevailing.

Added: 
    llvm/test/Transforms/LowerTypeTests/cfi-coff-comdat-rename.ll

Modified: 
    llvm/lib/Transforms/IPO/LowerTypeTests.cpp

Removed: 
    


################################################################################
diff  --git a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
index ebabece067db2..91e84a95b30b3 100644
--- a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
+++ b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
@@ -1711,8 +1711,22 @@ void LowerTypeTestsModule::buildBitSetsFromFunctionsNative(
           F->getValueType(), 0, F->getLinkage(), "", CombinedGlobalElemPtr, &M);
       FAlias->setVisibility(F->getVisibility());
       FAlias->takeName(F);
-      if (FAlias->hasName())
+      if (FAlias->hasName()) {
         F->setName(FAlias->getName() + ".cfi");
+        // For COFF we should also rename the comdat if this function also
+        // happens to be the key function. Even if the comdat name changes, this
+        // should still be fine since comdat and symbol resolution happens
+        // before LTO, so all symbols which would prevail have been selected.
+        if (F->hasComdat() && ObjectFormat == Triple::COFF &&
+            F->getComdat()->getName() == FAlias->getName()) {
+          Comdat *OldComdat = F->getComdat();
+          Comdat *NewComdat = M.getOrInsertComdat(F->getName());
+          for (GlobalObject &GO : M.global_objects()) {
+            if (GO.getComdat() == OldComdat)
+              GO.setComdat(NewComdat);
+          }
+        }
+      }
       replaceCfiUses(F, FAlias, IsJumpTableCanonical);
       if (!F->hasLocalLinkage())
         F->setVisibility(GlobalVariable::HiddenVisibility);

diff  --git a/llvm/test/Transforms/LowerTypeTests/cfi-coff-comdat-rename.ll b/llvm/test/Transforms/LowerTypeTests/cfi-coff-comdat-rename.ll
new file mode 100644
index 0000000000000..7dda7f6df10c3
--- /dev/null
+++ b/llvm/test/Transforms/LowerTypeTests/cfi-coff-comdat-rename.ll
@@ -0,0 +1,36 @@
+; REQUIRES: x86-registered-target
+; RUN: opt -S -passes=lowertypetests %s | FileCheck %s
+
+;; This is a check to assert we don't crash with:
+;;
+;;   LLVM ERROR: Associative COMDAT symbol '...' does not exist.
+;;
+;; So this just needs to exit normally.
+; RUN: opt -S -passes=lowertypetests %s | llc -asm-verbose=false
+
+target datalayout = "e-p:64:64"
+target triple = "x86_64-pc-windows-msvc"
+
+ at a = global [2 x ptr] [ptr @f1, ptr @f2]
+
+; CHECK: $f1.cfi = comdat any
+$f1 = comdat any
+
+; CHECK: @f1.cfi() comdat !type !0
+define void @f1() comdat !type !0 {
+  ret void
+}
+
+; CHECK: @f2.cfi() comdat($f1.cfi) !type !0
+define void @f2() comdat($f1) !type !0 {
+  ret void
+}
+
+declare i1 @llvm.type.test(ptr %ptr, metadata %bitset) nounwind readnone
+
+define i1 @foo(ptr %p) {
+  %x = call i1 @llvm.type.test(ptr %p, metadata !"typeid1")
+  ret i1 %x
+}
+
+!0 = !{i32 0, !"typeid1"}