Use internal linkage for __NoopCoro_ResumeDestroy

[dpaoliello]

__NoopCoro_ResumeDestroy currently has private linkage, which causes issues for Arm64EC. The Arm64EC lowering is trying to mangle and add thunks for __NoopCoro_ResumeDestroy, since it sees that it's address is taken (and, therefore, might be called from x64 code via a function pointer). MSVC's linker requires that the function be placed in COMDAT (LNK1361: non COMDAT symbol '.L#__NoopCoro_ResumeDestroy' in hybrid binary) which trips an assert in the verifier (comdat global value has private linkage) and the subsequent linking step fails since the private symbol isn't in the symbol table.

Since there is no reason to use private linkage for __NoopCoro_ResumeDestroy and other coro related functions have also been switched to internal linkage to improve debugging, this change switches to using internal linkage.

Fixes #158341

[llvmbot]

@llvm/pr-subscribers-llvm-transforms

@llvm/pr-subscribers-coroutines

Author: Daniel Paoliello (dpaoliello)

Changes

__NoopCoro_ResumeDestroy currently has private linkage, which causes issues for Arm64EC. The Arm64EC lowering is trying to mangle and add thunks for __NoopCoro_ResumeDestroy, since it sees that it's address is taken (and, therefore, might be called from x64 code via a function pointer). MSVC's linker requires that the function be placed in COMDAT (LNK1361: non COMDAT symbol '.L#__NoopCoro_ResumeDestroy' in hybrid binary) which trips an assert in the verifier (comdat global value has private linkage) and the subsequent linking step fails since the private symbol isn't in the symbol table.

Since there is no reason to use private linkage for __NoopCoro_ResumeDestroy and other coro related functions have also been switched to internal linkage to improve debugging, this change switches to using internal linkage.

Fixes #158341

---

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

3 Files Affected:

• (modified) llvm/lib/Transforms/Coroutines/CoroEarly.cpp (+1-1)
• (modified) llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll (+1-1)
• (modified) llvm/test/Transforms/Coroutines/coro-noop.ll (+1-1)

diff --git a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
index 6561b1cd4ade1..471b9ebb1626d 100644
--- a/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
+++ b/llvm/lib/Transforms/Coroutines/CoroEarly.cpp
@@ -132,7 +132,7 @@ void Lowerer::lowerCoroNoop(IntrinsicInst *II) {
 
     // Create a Noop function that does nothing.
     Function *NoopFn = Function::createWithDefaultAttr(
-        FnTy, GlobalValue::LinkageTypes::PrivateLinkage,
+        FnTy, GlobalValue::LinkageTypes::InternalLinkage,
         M.getDataLayout().getProgramAddressSpace(), "__NoopCoro_ResumeDestroy",
         &M);
     NoopFn->setCallingConv(CallingConv::Fast);
diff --git a/llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll b/llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll
index 4f302a6acc649..41a01bea48369 100644
--- a/llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll
+++ b/llvm/test/Transforms/Coroutines/coro-noop-pacbti.ll
@@ -1,7 +1,7 @@
 
 ; RUN: opt < %s -S -passes=coro-early | FileCheck %s
 
-; CHECK:      define private fastcc void @__NoopCoro_ResumeDestroy(ptr %0) #1 {
+; CHECK:      define internal fastcc void @__NoopCoro_ResumeDestroy(ptr %0) #1 {
 ; CHECK-NEXT: entry:
 ; CHECK-NEXT:   ret void
 ; CHECK-NEXT: }
diff --git a/llvm/test/Transforms/Coroutines/coro-noop.ll b/llvm/test/Transforms/Coroutines/coro-noop.ll
index 1e4f19a2ef66e..7156835e5b2d5 100644
--- a/llvm/test/Transforms/Coroutines/coro-noop.ll
+++ b/llvm/test/Transforms/Coroutines/coro-noop.ll
@@ -26,7 +26,7 @@ declare ptr @llvm.coro.noop()
 !4 = !{i32 2, !"Debug Info Version", i32 3}
 
 
-; CHECK: define private fastcc void @__NoopCoro_ResumeDestroy(ptr %0) !dbg ![[RESUME:[0-9]+]] {
+; CHECK: define internal fastcc void @__NoopCoro_ResumeDestroy(ptr %0) !dbg ![[RESUME:[0-9]+]] {
 ; CHECK-NEXT: entry
 ; CHECK-NEXT:    ret void
 

[efriedma-quic]

This seems fine, but we should probably also (in a separate patch) add a check to Arm64EC call lowering to make sure we aren't adding comdats to functions marked private. Not sure what we do at that point... we could mess with the linkage, or just error.

[rnk]

I think it's pretty reasonable to upgrade private linkage to internal in the ARM64EC pass that adds the comdats. Perhaps some might see that as information leakage or object file size regression, but setting aside static archives, object files are not typically the final distributed binary format, and even internal names are removed from most linked PE images.