[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331 Jonathan Wakely changed: What|Removed |Added Status|ASSIGNED|RESOLVED Resolution|--- |FIXED --- Comment #7 from Jonathan Wakely --- Should be fixed now.
[Bug libstdc++/108331] [13 Regression] ABI break of std::__c_file and std::fstream for win32 thread model of GCC for windows
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331 --- Comment #6 from CVS Commits --- The master branch has been updated by Jonathan Wakely : https://gcc.gnu.org/g:e2fc12a5dafadf15d804e1d2541528296e97a847 commit r13-5143-ge2fc12a5dafadf15d804e1d2541528296e97a847 Author: Jonathan Wakely Date: Thu Jan 12 10:40:49 2023 + 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331 Jonathan Wakely 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331 Richard Biener 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331 --- Comment #5 from Eric Botcazou --- > 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331
--- Comment #4 from Jonathan Wakely ---
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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331 --- Comment #3 from Eric Botcazou --- > 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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331
--- Comment #2 from Jonathan Wakely ---
(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
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=108331
Jonathan Wakely 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 ---
(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;
