Attempt to resolve PR83562

Attempt to resolve PR83562

[Liu Hao]

[-- Attachment #1.1.1: Type: text/plain, Size: 1200 bytes --]

[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.


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.
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.
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


-- 
Best regards,
LH_Mouse

[-- Attachment #1.1.2: 0001-libsupc-Make-the-destructor-parameter-to-__cxa_threa.patch --]
[-- Type: text/x-patch, Size: 3537 bytes --]

From ac325bdcd6e3fa522f8b59d436cd4b3ab663de5c Mon Sep 17 00:00:00 2001
From: Liu Hao <lh_mouse@126.com>
Date: Thu, 8 Oct 2020 10:26:13 +0800
Subject: [PATCH] libsupc++: Make the destructor parameter to
 `__cxa_thread_atexit()` use the `__thiscall` calling convention for
 i686-w64-mingw32

The mingw-w64 implementations of `__cxa_thread_atexit()` and `__cxa_atexit()` have been
using `__thiscall` since two years ago. Using the default calling convention (which is
`__cdecl`) causes crashes as explained in PR83562.

Calling conventions have no effect on x86_64-w64-mingw32.

Reference: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83562
Reference: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/crt/cxa_thread_atexit.c
Reference: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/f3e0fbb40cbc9f8821db8bd8a0c4dae8ff671e9f/
Reference: https://github.com/msys2/MINGW-packages/issues/7071
Signed-off-by: Liu Hao <lh_mouse@126.com>
---
 libstdc++-v3/libsupc++/atexit_thread.cc | 14 ++++++++++----
 libstdc++-v3/libsupc++/cxxabi.h         |  8 ++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/libstdc++-v3/libsupc++/atexit_thread.cc b/libstdc++-v3/libsupc++/atexit_thread.cc
index e97644f8cd4..4f7f982ac6b 100644
--- a/libstdc++-v3/libsupc++/atexit_thread.cc
+++ b/libstdc++-v3/libsupc++/atexit_thread.cc
@@ -30,16 +30,21 @@
 #include <windows.h>
 #endif
 
+// Simplify it a little for this file.
+#ifndef _GLIBCXX_CDTOR_CALLABI
+#  define _GLIBCXX_CDTOR_CALLABI
+#endif
+
 #if _GLIBCXX_HAVE___CXA_THREAD_ATEXIT
 
 // Libc provides __cxa_thread_atexit definition.
 
 #elif _GLIBCXX_HAVE___CXA_THREAD_ATEXIT_IMPL
 
-extern "C" int __cxa_thread_atexit_impl (void (*func) (void *),
+extern "C" int __cxa_thread_atexit_impl (void (_GLIBCXX_CDTOR_CALLABI *func) (void *),
 					 void *arg, void *d);
 extern "C" int
-__cxxabiv1::__cxa_thread_atexit (void (*dtor)(void *),
+__cxxabiv1::__cxa_thread_atexit (void (_GLIBCXX_CDTOR_CALLABI *dtor)(void *),
 				 void *obj, void *dso_handle)
   _GLIBCXX_NOTHROW
 {
@@ -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
@@ -116,7 +121,8 @@ namespace {
 }
 
 extern "C" int
-__cxxabiv1::__cxa_thread_atexit (void (*dtor)(void *), void *obj, void */*dso_handle*/)
+__cxxabiv1::__cxa_thread_atexit (void (_GLIBCXX_CDTOR_CALLABI *dtor)(void *),
+				 void *obj, void */*dso_handle*/)
   _GLIBCXX_NOTHROW
 {
   // Do this initialization once.
diff --git a/libstdc++-v3/libsupc++/cxxabi.h b/libstdc++-v3/libsupc++/cxxabi.h
index 000713ecdf8..3d6217a6fac 100644
--- a/libstdc++-v3/libsupc++/cxxabi.h
+++ b/libstdc++-v3/libsupc++/cxxabi.h
@@ -125,14 +125,22 @@ namespace __cxxabiv1
 
   // DSO destruction.
   int
+#ifdef _GLIBCXX_CDTOR_CALLABI
+  __cxa_atexit(void (_GLIBCXX_CDTOR_CALLABI *)(void*), void*, void*) _GLIBCXX_NOTHROW;
+#else
   __cxa_atexit(void (*)(void*), void*, void*) _GLIBCXX_NOTHROW;
+#endif
 
   void
   __cxa_finalize(void*);
 
   // TLS destruction.
   int
+#ifdef _GLIBCXX_CDTOR_CALLABI
+  __cxa_thread_atexit(void (_GLIBCXX_CDTOR_CALLABI *)(void*), void*, void *) _GLIBCXX_NOTHROW;
+#else
   __cxa_thread_atexit(void (*)(void*), void*, void *) _GLIBCXX_NOTHROW;
+#endif
 
   // Pure virtual functions.
   void
-- 
2.20.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: libstdc++: Attempt to resolve PR83562

[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.]
> [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

Re: libstdc++: Attempt to resolve PR83562

[Liu Hao]

[-- Attachment #1.1: Type: text/plain, Size: 1588 bytes --]

在 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


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: libstdc++: Attempt to resolve PR83562

[Liu Hao]

[-- Attachment #1.1: Type: text/plain, Size: 5505 bytes --]

在 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] <lh_ideapad> 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] <lh_ideapad> In case of evidence for that, the GCC prototype (with default __cdecl) should be wrong.
[14:31:10] <lh_ideapad> See this:  https://github.com/msys2/MINGW-packages/issues/7096
[14:52:05] <wbs> 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] <wbs> 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] <minifox> Title: Compiler Explorer (at godbolt.org)
[14:52:58] <wbs> 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] <wbs> 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] <wbs> lh_ideapad: so yes, I would agree with your patch changing libsupc++ to use thiscall
[15:13:01] <lh_ideapad> gcc is doing the same thing with a wrong calling convention , leaving a garbage value on
i686-w64-mingw32.
[15:13:38] <wbs> yup, so definite +1 on your libsupc++ patch for that
[15:14:00] <lh_ideapad> then how about `__cxa_atexit`?
[15:14:26] <wbs> I would say it should work the same, but gcc doesn't normally use that one, right?
[15:14:29] <lh_ideapad> it's not used by GCC (the standard `atexit()` is used).
[15:15:26] <wbs> clang has a flag -fuse-cxa-atexit, which makes it use cxa_atexit instead of atexit
[15:15:40] <lh_ideapad> I was a bit dubious on it.
[15:18:59] <lh_ideapad> GCC has `-fuse-cxa-atexit` too .  Let me check.
[15:18:59] <wbs> (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] <wbs> both clang and gcc use a wrapper function that fixes the calling convention, when using atexit at least
[15:20:22] <lh_ideapad> `-fuse-cxa-atexit` seems to have no effect on `i686-w64-mingw32-g++`.
[15:20:46] <wbs> exactly. so in practice it doesn't matter for gcc, but I think libsupc++ should handle it the same
[15:23:11] <lh_ideapad> ok I will compose a new patch for both functions later today.
[15:23:13] <lh_ideapad> :)
[15:23:25] <wbs> \o/
[15:24:40] <wbs> 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] <lh_ideapad> probably static linking is broken, so one must link against shared libstdc++.
[15:52:20] <lh_ideapad> it doesn't matter whether it is the CRT or libsupc++ implementation that is linked statically.
[15:53:13] <wbs> 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] <wbs> 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 (~pamaury@ip-41.net-89-3-53.rev.numericable.fr) has joined
[16:27:34] <lh_ideapad> wbs, there is a check in `libstdc++-v3/configure.ac` around line 275.
[16:29:22] <wbs> 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] <wbs> 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 (~pali@0001caa5.user.oftc.net) has joined
[16:34:52] <wbs> ah, here it is:
[16:34:52] <wbs> # Only do link tests if native. Else, hardcode.
[16:34:52] <wbs> if $GLIBCXX_IS_NATIVE; then
```





-- 
Best regards,
LH_Mouse


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: libstdc++: Attempt to resolve PR83562

[Jonathan Yong]

[-- Attachment #1.1.1: Type: text/plain, Size: 671 bytes --]

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.


[-- Attachment #1.1.2: OpenPGP_0x713B5FE29C145D45_and_old_rev.asc --]
[-- Type: application/pgp-keys, Size: 8035 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]

Fwd: libstdc++: Attempt to resolve PR83562

[Liu Hao]

[-- Attachment #1.1.1: Type: text/plain, Size: 6454 bytes --]

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\x0f





-------- 转发的消息 --------
主题: Re: libstdc++: Attempt to resolve PR83562
日期: Tue, 27 Oct 2020 22:38:29 +0800
发件人: Liu Hao <lh_mouse@126.com>
收件人: Jason Merrill <jason@redhat.com>, GCC Patches <gcc-patches@gcc.gnu.org>

在 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] <lh_ideapad> 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] <lh_ideapad> In case of evidence for that, the GCC prototype (with default __cdecl)
should be wrong.
[14:31:10] <lh_ideapad> See this:  https://github.com/msys2/MINGW-packages/issues/7096
[14:52:05] <wbs> 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] <wbs> 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] <minifox> Title: Compiler Explorer (at godbolt.org)
[14:52:58] <wbs> 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] <wbs> 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] <wbs> lh_ideapad: so yes, I would agree with your patch changing libsupc++ to use thiscall
[15:13:01] <lh_ideapad> gcc is doing the same thing with a wrong calling convention , leaving a
garbage value on
i686-w64-mingw32.
[15:13:38] <wbs> yup, so definite +1 on your libsupc++ patch for that
[15:14:00] <lh_ideapad> then how about `__cxa_atexit`?
[15:14:26] <wbs> I would say it should work the same, but gcc doesn't normally use that one, right?
[15:14:29] <lh_ideapad> it's not used by GCC (the standard `atexit()` is used).
[15:15:26] <wbs> clang has a flag -fuse-cxa-atexit, which makes it use cxa_atexit instead of atexit
[15:15:40] <lh_ideapad> I was a bit dubious on it.
[15:18:59] <lh_ideapad> GCC has `-fuse-cxa-atexit` too .  Let me check.
[15:18:59] <wbs> (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] <wbs> both clang and gcc use a wrapper function that fixes the calling convention, when
using atexit at least
[15:20:22] <lh_ideapad> `-fuse-cxa-atexit` seems to have no effect on `i686-w64-mingw32-g++`.
[15:20:46] <wbs> exactly. so in practice it doesn't matter for gcc, but I think libsupc++ should
handle it the same
[15:23:11] <lh_ideapad> ok I will compose a new patch for both functions later today.
[15:23:13] <lh_ideapad> :)
[15:23:25] <wbs> \o/
[15:24:40] <wbs> 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] <lh_ideapad> probably static linking is broken, so one must link against shared libstdc++.
[15:52:20] <lh_ideapad> it doesn't matter whether it is the CRT or libsupc++ implementation that is
linked statically.
[15:53:13] <wbs> 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] <wbs> 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 (~pamaury@ip-41.net-89-3-53.rev.numericable.fr) has joined
[16:27:34] <lh_ideapad> wbs, there is a check in `libstdc++-v3/configure.ac` around line 275.
[16:29:22] <wbs> 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] <wbs> 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 (~pali@0001caa5.user.oftc.net) has joined
[16:34:52] <wbs> ah, here it is:
[16:34:52] <wbs> # Only do link tests if native. Else, hardcode.
[16:34:52] <wbs> if $GLIBCXX_IS_NATIVE; then
```





-- 
Best regards,
LH_Mouse



[-- Attachment #1.1.2: 0001-libsupc-Make-the-destructor-parameter-to-__cxa_threa.patch --]
[-- Type: text/x-patch, Size: 3537 bytes --]

From ac325bdcd6e3fa522f8b59d436cd4b3ab663de5c Mon Sep 17 00:00:00 2001
From: Liu Hao <lh_mouse@126.com>
Date: Thu, 8 Oct 2020 10:26:13 +0800
Subject: [PATCH] libsupc++: Make the destructor parameter to
 `__cxa_thread_atexit()` use the `__thiscall` calling convention for
 i686-w64-mingw32

The mingw-w64 implementations of `__cxa_thread_atexit()` and `__cxa_atexit()` have been
using `__thiscall` since two years ago. Using the default calling convention (which is
`__cdecl`) causes crashes as explained in PR83562.

Calling conventions have no effect on x86_64-w64-mingw32.

Reference: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=83562
Reference: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/master/tree/mingw-w64-crt/crt/cxa_thread_atexit.c
Reference: https://sourceforge.net/p/mingw-w64/mingw-w64/ci/f3e0fbb40cbc9f8821db8bd8a0c4dae8ff671e9f/
Reference: https://github.com/msys2/MINGW-packages/issues/7071
Signed-off-by: Liu Hao <lh_mouse@126.com>
---
 libstdc++-v3/libsupc++/atexit_thread.cc | 14 ++++++++++----
 libstdc++-v3/libsupc++/cxxabi.h         |  8 ++++++++
 2 files changed, 18 insertions(+), 4 deletions(-)

diff --git a/libstdc++-v3/libsupc++/atexit_thread.cc b/libstdc++-v3/libsupc++/atexit_thread.cc
index e97644f8cd4..4f7f982ac6b 100644
--- a/libstdc++-v3/libsupc++/atexit_thread.cc
+++ b/libstdc++-v3/libsupc++/atexit_thread.cc
@@ -30,16 +30,21 @@
 #include <windows.h>
 #endif
 
+// Simplify it a little for this file.
+#ifndef _GLIBCXX_CDTOR_CALLABI
+#  define _GLIBCXX_CDTOR_CALLABI
+#endif
+
 #if _GLIBCXX_HAVE___CXA_THREAD_ATEXIT
 
 // Libc provides __cxa_thread_atexit definition.
 
 #elif _GLIBCXX_HAVE___CXA_THREAD_ATEXIT_IMPL
 
-extern "C" int __cxa_thread_atexit_impl (void (*func) (void *),
+extern "C" int __cxa_thread_atexit_impl (void (_GLIBCXX_CDTOR_CALLABI *func) (void *),
 					 void *arg, void *d);
 extern "C" int
-__cxxabiv1::__cxa_thread_atexit (void (*dtor)(void *),
+__cxxabiv1::__cxa_thread_atexit (void (_GLIBCXX_CDTOR_CALLABI *dtor)(void *),
 				 void *obj, void *dso_handle)
   _GLIBCXX_NOTHROW
 {
@@ -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
@@ -116,7 +121,8 @@ namespace {
 }
 
 extern "C" int
-__cxxabiv1::__cxa_thread_atexit (void (*dtor)(void *), void *obj, void */*dso_handle*/)
+__cxxabiv1::__cxa_thread_atexit (void (_GLIBCXX_CDTOR_CALLABI *dtor)(void *),
+				 void *obj, void */*dso_handle*/)
   _GLIBCXX_NOTHROW
 {
   // Do this initialization once.
diff --git a/libstdc++-v3/libsupc++/cxxabi.h b/libstdc++-v3/libsupc++/cxxabi.h
index 000713ecdf8..3d6217a6fac 100644
--- a/libstdc++-v3/libsupc++/cxxabi.h
+++ b/libstdc++-v3/libsupc++/cxxabi.h
@@ -125,14 +125,22 @@ namespace __cxxabiv1
 
   // DSO destruction.
   int
+#ifdef _GLIBCXX_CDTOR_CALLABI
+  __cxa_atexit(void (_GLIBCXX_CDTOR_CALLABI *)(void*), void*, void*) _GLIBCXX_NOTHROW;
+#else
   __cxa_atexit(void (*)(void*), void*, void*) _GLIBCXX_NOTHROW;
+#endif
 
   void
   __cxa_finalize(void*);
 
   // TLS destruction.
   int
+#ifdef _GLIBCXX_CDTOR_CALLABI
+  __cxa_thread_atexit(void (_GLIBCXX_CDTOR_CALLABI *)(void*), void*, void *) _GLIBCXX_NOTHROW;
+#else
   __cxa_thread_atexit(void (*)(void*), void*, void *) _GLIBCXX_NOTHROW;
+#endif
 
   // Pure virtual functions.
   void
-- 
2.20.1


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Fwd: libstdc++: Attempt to resolve PR83562

[Liu Hao]

[-- Attachment #1.1: Type: text/plain, Size: 801 bytes --]

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\x0f
> 
> 



-- 
Best regards,
LH_Mouse


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: Fwd: libstdc++: Attempt to resolve PR83562

[Martin Storsjö]

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

[Jonathan Yong]

[-- Attachment #1.1.1: Type: text/plain, Size: 931 bytes --]

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.

[-- Attachment #1.1.2: OpenPGP_0x713B5FE29C145D45_and_old_rev.asc --]
[-- Type: application/pgp-keys, Size: 8035 bytes --]

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 840 bytes --]