[llvm] [llvm][CFI] Do not canonicalize COFF functions in a comdat (PR #139962)

[via llvm-commits]

https://github.com/PiJoules updated https://github.com/llvm/llvm-project/pull/139962

>From 60886e801e30cb31361a4ce64849d2dec87243d0 Mon Sep 17 00:00:00 2001
From: Leonard Chan <leonardchan at google.com>
Date: Wed, 14 May 2025 13:41:23 -0700
Subject: [PATCH] [llvm][CFI] Do not canonicalize COFF functions in a comdat

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 just change the comdat name since symbol
resolution has already happened before LTO.
---
 llvm/lib/Transforms/IPO/LowerTypeTests.cpp    | 24 ++++++++++++++-
 .../LowerTypeTests/cfi-coff-comdat-rename.ll  | 29 +++++++++++++++++++
 2 files changed, 52 insertions(+), 1 deletion(-)
 create mode 100644 llvm/test/Transforms/LowerTypeTests/cfi-coff-comdat-rename.ll

diff --git a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
index d855647095550..f01067172379c 100644
--- a/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
+++ b/llvm/lib/Transforms/IPO/LowerTypeTests.cpp
@@ -246,6 +246,18 @@ void ByteArrayBuilder::allocate(const std::set<uint64_t> &Bits,
 bool lowertypetests::isJumpTableCanonical(Function *F) {
   if (F->isDeclarationForLinker())
     return false;
+
+  // Do not canonicalize a comdat'd COFF function because this could end up
+  // renaming the comdat key function without renaming the comdat key. We cannot
+  // rename the key in this LTO unit because other TUs may reference the
+  // original key name. To prevent this, just ignore canonicalization for
+  // comdat'd COFF functions.
+  // if (F->hasComdat()) {
+  //  Triple TargetTriple(F->getParent()->getTargetTriple());
+  //  if (TargetTriple.isOSBinFormatCOFF())
+  //    return false;
+  //}
+
   auto *CI = mdconst::extract_or_null<ConstantInt>(
       F->getParent()->getModuleFlag("CFI Canonical Jump Tables"));
   if (!CI || !CI->isZero())
@@ -1715,8 +1727,18 @@ 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 *NewComdat = M.getOrInsertComdat(F->getName());
+          F->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..5fcbf911b02ce
--- /dev/null
+++ b/llvm/test/Transforms/LowerTypeTests/cfi-coff-comdat-rename.ll
@@ -0,0 +1,29 @@
+; 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 ptr @f3
+
+$f3 = comdat any
+
+; CHECK: @f3.cfi() comdat !type !0
+define void @f3() comdat !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"}