Re: Fwd: libstdc++: Attempt to resolve PR83562
On 11/6/20 8:34 AM, Martin Storsjö wrote: On Fri, 6 Nov 2020, Liu Hao via Gcc-patches wrote: 在 2020/10/29 下午3:56, Liu Hao 写道: I forward it here for comments. Basing on the behavior of both GCC and Clang, `__cxa_thread_atexit` is used to register the destructor of thread_local objects directly, suggesting the first parameter should have `__thiscall` convention. libstdc++ used the default `__cdecl` convention and caused crashes on 1686-w64-mingw32 (see PR83562). But to my surprise, libcxxabi uses `__cdecl` too [1], but I haven't heard any of relevant reports so far. Original patch is attached in case you can't find it in gcc-patches. FWIW, this patch looks good and correct to me, from a mingw perspective. // Martin Thanks pushed to gcc master branch as 7fc0f78c3f43af1967cb7b1ee8f4947f3b890aa2. OpenPGP_0x713B5FE29C145D45_and_old_rev.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: Fwd: libstdc++: Attempt to resolve PR83562
On Fri, 6 Nov 2020, Liu Hao via Gcc-patches wrote: 在 2020/10/29 下午3:56, Liu Hao 写道: I forward it here for comments. Basing on the behavior of both GCC and Clang, `__cxa_thread_atexit` is used to register the destructor of thread_local objects directly, suggesting the first parameter should have `__thiscall` convention. libstdc++ used the default `__cdecl` convention and caused crashes on 1686-w64-mingw32 (see PR83562). But to my surprise, libcxxabi uses `__cdecl` too [1], but I haven't heard any of relevant reports so far. Original patch is attached in case you can't find it in gcc-patches. FWIW, this patch looks good and correct to me, from a mingw perspective. // Martin
Re: Fwd: libstdc++: Attempt to resolve PR83562
Ping? 在 2020/10/29 下午3:56, Liu Hao 写道: > I forward it here for comments. > > Basing on the behavior of both GCC and Clang, `__cxa_thread_atexit` is used > to register the > destructor of thread_local objects directly, suggesting the first parameter > should have `__thiscall` > convention. > > libstdc++ used the default `__cdecl` convention and caused crashes on > 1686-w64-mingw32 (see > PR83562). But to my surprise, libcxxabi uses `__cdecl` too [1], but I haven't > heard any of relevant > reports so far. > > Original patch is attached in case you can't find it in gcc-patches. > > > [1] > https://github.com/llvm/llvm-project/blob/97b351a827677ebbedc10bfbce8ef8844c246553/libcxxabi/src/cxa_thread_atexit.cpp#L22 > > -- Best regards, LH_Mouse signature.asc Description: OpenPGP digital signature
Fwd: libstdc++: Attempt to resolve PR83562
I forward it here for comments. Basing on the behavior of both GCC and Clang, `__cxa_thread_atexit` is used to register the destructor of thread_local objects directly, suggesting the first parameter should have `__thiscall` convention. libstdc++ used the default `__cdecl` convention and caused crashes on 1686-w64-mingw32 (see PR83562). But to my surprise, libcxxabi uses `__cdecl` too [1], but I haven't heard any of relevant reports so far. Original patch is attached in case you can't find it in gcc-patches. [1] https://github.com/llvm/llvm-project/blob/97b351a827677ebbedc10bfbce8ef8844c246553/libcxxabi/src/cxa_thread_atexit.cpp#L22 转发的消息 主题: Re: libstdc++: Attempt to resolve PR83562 日期: Tue, 27 Oct 2020 22:38:29 +0800 发件人: Liu Hao 收件人: Jason Merrill , GCC Patches 在 2020/10/8 22:56, Jason Merrill 写道: > > Hmm, why isn't the mingw implementation used for all programs? That would > avoid the bug. > There was a little further discussion about this [1]. TL;DR: The mingw-w64 function is linked statically and subject to issues about order of destruction. Recently mingw-w64 has got its own `__cxa_thread_atexit()` so libstdc++ no longer exposes it. This patch for libstdc++ fixes calling conventions for destructors on i686 so they match mingw-w64 ones. [1] https://github.com/msys2/MINGW-packages/issues/7096 [2] Below is a direct quote from #mingw-w64 on OFTC: (lh_ideapad is me and wbs is Martin Storsjö.) ``` [14:29:32] wbs, what was the rationale for the `__thiscall` convention for `__cxa_thread_atexit()` and `__cxa_atexit()` in our CRT? I suspect it is correct, but it is not specified anywhere in Itanium ABI. [14:30:41] In case of evidence for that, the GCC prototype (with default __cdecl) should be wrong. [14:31:10] See this: https://github.com/msys2/MINGW-packages/issues/7096 [14:52:05] lh_ideapad: itanium ABI doesn't really talk about windows things, but, the function that is passed to __cxa_thread_atexit is the object's destructor function, and when calling the destructor, which is technically a member function, it's done with the thiscall calling convention [14:52:31] lh_ideapad: example: https://godbolt.org/z/qbfWT1 (only clang as there's no gcc-mingw there, but if you try the same there you'll see the same thing) [14:52:35] Title: Compiler Explorer (at godbolt.org) [14:52:58] lh_ideapad: the destruct function shows that when calling __ZN7MyClassD1Ev, the destructor, it passes the object pointer in ecx, i.e. thiscall [14:53:42] lh_ideapad: and when adding the object to the cleanup list, the __ZN7MyClassD1Ev function is passed directly to ___cxa_thread_atexit, which then will need to call the function using the thiscall convention [14:59:54] lh_ideapad: so yes, I would agree with your patch changing libsupc++ to use thiscall [15:13:01] gcc is doing the same thing with a wrong calling convention , leaving a garbage value on i686-w64-mingw32. [15:13:38] yup, so definite +1 on your libsupc++ patch for that [15:14:00] then how about `__cxa_atexit`? [15:14:26] I would say it should work the same, but gcc doesn't normally use that one, right? [15:14:29] it's not used by GCC (the standard `atexit()` is used). [15:15:26] clang has a flag -fuse-cxa-atexit, which makes it use cxa_atexit instead of atexit [15:15:40] I was a bit dubious on it. [15:18:59] GCC has `-fuse-cxa-atexit` too . Let me check. [15:18:59] (I tested it), clang does use __cxa_atexit if the -fuse-cxa-atexit flag is used, and then the dtor (thiscall) is passed directly to __cxa_atexit, so that's +1 datapoint to that it should have thiscall. gcc doesn't use __cxa_atexit for i686 windows despite -fuse-cxa-atexit, so that's no points in either direction [15:19:28] both clang and gcc use a wrapper function that fixes the calling convention, when using atexit at least [15:20:22] `-fuse-cxa-atexit` seems to have no effect on `i686-w64-mingw32-g++`. [15:20:46] exactly. so in practice it doesn't matter for gcc, but I think libsupc++ should handle it the same [15:23:11] ok I will compose a new patch for both functions later today. [15:23:13] :) [15:23:25] \o/ [15:24:40] then for the other issue that the user was posting about; I remember testing and noticing that the mingw-w64-crt implementation of __cxa_thread_atexit doesn't work with emutls, but in all of my tests, it has been a non-issue as it has ended up using the libsupc++ code instead [15:50:50] probably static linking is broken, so one must link against shared libstdc++. [15:52:20] it doesn't matter whether it is the CRT or libsupc++ implementation that is linked statically. [15:53:13] it seems like current msys builds of libstdc++ doesn't include __cxa_thread_atexit in libstdc++ at all. I'm pretty sure I tested this back when I made the mingw version, but I'll investigate and try to pinpoint what changed (did gcc at some point start noticing that mingw-w64-crt contains it and st
Re: libstdc++: Attempt to resolve PR83562
On 10/27/20 2:38 PM, Liu Hao via Gcc-patches wrote: 在 2020/10/8 22:56, Jason Merrill 写道: Hmm, why isn't the mingw implementation used for all programs? That would avoid the bug. There was a little further discussion about this [1]. TL;DR: The mingw-w64 function is linked statically and subject to issues about order of destruction. Recently mingw-w64 has got its own `__cxa_thread_atexit()` so libstdc++ no longer exposes it. This patch for libstdc++ fixes calling conventions for destructors on i686 so they match mingw-w64 ones. Any more comments? I'd like to push this soon if there are no more issues open. OpenPGP_0x713B5FE29C145D45_and_old_rev.asc Description: application/pgp-keys OpenPGP_signature Description: OpenPGP digital signature
Re: libstdc++: Attempt to resolve PR83562
在 2020/10/8 22:56, Jason Merrill 写道: > > Hmm, why isn't the mingw implementation used for all programs? That would > avoid the bug. > There was a little further discussion about this [1]. TL;DR: The mingw-w64 function is linked statically and subject to issues about order of destruction. Recently mingw-w64 has got its own `__cxa_thread_atexit()` so libstdc++ no longer exposes it. This patch for libstdc++ fixes calling conventions for destructors on i686 so they match mingw-w64 ones. [1] https://github.com/msys2/MINGW-packages/issues/7096 [2] Below is a direct quote from #mingw-w64 on OFTC: (lh_ideapad is me and wbs is Martin Storsjö.) ``` [14:29:32] wbs, what was the rationale for the `__thiscall` convention for `__cxa_thread_atexit()` and `__cxa_atexit()` in our CRT? I suspect it is correct, but it is not specified anywhere in Itanium ABI. [14:30:41] In case of evidence for that, the GCC prototype (with default __cdecl) should be wrong. [14:31:10] See this: https://github.com/msys2/MINGW-packages/issues/7096 [14:52:05] lh_ideapad: itanium ABI doesn't really talk about windows things, but, the function that is passed to __cxa_thread_atexit is the object's destructor function, and when calling the destructor, which is technically a member function, it's done with the thiscall calling convention [14:52:31] lh_ideapad: example: https://godbolt.org/z/qbfWT1 (only clang as there's no gcc-mingw there, but if you try the same there you'll see the same thing) [14:52:35] Title: Compiler Explorer (at godbolt.org) [14:52:58] lh_ideapad: the destruct function shows that when calling __ZN7MyClassD1Ev, the destructor, it passes the object pointer in ecx, i.e. thiscall [14:53:42] lh_ideapad: and when adding the object to the cleanup list, the __ZN7MyClassD1Ev function is passed directly to ___cxa_thread_atexit, which then will need to call the function using the thiscall convention [14:59:54] lh_ideapad: so yes, I would agree with your patch changing libsupc++ to use thiscall [15:13:01] gcc is doing the same thing with a wrong calling convention , leaving a garbage value on i686-w64-mingw32. [15:13:38] yup, so definite +1 on your libsupc++ patch for that [15:14:00] then how about `__cxa_atexit`? [15:14:26] I would say it should work the same, but gcc doesn't normally use that one, right? [15:14:29] it's not used by GCC (the standard `atexit()` is used). [15:15:26] clang has a flag -fuse-cxa-atexit, which makes it use cxa_atexit instead of atexit [15:15:40] I was a bit dubious on it. [15:18:59] GCC has `-fuse-cxa-atexit` too . Let me check. [15:18:59] (I tested it), clang does use __cxa_atexit if the -fuse-cxa-atexit flag is used, and then the dtor (thiscall) is passed directly to __cxa_atexit, so that's +1 datapoint to that it should have thiscall. gcc doesn't use __cxa_atexit for i686 windows despite -fuse-cxa-atexit, so that's no points in either direction [15:19:28] both clang and gcc use a wrapper function that fixes the calling convention, when using atexit at least [15:20:22] `-fuse-cxa-atexit` seems to have no effect on `i686-w64-mingw32-g++`. [15:20:46] exactly. so in practice it doesn't matter for gcc, but I think libsupc++ should handle it the same [15:23:11] ok I will compose a new patch for both functions later today. [15:23:13] :) [15:23:25] \o/ [15:24:40] then for the other issue that the user was posting about; I remember testing and noticing that the mingw-w64-crt implementation of __cxa_thread_atexit doesn't work with emutls, but in all of my tests, it has been a non-issue as it has ended up using the libsupc++ code instead [15:50:50] probably static linking is broken, so one must link against shared libstdc++. [15:52:20] it doesn't matter whether it is the CRT or libsupc++ implementation that is linked statically. [15:53:13] it seems like current msys builds of libstdc++ doesn't include __cxa_thread_atexit in libstdc++ at all. I'm pretty sure I tested this back when I made the mingw version, but I'll investigate and try to pinpoint what changed (did gcc at some point start noticing that mingw-w64-crt contains it and stop providing their own?) or whether I just missed something when I tested it back when I wrote it... [15:59:47] I'll follow up on that other issue and mingw bugtracker issue, but I'll do a couple hours of comple testing/studying things first to be able to give an informed comment [16:15:34] * pamaury (~pama...@ip-41.net-89-3-53.rev.numericable.fr) has joined [16:27:34] wbs, there is a check in `libstdc++-v3/configure.ac` around line 275. [16:29:22] lh_ideapad: yeah, but in my tests it doesn't pick up on it. I test by cross-bootstrapping a toolchain, so maybe this check runs before the mingw-w64-crt actually is built [16:29:50] so it doesn't detect if just bootstrapping once, but if building a new gcc in an already complete environment, it behaves differently [16:33:21] * Pali (~p...@0001caa5.user.oftc.net)
Re: libstdc++: Attempt to resolve PR83562
在 2020/10/8 22:56, Jason Merrill 写道:
> On 10/7/20 10:52 PM, Liu Hao via Gcc-patches wrote:
>> [Please CC me as I am not subscribed to this list.]
>
> Hmm, why isn't the mingw implementation used for all programs? That would
> avoid the bug.
>
I am afraid the libstdc++ implementation has to be kept for compatibility, as
the mingw-w64 one was only added two years
ago. Neither am I clear about MinGW.org.
>
> This patch is a good start but won't actually fix the bug: the calling
> convention only makes a difference when we actually
> call the function, at the line
>
> e->destructor (e->object);
>
> in atexit_thread.cc, and your patch doesn't change the calling convention for
> elt::destructor.
>
> I'd think we should add the attribute to __cxa_cdtor_type, and use it more
> consistently for __cxa_*atexit and __cxa_throw.
>
This patch contains a hunk
```diff
@@ -52,7 +57,7 @@ namespace {
// One element in a singly-linked stack of cleanups.
struct elt
{
-void (*destructor)(void *);
+void (_GLIBCXX_CDTOR_CALLABI *destructor)(void *);
void *object;
elt *next;
#ifdef _GLIBCXX_THREAD_ATEXIT_WIN32
```
which should suffice. I am not against introduction of another macro for this
calling convention thing, as the name
`__cxa_thread_atexit()` doesn't suggest its first argument is a non-static
member function. (Technically I think it should
not be, so GCC's use of it to register the destructor looks like a bug. The
call should go through a thunk.)
--
Best regards,
LH_Mouse
signature.asc
Description: OpenPGP digital signature
Re: libstdc++: Attempt to resolve PR83562
On 10/7/20 10:52 PM, Liu Hao via Gcc-patches wrote: [Please CC me as I am not subscribed to this list.] [This patch is only a draft and has not been tested at all.] Some details have been discussed in [1]. mingw-w64 has got an implementation [2] [3] for static libraries, but it takes a destructor using the `__thiscall` convention. As both functions have C linkage, they should agree with each other and should behave simliarily. Hmm, why isn't the mingw implementation used for all programs? That would avoid the bug. Considerations: 0) This is going to be an ABI breakage for i?86. I am not sure whether people would expect the old, broken behavior. I'm sure they wouldn't. 1) GCC doesn't provide `__cxa_atexit()`, but it would suffer from the same problem. At the moment GCC calls `atexit()` to register destructors so it appears to work. Yes. 2) There is an explicit reference to `__cxa_atexit` in 'gcc/cp/decl.c' and it probably be changed, unless it is not used by 1?86-w64-mingw32 targets. [1] https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83562 [2] https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/crt/cxa_thread_atexit.c [3] https://sourceforge.net/p/mingw-w64/mingw-w64/ci/f3e0fbb40cbc9f8821db8bd8a0c4dae8ff671e9f/ [4] https://github.com/msys2/MINGW-packages/issues/7071 This patch is a good start but won't actually fix the bug: the calling convention only makes a difference when we actually call the function, at the line e->destructor (e->object); in atexit_thread.cc, and your patch doesn't change the calling convention for elt::destructor. I'd think we should add the attribute to __cxa_cdtor_type, and use it more consistently for __cxa_*atexit and __cxa_throw. Jason
