[Bug libstdc++/108331] New: ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[Bug libstdc++/108331] New: ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[unlvsur at live dot com]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331

            Bug ID: 108331
           Summary: ABI break of std::__c_file and std::fstream for win32
                    thread model of GCC for windows
           Product: gcc
           Version: 13.0
            Status: UNCONFIRMED
          Severity: normal
          Priority: P3
         Component: libstdc++
          Assignee: unassigned at gcc dot gnu.org
          Reporter: unlvsur at live dot com
  Target Milestone: ---

The recent win32 thread model changes cause abi break of std::fstream due to
layout silently change.

I do not understand why you need a mutex for std::fstream. Now win32, posix and
mcf thread model they cause abi issues for this due to that lock. Can we unify
the abi for windows here?

[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |ebotcazou at gcc dot gnu.org
   Last reconfirmed|                            |2023-01-07
     Ever confirmed|0                           |1
            Summary|ABI break of std::__c_file  |[13 Regression] ABI break
                   |and std::fstream for win32  |of std::__c_file and
                   |thread model of GCC for     |std::fstream for win32
                   |windows                     |thread model of GCC for
                   |                            |windows
   Target Milestone|---                         |13.0
           Keywords|                            |ABI
             Status|UNCONFIRMED                 |NEW

--- Comment #1 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to cqwrteur from comment #0)
> I do not understand why you need a mutex for std::fstream.

It is there for historical reasons, it's completely unused.

> Now win32, posix
> and mcf thread model they cause abi issues for this due to that lock. Can we
> unify the abi for windows here?

No. The thread model is already ABI-changing, there is no way to make
std::mutex ABI compatible when it has a wildly different implementation.

But the std::basic_filebuf::_M_lock member does indeed change layout for the
win32 thread model between GCC 12 and GCC 13. We can't make the ABI consistent
between different thread models, but we do want it to be consistent for a
single thread model.

Since that mutex is unused anyway, we should just define it consistently with
GCC 12 for the win32 thread model, i.e. as a struct like:

typedef struct {
  long counter;
  void *sema;
} __gthread_mutex_t;

Eric, can we add a new macro to gthr-win32.h that says "this is the win32
thread model" which libstdc++ can then use like so:

--- a/libstdc++-v3/config/io/c_io_stdio.h
+++ b/libstdc++-v3/config/io/c_io_stdio.h
@@ -39,7 +39,17 @@ namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION

+#ifdef __GTHREAD_WIN32
+  // The layout of __gthread_mutex_t changed in GCC 13, but libstdc++ doesn't
+  // actually use the basic_filebuf::_M_lock member, so define it consistently
+  // with the old __gthread_mutex_t to avoid an unnecessary layout change:
+  struct __c_lock {
+    long counter;
+    void *sema;
+  };
+#else
   typedef __gthread_mutex_t __c_lock;
+#endif

   // for basic_file.h
   typedef FILE __c_file;

[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331

--- Comment #2 from Jonathan Wakely <redi at gcc dot gnu.org> ---
(In reply to Jonathan Wakely from comment #1)
> --- a/libstdc++-v3/config/io/c_io_stdio.h
> +++ b/libstdc++-v3/config/io/c_io_stdio.h
> @@ -39,7 +39,17 @@ namespace std _GLIBCXX_VISIBILITY(default)
>  {
>  _GLIBCXX_BEGIN_NAMESPACE_VERSION
>  
> +#ifdef __GTHREAD_WIN32
> +  // The layout of __gthread_mutex_t changed in GCC 13, but libstdc++
> doesn't
> +  // actually use the basic_filebuf::_M_lock member, so define it
> consistently
> +  // with the old __gthread_mutex_t to avoid an unnecessary layout change:
> +  struct __c_lock {
> +    long counter;
> +    void *sema;

We should use reserved names here, like __unused1 and __unused2.

> +  };
> +#else
>    typedef __gthread_mutex_t __c_lock;
> +#endif
>  
>    // for basic_file.h
>    typedef FILE __c_file;

[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331

--- Comment #3 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> Eric, can we add a new macro to gthr-win32.h that says "this is the win32
> thread model" which libstdc++ can then use like so:

Sure, but can't we define a __GTHREAD_TYPE_FOR_HISTORICAL_MUTEX macro instead
that would expand to a __gthread_historical_mutex_t defined in gthr-win32.h?
Or something along these lines or with better monikers.

[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331

--- Comment #4 from Jonathan Wakely <redi at gcc dot gnu.org> ---
That would be more flexible, but I'm not sure this is a problem that will
happen again. This is a particular case where we have a completely unused
member variable that just needs a specific layout.

It occurs to me libstdc++ could just check the GCC_GTHR_WIN32_H include guard,
although an explicitly-defined macro with a documented purpose (even if just
via a comment) would be less fragile.

How's this?

diff --git a/libgcc/config/i386/gthr-win32.h b/libgcc/config/i386/gthr-win32.h
index 146357fa436..050c7a21fcc 100644
--- a/libgcc/config/i386/gthr-win32.h
+++ b/libgcc/config/i386/gthr-win32.h
@@ -381,6 +381,14 @@ typedef struct timespec __gthread_time_t;
 #define __GTHREAD_COND_INIT_FUNCTION __gthread_cond_init_function
 #define __GTHREAD_TIME_INIT {0, 0}

+// Libstdc++ std::basic_filebuf needs the old definition of __gthread_mutex_t
+// for layout purposes, but doesn't actually use it.
+typedef struct {
+  long __unused1;
+  void *__unused2;
+} __gthr_win32_legacy_mutex_t;
+#define __GTHREAD_LEGACY_MUTEX_T __gthr_win32_legacy_mutex_t
+
 #if defined (_WIN32) && !defined(__CYGWIN__)
 #define MINGW32_SUPPORTS_MT_EH 1
 /* Mingw runtime >= v0.3 provides a magic variable that is set to nonzero
diff --git a/libstdc++-v3/config/io/c_io_stdio.h
b/libstdc++-v3/config/io/c_io_stdio.h
index 1a5e05a844a..e9e6e3ef4d8 100644
--- a/libstdc++-v3/config/io/c_io_stdio.h
+++ b/libstdc++-v3/config/io/c_io_stdio.h
@@ -39,7 +39,14 @@ namespace std _GLIBCXX_VISIBILITY(default)
 {
 _GLIBCXX_BEGIN_NAMESPACE_VERSION

+#ifdef __GTHREAD_LEGACY_MUTEX_T
+  // The layout of __gthread_mutex_t changed in GCC 13, but libstdc++ doesn't
+  // actually use the basic_filebuf::_M_lock member, so define it consistently
+  // with the old __gthread_mutex_t to avoid an unnecessary layout change:
+  typedef __GTHREAD_LEGACY_MUTEX_T __c_lock;
+#else
   typedef __gthread_mutex_t __c_lock;
+#endif

   // for basic_file.h
   typedef FILE __c_file;

[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[ebotcazou at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331

--- Comment #5 from Eric Botcazou <ebotcazou at gcc dot gnu.org> ---
> How's this?

This looks good to me, thanks!

[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[rguenth at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331

Richard Biener <rguenth at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
           Priority|P3                          |P1

[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|NEW                         |ASSIGNED
           Assignee|unassigned at gcc dot gnu.org      |redi at gcc dot gnu.org

[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[cvs-commit at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331

--- Comment #6 from CVS Commits <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Jonathan Wakely <redi@gcc.gnu.org>:

https://gcc.gnu.org/g:e2fc12a5dafadf15d804e1d2541528296e97a847

commit r13-5143-ge2fc12a5dafadf15d804e1d2541528296e97a847
Author: Jonathan Wakely <jwakely@redhat.com>
Date:   Thu Jan 12 10:40:49 2023 +0000

    libstdc++: Fix unintended layout change to std::basic_filebuf [PR108331]

    GCC 13 has a new implementation of gthr-win32.h which supports C++11
    mutexes, threads etc. but this causes an unintended ABI break. The
    __gthread_mutex_t type is always used in std::basic_filebuf even in
    C++98, so independent of whether C++11 sync primitives work or not.
    Because that type changed for the win32 thread model, we have a layout
    change in std::basic_filebuf. The member is completely unused, it just
    gets passed to the std::__basic_file constructor and ignored. So we
    don't need that mutex to actually work, we just need its layout to not
    change.

    Introduce a new __gthr_win32_legacy_mutex_t struct in gthr-win32.h with
    the old layout, and conditionally use that in std::basic_filebuf.

            PR libstdc++/108331

    libgcc/ChangeLog:

            * config/i386/gthr-win32.h (__gthr_win32_legacy_mutex_t): New
            struct matching the previous __gthread_mutex_t struct.
            (__GTHREAD_LEGACY_MUTEX_T): Define.

    libstdc++-v3/ChangeLog:

            * config/io/c_io_stdio.h (__c_lock): Define as a typedef for
            __GTHREAD_LEGACY_MUTEX_T if defined.

[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows

[redi at gcc dot gnu.org]

https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331

Jonathan Wakely <redi at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|ASSIGNED                    |RESOLVED
         Resolution|---                         |FIXED

--- Comment #7 from Jonathan Wakely <redi at gcc dot gnu.org> ---
Should be fixed now.