Skip to content

Commit

Permalink
[compiler-rt] Remove duplicate MS names for chkstk symbols (llvm#80450)
Browse files Browse the repository at this point in the history
Prior to 885d7b7, 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 885d7b7, 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
mstorsjo/llvm-mingw#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 885d7b7 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).
  • Loading branch information
mstorsjo authored Feb 3, 2024
1 parent 752c172 commit 248aeac
Show file tree
Hide file tree
Showing 2 changed files with 0 additions and 4 deletions.
2 changes: 0 additions & 2 deletions compiler-rt/lib/builtins/i386/chkstk.S
Original file line number Diff line number Diff line change
Expand Up @@ -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
Expand All @@ -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__
2 changes: 0 additions & 2 deletions compiler-rt/lib/builtins/x86_64/chkstk.S
Original file line number Diff line number Diff line change
Expand Up @@ -18,7 +18,6 @@
.text
.balign 4
DEFINE_COMPILERRT_FUNCTION(___chkstk_ms)
DEFINE_COMPILERRT_FUNCTION(__chkstk)
push %rcx
push %rax
cmp $0x1000,%rax
Expand All @@ -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__

0 comments on commit 248aeac

Please sign in to comment.