Bug 29373

Summary: Canadian Compilation Failed! gdbsupport/packed.h:41:40: error: static assertion failed for x86_64-w64-mingw32 host.
Product: gdb Reporter: cqwrteur <euloanty>
Component: buildAssignee: Not yet assigned to anyone <unassigned>
Status: RESOLVED FIXED    
Severity: normal CC: euloanty, palves, pedro, tromey, vries
Priority: P1    
Version: HEAD   
Target Milestone: 13.1   
Host: x86_64-w64-mingw32 Target: i586-msdosdjgpp
Build: x86_64-linux-gnu Last reconfirmed: 2022-07-18 00:00:00
Attachments: build failure
config file

Description cqwrteur 2022-07-16 20:52:20 UTC
Created attachment 14214 [details]
build failure

gdbsupport's static_assert breaks x86_64-w64-mingw32 host.
Comment 1 cqwrteur 2022-07-16 20:53:40 UTC
Created attachment 14215 [details]
config file
Comment 2 Tom Tromey 2022-07-18 18:33:09 UTC
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.
Comment 3 Tom de Vries 2022-07-18 21:18:27 UTC
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.
Comment 4 Pedro Alves 2022-07-18 23:34:02 UTC
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
Comment 5 cqwrteur 2022-07-19 04:21:47 UTC
(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
Comment 6 Pedro Alves 2022-07-19 11:49:56 UTC
> 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.
Comment 7 Pedro Alves 2022-07-19 14:48:14 UTC
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.
Comment 8 Pedro Alves 2022-07-19 14:48:53 UTC
... I'll have TIME to post this ...
Comment 9 Tom de Vries 2022-07-21 12:24:07 UTC
(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 (==)
 ^
...
Comment 10 Pedro Alves 2022-07-21 13:45:05 UTC
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...
Comment 11 Pedro Alves 2022-07-21 14:34:29 UTC
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.
Comment 12 Tom de Vries 2022-07-21 14:53:30 UTC
(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
...
Comment 13 Pedro Alves 2022-07-21 15:27:22 UTC
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.
Comment 14 Sourceware Commits 2022-07-25 15:12:16 UTC
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
Comment 15 Pedro Alves 2022-07-25 15:20:25 UTC
Should be fixed now.