Created attachment 14214 [details] build failure gdbsupport's static_assert breaks x86_64-w64-mingw32 host.
Created attachment 14215 [details] config file
Pedro, Tom -- FWIW we encountered this internally @ AdaCore as well. The sizeof assert fails, I guess (I didn't look) due to a difference in the Windows ABI. It seems to me that just removing the assert is probably fine.
I wonder if this is indeed an abi issue, or if it's basically complaining that it's missing the alternative implementation of the the packed template, not relying on a packed attribute implementation, that Pedro initially proposed.
I can reproduce this here, cross building with mingw on Fedora. This is indeed an ABI issue -- mingw defaults to "-mms-bitfields", which affects how bitfields are laid out. Luckily, there's an attribute to switch ABI on a per struct basis. See: https://gcc.gnu.org/onlinedocs/gcc/x86-Variable-Attributes.html This fixes the build for me: diff --git c/gdbsupport/packed.h w/gdbsupport/packed.h index 3468cf44207..a3389afdfb7 100644 --- c/gdbsupport/packed.h +++ w/gdbsupport/packed.h @@ -20,6 +20,12 @@ #include "traits.h" +#ifdef __MINGW32__ +# define ATTRIBUTE_GCC_STRUCT __attribute__((gcc_struct)) +#else +# define ATTRIBUTE_GCC_STRUCT +#endif + /* Each instantiation and full specialization of the packed template defines a type that behaves like a given scalar type, but that has byte alignment, and, may optionally have a smaller size than the @@ -58,7 +64,7 @@ struct packed private: T m_val : (Bytes * HOST_CHAR_BIT) ATTRIBUTE_PACKED; -}; +} ATTRIBUTE_GCC_STRUCT; /* Add some comparisons between std::atomic<packed<T>> and T. We need this because the regular comparisons would require two implicit
(In reply to Pedro Alves from comment #4) > I can reproduce this here, cross building with mingw on Fedora. > > This is indeed an ABI issue -- mingw defaults to "-mms-bitfields", which > affects how bitfields are laid out. Luckily, there's an attribute to switch > ABI on a per struct basis. See: > > https://gcc.gnu.org/onlinedocs/gcc/x86-Variable-Attributes.html > > This fixes the build for me: > > diff --git c/gdbsupport/packed.h w/gdbsupport/packed.h > index 3468cf44207..a3389afdfb7 100644 > --- c/gdbsupport/packed.h > +++ w/gdbsupport/packed.h > @@ -20,6 +20,12 @@ > > #include "traits.h" > > +#ifdef __MINGW32__ > +# define ATTRIBUTE_GCC_STRUCT __attribute__((gcc_struct)) > +#else > +# define ATTRIBUTE_GCC_STRUCT > +#endif > + > /* Each instantiation and full specialization of the packed template > defines a type that behaves like a given scalar type, but that has > byte alignment, and, may optionally have a smaller size than the > @@ -58,7 +64,7 @@ struct packed > > private: > T m_val : (Bytes * HOST_CHAR_BIT) ATTRIBUTE_PACKED; > -}; > +} ATTRIBUTE_GCC_STRUCT; > > /* Add some comparisons between std::atomic<packed<T>> and T. We need > this because the regular comparisons would require two implicit Also cygwin and other pe targets. you probably need #if __has_cpp_attribute(__gnu__::__gcc_struct__) #define ATTRIBUTE_GCC_STRUCT [[__gnu__::__gcc_struct__]] #endif something like that to detect the feature
> Also cygwin and other pe targets. Does Cygwin default to "-mms-bitfields" too? What other PE hosts are there? (It's hosts that matter here, not targets.) > #if __has_cpp_attribute(__gnu__::__gcc_struct__) > #define ATTRIBUTE_GCC_STRUCT [[__gnu__::__gcc_struct__]] > #endif An issue with that is that __has_attribute and __has_cpp_attribute were added in GCC 5, while supposedly the minimum supported GCC version for building GDB is GCC 4.8. I have no idea whether anyone out there would still build GDB for Windows using that version of GCC, though. Explicitly checking mingw/cygwin would work there too, as __gcc_struct__ was already supported back then.
Googling around, I found that Clang for Windows does not support the gcc_struct attribute. E.g., here: https://code.videolan.org/videolan/libdvdread/-/issues/19 with that in mind, I added back (or rather, reimplemented), the fallback implementation using byte arrays, and make _WIN32 + clang use it. To make sure it all behaves as intended, I wrote some units tests. I don't think I'll have to post this to the list today, so I pushed it to the users/palves/packed branch on sourceware.
... I'll have TIME to post this ...
(In reply to Pedro Alves from comment #7) > ... I pushed it to > the users/palves/packed branch on sourceware. I've build the patch with gcc 4.8.5, and ran into: ... In file included from gdb/unittests/packed-selftests.c:22:0: gdbsupport/packed.h:120:27: error: ‘atomic’ in namespace ‘std’ does not name a type bool operator OP (const std::atomic<packed<T, Bytes>> &lhs, \ ^ gdbsupport/packed.h:152:1: note: in expansion of macro ‘PACKED_ATOMIC_OP’ PACKED_ATOMIC_OP (==) ^ ...
Thanks, I'll try to reproduce it. It's probably just missing #include <atomic>. BTW, I tried building unittests/packed-selftests.c with "clang -target x86_64-pc-windows-gnu", and turns out that even though clang doesn't support gcc_struct, it actually doesn't need it, the static asserts pass without it. So I'm dropping the array based implementation, again...
LOL, neverending story. Clang 10 doesn't need the alternative implementation, but clang 14 does. Maybe Clang didn't implement MS bitfields properly in version 10 yet.
(In reply to Pedro Alves from comment #10) > Thanks, I'll try to reproduce it. It's probably just missing #include > <atomic>. > Ack, managed to build with gcc 4.8.5 and: ... diff --git a/gdbsupport/packed.h b/gdbsupport/packed.h index d65e0bc33f0..097bd493fa4 100644 --- a/gdbsupport/packed.h +++ b/gdbsupport/packed.h @@ -18,6 +18,7 @@ #ifndef PACKED_H #define PACKED_H +#include <atomic> #include "traits.h" /* Each instantiation and full specialization of the packed template ...
Thanks. Posted here now: [PATCH 0/3] struct packed and Windows ports (PR build/29373) https://sourceware.org/pipermail/gdb-patches/2022-July/190957.html I've confirmed this builds with gcc version 4.8.5, too.
The master branch has been updated by Pedro Alves <palves@sourceware.org>: https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=e249e6b8012ea0a14e5768d31becd7b4caff8e77 commit e249e6b8012ea0a14e5768d31becd7b4caff8e77 Author: Pedro Alves <pedro@palves.net> Date: Tue Jul 19 00:26:33 2022 +0100 struct packed: Unit tests and more operators For PR gdb/29373, I wrote an alternative implementation of struct packed that uses a gdb_byte array for internal representation, needed for mingw+clang. While adding that, I wrote some unit tests to make sure both implementations behave the same. While at it, I implemented all relational operators. This commit adds said unit tests and relational operators. The alternative gdb_byte array implementation will come next. Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29373 Change-Id: I023315ee03622c59c397bf4affc0b68179c32374
Should be fixed now.