[compiler-rt] [compiler-rt] Remove duplicate MS names for chkstk symbols (PR #80450)

Fri Feb 2 07:40:54 PST 2024

https://github.com/mstorsjo created https://github.com/llvm/llvm-project/pull/80450

Prior to 885d7b759b5c166c07c07f4c58c6e0ba110fb0c2, the builtins library contained two chkstk implementations for each of i386 and x86_64, one that was used in mingw environments, and one unused (with a symbol name not matching anything that is used anywhere). Some of the functions additionally had other, also unused, aliases.

After cleaning this up in 885d7b759b5c166c07c07f4c58c6e0ba110fb0c2, the unused symbol names were removed.

At the same time, symbol aliases were added for the names as they are used by MSVC; the functions are functionally equivalent, but have different names between mingw and MSVC style environments.

By adding a symbol alias (so that one object file contains two different symbols for the same function), users can run into problems with duplicate definitions, if they themselves define one of the symbols (for various reasons), but need to link in the other one.

This happens for Wine, which provides their own definition of "__chkstk", but when built in mingw mode does need compiler-rt to provide the mingw specific symbol names; see
https://github.com/mstorsjo/llvm-mingw/issues/397.

To avoid the issue, remove the extra MS style names. They weren't entirely usable as such for MSVC style environments anyway, as compiler-rt builtins don't build these object files at all, when built in MSVC mode; thus, the effort to provide them for MSVC style environments in 885d7b759b5c166c07c07f4c58c6e0ba110fb0c2 was a half-hearted step towards that.

If we really do want to provide those functions (as an alternative to the ones provided by MSVC itself), we should do it in a separate object file (even if the function implementation is the same), so that users who have a definition of one of them but need a definition of the other, won't have conflicts.

Additionally, if we do want to provide them for MSVC, those files actually should be built when building the builtins in MSVC mode as well (see compiler-rt/lib/builtins/CMakeLists.txt).

If we do that, there's a risk that an MSVC style build ends up linking in and preferring our implementation over the one provided by MSVC, which would be suboptimal. Our implementation always probes the requested amount of stack, while the MSVC one checks the amount of allocated stack and only probes as much as really is needed.

In short - this reverts the situation to what it was in the 17.x release series (except for unused functions that have been removed).

>From 06394294fe76e3f26c816122aaaa7c3c223e2369 Mon Sep 17 00:00:00 2001
From: =?UTF-8?q?Martin=20Storsj=C3=B6?= <martin at martin.st>
Date: Fri, 2 Feb 2024 17:19:14 +0200
Subject: [PATCH] [compiler-rt] Remove duplicate MS names for chkstk symbols

Prior to 885d7b759b5c166c07c07f4c58c6e0ba110fb0c2, the builtins
library contained two chkstk implementations for each of i386 and
x86_64, one that was used in mingw environments, and one unused
(with a symbol name not matching anything that is used anywhere).
Some of the functions additionally had other, also unused, aliases.

After cleaning this up in 885d7b759b5c166c07c07f4c58c6e0ba110fb0c2,
the unused symbol names were removed.

At the same time, symbol aliases were added for the names as they
are used by MSVC; the functions are functionally equivalent, but
have different names between mingw and MSVC style environments.

By adding a symbol alias (so that one object file contains two
different symbols for the same function), users can run into
problems with duplicate definitions, if they themselves define
one of the symbols (for various reasons), but need to link in the
other one.

This happens for Wine, which provides their own definition of
"__chkstk", but when built in mingw mode does need compiler-rt to
provide the mingw specific symbol names; see
https://github.com/mstorsjo/llvm-mingw/issues/397.

To avoid the issue, remove the extra MS style names. They weren't
entirely usable as such for MSVC style environments anyway, as
compiler-rt builtins don't build these object files at all, when
built in MSVC mode; thus, the effort to provide them for MSVC
style environments in 885d7b759b5c166c07c07f4c58c6e0ba110fb0c2 was
a half-hearted step towards that.

If we really do want to provide those functions (as an alternative
to the ones provided by MSVC itself), we should do it in a separate
object file (even if the function implementation is the same), so
that users who have a definition of one of them but need a
definition of the other, won't have conflicts.

Additionally, if we do want to provide them for MSVC, those files
actually should be built when building the builtins in MSVC mode
as well (see compiler-rt/lib/builtins/CMakeLists.txt).

If we do that, there's a risk that an MSVC style build ends up
linking in and preferring our implementation over the one provided
by MSVC, which would be suboptimal. Our implementation always
probes the requested amount of stack, while the MSVC one checks the
amount of allocated stack and only probes as much as really is
needed.

In short - this reverts the situation to what it was in the 17.x
release series (except for unused functions that have been removed).
---
 compiler-rt/lib/builtins/i386/chkstk.S   | 2 --
 compiler-rt/lib/builtins/x86_64/chkstk.S | 2 --
 2 files changed, 4 deletions(-)

diff --git a/compiler-rt/lib/builtins/i386/chkstk.S b/compiler-rt/lib/builtins/i386/chkstk.S
index a84bb0ee30070..cdd9a4c2a5752 100644
--- a/compiler-rt/lib/builtins/i386/chkstk.S
+++ b/compiler-rt/lib/builtins/i386/chkstk.S
@@ -14,7 +14,6 @@
 .text
 .balign 4
 DEFINE_COMPILERRT_FUNCTION(_alloca) // _chkstk and _alloca are the same function
-DEFINE_COMPILERRT_FUNCTION(_chkstk)
         push   %ecx
         cmp    $0x1000,%eax
         lea    8(%esp),%ecx     // esp before calling this routine -> ecx
@@ -35,7 +34,6 @@ DEFINE_COMPILERRT_FUNCTION(_chkstk)
         push   (%eax)           // push return address onto the stack
         sub    %esp,%eax        // restore the original value in eax
         ret
-END_COMPILERRT_FUNCTION(_chkstk)
 END_COMPILERRT_FUNCTION(_alloca)
 
 #endif // __i386__
diff --git a/compiler-rt/lib/builtins/x86_64/chkstk.S b/compiler-rt/lib/builtins/x86_64/chkstk.S
index 494ee261193bc..ad7953a116ac7 100644
--- a/compiler-rt/lib/builtins/x86_64/chkstk.S
+++ b/compiler-rt/lib/builtins/x86_64/chkstk.S
@@ -18,7 +18,6 @@
 .text
 .balign 4
 DEFINE_COMPILERRT_FUNCTION(___chkstk_ms)
-DEFINE_COMPILERRT_FUNCTION(__chkstk)
         push   %rcx
         push   %rax
         cmp    $0x1000,%rax
@@ -36,7 +35,6 @@ DEFINE_COMPILERRT_FUNCTION(__chkstk)
         pop    %rax
         pop    %rcx
         ret
-END_COMPILERRT_FUNCTION(__chkstk)
 END_COMPILERRT_FUNCTION(___chkstk_ms)
 
 #endif // __x86_64__

