Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[llvm] Set emulated-tls by default for x86_64-windows-gnu target #79542

Closed
wants to merge 1 commit into from

Conversation

trcrsired
Copy link

GCC on Windows uses Emulated TLS by default.
msys2 is doing this patch too. We should just enable this upstream.
https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-clang/0004-enable-emutls-for-mingw.patch

Copy link

Thank you for submitting a Pull Request (PR) to the LLVM Project!

This PR will be automatically labeled and the relevant teams will be
notified.

If you wish to, you can add reviewers by using the "Reviewers" section on this page.

If this is not working for you, it is probably because you do not have write
permissions for the repository. In which case you can instead tag reviewers by
name in a comment by using @ followed by their GitHub username.

If you have received no comments on your PR for a week, you can request a review
by "ping"ing the PR by adding a comment “Ping”. The common courtesy "ping" rate
is once a week. Please remember that you are asking for valuable time from other developers.

If you have further questions, they may be answered by the LLVM GitHub User Guide.

You can also ask questions in a comment on this PR, on the LLVM Discord or on the forums.

x86_64-windows-gnu target by default enables emulated-tls on gcc,
we should do the same
@trcrsired trcrsired force-pushed the mingwtargetdefaultemulatedtls branch from 54c8e7d to ddc0229 Compare January 27, 2024 00:22
@asl asl requested a review from mstorsjo January 27, 2024 06:25
@mstorsjo
Copy link
Member

msys2 is doing this patch too. We should just enable this upstream. https://github.com/msys2/MINGW-packages/blob/master/mingw-w64-clang/0004-enable-emutls-for-mingw.patch

Not quite. Msys2 are enabling this patch conditionally, in some of their environments (the GCC based ones), but not in all of them - in particular, not in the clang based ones: https://github.com/msys2/MINGW-packages/blob/097b2e3ac9fbdda192010f2b111b6cc628cd1190/mingw-w64-clang/PKGBUILD#L165-L169

While this patch indeed does help interoperability with GCC based environments, in particular interop with libstdc++, this is an ABI break for all environments that currently use the Clang default of native TLS. I.e. this would be an ABI break for llvm-mingw, and for the clang environments in msys2, at the very least. Thus - in order to proceed with such a change, all the aspects of such an ABI break must be considered and agreed upon by involved vendors, CC @mati865 @lazka @jeremyd2019.

The title of this PR is misleading; it claims to enable this behaviour for the x86_64-windows-gnu target - while the patch actually enables it for all architectures.

For the x86 based targets, there could be some merit in doing a change like this, for interop with GCC. But for ARM targets, there's no preexisting GCC based environments to be compatible with, so there's really no reason to break ABI there. (There exists an ongoing effort to get a working aarch64-mingw target in GCC though, but it's not upstream yet, and AFAIK they are at a stage where there might be various pieces missing wrt ABI compat from their side anyway.)

As for the implications of a potential ABI break: With native TLS, which is the default in llvm-mingw and the msys2 clang environments, one cannot do direct access to TLS variables across DLL boundaries (i.e., any access to a TLS variable from another DLL needs to happen via an accessor function). (Overall this is a reasonable design anyway - keeping the individual TLS variables as static within one translation unit greatly simplifies the code generation for accessing it, and it avoids or used to avoid GCC issues with that anyway, see e.g. 7106f58.)

If there's no access to TLS variables across DLLs, the scope of an ABI break mostly is within individual DLLs, i.e. it would affect existing object files, and static libraries. Such an ABI break could maybe be tolerable, if other things work out. What do the msys2 maintainers think about that?

Finally, I have concerns about how this could interact with lower level constructs. Apparently one such issue has already been encountered and fixed by @jeremyd2019 in mingw-w64/mingw-w64@6ded96c. There could potentially be other such issues elsewhere - this would need to be test run a fair bit before I'd be comfortable changing it, from the llvm-mingw point of view. (Note to self: Related issues to look into, #78207 and https://reviews.llvm.org/D155278.)

In short: This needs to be reframed to acknowledge that it is an ABI break. If the relevant stakeholders agree, we could consider taking such an ABI break, but such a discussion needs to be had first. This should be changed to only affect i386 and x86_64, but no other architectures. And we would need to testrun this a bit to make sure it doesn't regress corner cases in existing setups in e.g. llvm-mingw.

Copy link
Member

@mstorsjo mstorsjo left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Marking as changes requested, to indicate that we don't want to merge this as such quite yet.

@mstorsjo
Copy link
Member

And another practical concern: This breaks at least one test in the testsuite, CodeGen/X86/tls.ll, as shown in the CI tests here. Possibly more, if running the full LLVM+Clang tests in a native mingw environment - these would need to be checked and updated accordingly.

@mati865
Copy link
Contributor

mati865 commented Jan 28, 2024

Years ago, when such a change wouldn't cause such widespread breakage, it was rejected by @rnk (and in my opinion, it was the right decision). This has proven to be one of the selling points for using LLVM over GNU toolchains.

Given those reasons, I'm against changing the default, but I wouldn't oppose making this configurable.

@trcrsired trcrsired closed this Jan 28, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants