[PATCH 0/3] struct packed and Windows ports (PR build/29373)

[PATCH 0/3] struct packed and Windows ports (PR build/29373)

[Pedro Alves]

PR build/29373 points out that mingw GDB builds are brokenly current,
failing static assertions in gdbsupport/packed.h.  This series fixes
it, for both mingw + gcc, and mingw + clang.

Pedro Alves (3):
  struct packed: Use gcc_struct on Windows
  struct packed: Unit tests and more operators
  struct packed: Add fallback byte array implementation

 gdb/Makefile.in                  |   1 +
 gdb/unittests/packed-selftests.c | 132 ++++++++++++++++++++++++++++++
 gdbsupport/packed.h              | 134 ++++++++++++++++++++++++-------
 3 files changed, 236 insertions(+), 31 deletions(-)
 create mode 100644 gdb/unittests/packed-selftests.c


base-commit: d65edaa0bc3f24537ecde3735b1fa041f36f4ab8
-- 
2.36.0

[PATCH 1/3] struct packed: Use gcc_struct on Windows

[Pedro Alves]

Building GDB on mingw/gcc hosts is currently broken, due to a static
assertion failure in gdbsupport/packed.h:

  In file included from ../../../../../binutils-gdb/gdb/../gdbsupport/common-defs.h:201,
		   from ../../../../../binutils-gdb/gdb/defs.h:28,
		   from ../../../../../binutils-gdb/gdb/dwarf2/read.c:31:
  ../../../../../binutils-gdb/gdb/../gdbsupport/packed.h: In instantiation of 'packed<T, Bytes>::packed(T) [with T = dwarf_unit_type; long long unsigned int Bytes = 1]':
  ../../../../../binutils-gdb/gdb/dwarf2/read.h:181:74:   required from here
  ../../../../../binutils-gdb/gdb/../gdbsupport/packed.h:41:40: error: static assertion failed
     41 |     gdb_static_assert (sizeof (packed) == Bytes);
	|                        ~~~~~~~~~~~~~~~~^~~~~~~~
  ../../../../../binutils-gdb/gdb/../gdbsupport/gdb_assert.h:27:48: note: in definition of macro 'gdb_static_assert'
     27 | #define gdb_static_assert(expr) static_assert (expr, "")
	|                                                ^~~~
  ../../../../../binutils-gdb/gdb/../gdbsupport/packed.h:41:40: note: the comparison reduces to '(4 == 1)'
     41 |     gdb_static_assert (sizeof (packed) == Bytes);
	|                        ~~~~~~~~~~~~~~~~^~~~~~~~


The issue is that mingw gcc defaults to "-mms-bitfields", which
affects how bitfields are laid out.  We can however tell GCC that we
want the regular GCC layout instead using attribute gcc_struct.

Attribute gcc_struct is not implemented in "clang -target
x86_64-pc-windows-gnu", so that will need a different fix.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29373

Change-Id: I023315ee03622c59c397bf4affc0b68179c32374
---
 gdbsupport/packed.h | 10 +++++++++-
 1 file changed, 9 insertions(+), 1 deletion(-)

diff --git a/gdbsupport/packed.h b/gdbsupport/packed.h
index 3468cf44207..53164a9e0c3 100644
--- a/gdbsupport/packed.h
+++ b/gdbsupport/packed.h
@@ -27,8 +27,16 @@
    bit-fields (and ENUM_BITFIELD), when the fields must have separate
    memory locations to avoid data races.  */
 
+/* We need gcc_struct on Windows GCC, as otherwise the size of e.g.,
+   "packed<int, 1>" will be larger than what we want.  */
+#if defined _WIN32
+# define ATTRIBUTE_GCC_STRUCT __attribute__((__gcc_struct__))
+#else
+# define ATTRIBUTE_GCC_STRUCT
+#endif
+
 template<typename T, size_t Bytes = sizeof (T)>
-struct packed
+struct ATTRIBUTE_GCC_STRUCT packed
 {
 public:
   packed () noexcept = default;
-- 
2.36.0

[PATCH 2/3] struct packed: Unit tests and more operators

[Pedro Alves]

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
---
 gdb/Makefile.in                  |   1 +
 gdb/unittests/packed-selftests.c | 132 +++++++++++++++++++++++++++++++
 gdbsupport/packed.h              |  79 +++++++++++-------
 3 files changed, 182 insertions(+), 30 deletions(-)
 create mode 100644 gdb/unittests/packed-selftests.c

diff --git a/gdb/Makefile.in b/gdb/Makefile.in
index 57c29a78b7a..aebb7dc5ea3 100644
--- a/gdb/Makefile.in
+++ b/gdb/Makefile.in
@@ -464,6 +464,7 @@ SELFTESTS_SRCS = \
 	unittests/offset-type-selftests.c \
 	unittests/observable-selftests.c \
 	unittests/optional-selftests.c \
+	unittests/packed-selftests.c \
 	unittests/parallel-for-selftests.c \
 	unittests/parse-connection-spec-selftests.c \
 	unittests/path-join-selftests.c \
diff --git a/gdb/unittests/packed-selftests.c b/gdb/unittests/packed-selftests.c
new file mode 100644
index 00000000000..3438a5a2555
--- /dev/null
+++ b/gdb/unittests/packed-selftests.c
@@ -0,0 +1,132 @@
+/* Self tests for packed for GDB, the GNU debugger.
+
+   Copyright (C) 2022 Free Software Foundation, Inc.
+
+   This file is part of GDB.
+
+   This program is free software; you can redistribute it and/or modify
+   it under the terms of the GNU General Public License as published by
+   the Free Software Foundation; either version 3 of the License, or
+   (at your option) any later version.
+
+   This program is distributed in the hope that it will be useful,
+   but WITHOUT ANY WARRANTY; without even the implied warranty of
+   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
+   GNU General Public License for more details.
+
+   You should have received a copy of the GNU General Public License
+   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
+
+#include "defs.h"
+#include "gdbsupport/selftest.h"
+#include "gdbsupport/packed.h"
+
+namespace selftests {
+namespace packed_tests {
+
+enum test_enum
+{
+  TE_A = 1,
+  TE_B = 2,
+  TE_C = 3,
+  TE_D = 4,
+};
+
+gdb_static_assert (sizeof (packed<test_enum, 1>) == 1);
+gdb_static_assert (sizeof (packed<test_enum, 2>) == 2);
+gdb_static_assert (sizeof (packed<test_enum, 3>) == 3);
+gdb_static_assert (sizeof (packed<test_enum, 4>) == 4);
+
+gdb_static_assert (alignof (packed<test_enum, 1>) == 1);
+gdb_static_assert (alignof (packed<test_enum, 2>) == 1);
+gdb_static_assert (alignof (packed<test_enum, 3>) == 1);
+gdb_static_assert (alignof (packed<test_enum, 4>) == 1);
+
+/* Triviality checks.  */
+#define CHECK_TRAIT(TRAIT)			\
+  static_assert (std::TRAIT<packed<test_enum, 1>>::value, "")
+
+#if HAVE_IS_TRIVIALLY_COPYABLE
+
+CHECK_TRAIT (is_trivially_copyable);
+CHECK_TRAIT (is_trivially_copy_constructible);
+CHECK_TRAIT (is_trivially_move_constructible);
+CHECK_TRAIT (is_trivially_copy_assignable);
+CHECK_TRAIT (is_trivially_move_assignable);
+
+#endif
+
+#undef CHECK_TRAIT
+
+/* Entry point.  */
+
+static void
+run_tests ()
+{
+  typedef packed<unsigned int, 2> packed_2;
+
+  packed_2 p1;
+  packed_2 p2 (0x0102);
+  p1 = 0x0102;
+
+  SELF_CHECK (p1 == p1);
+  SELF_CHECK (p1 == p2);
+  SELF_CHECK (p1 == 0x0102);
+  SELF_CHECK (0x0102 == p1);
+
+  SELF_CHECK (p1 != 0);
+  SELF_CHECK (0 != p1);
+
+  SELF_CHECK (p1 != 0x0103);
+  SELF_CHECK (0x0103 != p1);
+
+  SELF_CHECK (p1 != 0x01020102);
+  SELF_CHECK (0x01020102 != p1);
+
+  SELF_CHECK (p1 != 0x01020000);
+  SELF_CHECK (0x01020000 != p1);
+
+  /* Check truncation.  */
+  p1 = 0x030102;
+  SELF_CHECK (p1 == 0x0102);
+  SELF_CHECK (p1 != 0x030102);
+
+  /* Check that the custom std::atomic/packed/T relational operators
+     work as intended.  No need for fully comprehensive tests, as all
+     operators are defined in the same way, via a macro.  We just want
+     to make sure that we can compare atomic-wrapped packed, with
+     packed, and with the packed underlying type.  */
+
+  std::atomic<packed<unsigned int, 2>> atomic_packed_2 (0x0102);
+
+  SELF_CHECK (atomic_packed_2 == atomic_packed_2);
+  SELF_CHECK (atomic_packed_2 == p1);
+  SELF_CHECK (p1 == atomic_packed_2);
+  SELF_CHECK (atomic_packed_2 == 0x0102u);
+  SELF_CHECK (0x0102u == atomic_packed_2);
+
+  SELF_CHECK (atomic_packed_2 >= 0x0102u);
+  SELF_CHECK (atomic_packed_2 <= 0x0102u);
+  SELF_CHECK (atomic_packed_2 > 0u);
+  SELF_CHECK (atomic_packed_2 < 0x0103u);
+  SELF_CHECK (atomic_packed_2 >= 0u);
+  SELF_CHECK (atomic_packed_2 <= 0x0102u);
+  SELF_CHECK (!(atomic_packed_2 > 0x0102u));
+  SELF_CHECK (!(atomic_packed_2 < 0x0102u));
+
+  /* Check std::atomic<packed> truncation behaves the same as without
+     std::atomic.  */
+  atomic_packed_2 = 0x030102;
+  SELF_CHECK (atomic_packed_2 == 0x0102u);
+  SELF_CHECK (atomic_packed_2 != 0x030102u);
+}
+
+} /* namespace packed_tests */
+} /* namespace selftests */
+
+void _initialize_packed_selftests ();
+void
+_initialize_packed_selftests ()
+{
+  selftests::register_test ("packed", selftests::packed_tests::run_tests);
+}
diff --git a/gdbsupport/packed.h b/gdbsupport/packed.h
index 53164a9e0c3..d721b02c056 100644
--- a/gdbsupport/packed.h
+++ b/gdbsupport/packed.h
@@ -19,6 +19,7 @@
 #define PACKED_H
 
 #include "traits.h"
+#include <atomic>
 
 /* Each instantiation and full specialization of the packed template
    defines a type that behaves like a given scalar type, but that has
@@ -68,37 +69,55 @@ struct ATTRIBUTE_GCC_STRUCT packed
   T m_val : (Bytes * HOST_CHAR_BIT) ATTRIBUTE_PACKED;
 };
 
-/* Add some comparisons between std::atomic<packed<T>> and T.  We need
-   this because the regular comparisons would require two implicit
-   conversions to go from T to std::atomic<packed<T>>:
-
-     T         -> packed<T>
-     packed<T> -> std::atomic<packed<T>>
-
-   and C++ only does one.  */
-
-template<typename T, size_t Bytes>
-bool operator== (T lhs, const std::atomic<packed<T, Bytes>> &rhs)
-{
-  return lhs == rhs.load ();
-}
-
-template<typename T, size_t Bytes>
-bool operator== (const std::atomic<packed<T, Bytes>> &lhs, T rhs)
-{
-  return lhs.load () == rhs;
-}
+/* Add some comparisons between std::atomic<packed<T>> and packed<T>
+   and T.  We need this because even though std::atomic<T> doesn't
+   define these operators, the relational expressions still work via
+   implicit conversions.  Those wouldn't work when wrapped in packed
+   without these operators, because they'd require two implicit
+   conversions to go from T to packed<T> to std::atomic<packed<T>>
+   (and back), and C++ only does one.  */
+
+#define PACKED_ATOMIC_OP(OP)						\
+  template<typename T, size_t Bytes>					\
+  bool operator OP (const std::atomic<packed<T, Bytes>> &lhs,		\
+		    const std::atomic<packed<T, Bytes>> &rhs)		\
+  {									\
+    return lhs.load () OP rhs.load ();					\
+  }									\
+									\
+  template<typename T, size_t Bytes>					\
+  bool operator OP (T lhs, const std::atomic<packed<T, Bytes>> &rhs)	\
+  {									\
+    return lhs OP rhs.load ();						\
+  }									\
+									\
+  template<typename T, size_t Bytes>					\
+  bool operator OP (const std::atomic<packed<T, Bytes>> &lhs, T rhs)	\
+  {									\
+    return lhs.load () OP rhs;						\
+  }									\
+									\
+  template<typename T, size_t Bytes>					\
+  bool operator OP (const std::atomic<packed<T, Bytes>> &lhs,		\
+		    packed<T, Bytes> rhs)				\
+  {									\
+    return lhs.load () OP rhs;						\
+  }									\
+									\
+  template<typename T, size_t Bytes>					\
+  bool operator OP (packed<T, Bytes> lhs,				\
+		    const std::atomic<packed<T, Bytes>> &rhs)		\
+  {									\
+    return lhs OP rhs.load ();						\
+  }
 
-template<typename T, size_t Bytes>
-bool operator!= (T lhs, const std::atomic<packed<T, Bytes>> &rhs)
-{
-  return !(lhs == rhs);
-}
+PACKED_ATOMIC_OP (==)
+PACKED_ATOMIC_OP (!=)
+PACKED_ATOMIC_OP (>)
+PACKED_ATOMIC_OP (<)
+PACKED_ATOMIC_OP (>=)
+PACKED_ATOMIC_OP (<=)
 
-template<typename T, size_t Bytes>
-bool operator!= (const std::atomic<packed<T, Bytes>> &lhs, T rhs)
-{
-  return !(lhs == rhs);
-}
+#undef PACKED_ATOMIC_OP
 
 #endif
-- 
2.36.0

[PATCH 3/3] struct packed: Add fallback byte array implementation

[Pedro Alves]

Attribute gcc_struct is not implemented in Clang targeting Windows, so
add a fallback standard-conforming implementation based on arrays.

I ran the testsuite on x86_64 GNU/Linux with this implementation
forced, and saw no regressions.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29373

Change-Id: I023315ee03622c59c397bf4affc0b68179c32374
---
 gdbsupport/packed.h | 51 ++++++++++++++++++++++++++++++++++++++++++---
 1 file changed, 48 insertions(+), 3 deletions(-)

diff --git a/gdbsupport/packed.h b/gdbsupport/packed.h
index d721b02c056..dbece2a29aa 100644
--- a/gdbsupport/packed.h
+++ b/gdbsupport/packed.h
@@ -28,9 +28,27 @@
    bit-fields (and ENUM_BITFIELD), when the fields must have separate
    memory locations to avoid data races.  */
 
-/* We need gcc_struct on Windows GCC, as otherwise the size of e.g.,
-   "packed<int, 1>" will be larger than what we want.  */
-#if defined _WIN32
+/* There are two implementations here -- one standard compliant, using
+   a byte array for internal representation, and another that relies
+   on bitfields and attribute packed (and attribute gcc_struct on
+   Windows).  The latter is preferable, as it is more convenient when
+   debugging GDB -- printing a struct packed variable prints its field
+   using its natural type, which is particularly useful if the type is
+   an enum -- but may not work on all compilers.  */
+
+/* Clang targeting Windows does not support attribute gcc_struct, so
+   we use the alternative byte array implemention there. */
+#if defined _WIN32 && defined __clang__
+# define PACKED_USE_ARRAY 1
+#else
+# define PACKED_USE_ARRAY 0
+#endif
+
+/* For the preferred implementation, we need gcc_struct on Windows, as
+   otherwise the size of e.g., "packed<int, 1>" will be larger than
+   what we want.  Clang targeting Windows does not support attribute
+   gcc_struct.  */
+#if !PACKED_USE_ARRAY && defined _WIN32 && !defined __clang__
 # define ATTRIBUTE_GCC_STRUCT __attribute__((__gcc_struct__))
 #else
 # define ATTRIBUTE_GCC_STRUCT
@@ -44,7 +62,18 @@ struct ATTRIBUTE_GCC_STRUCT packed
 
   packed (T val)
   {
+    gdb_static_assert (sizeof (ULONGEST) >= sizeof (T));
+
+#if PACKED_USE_ARRAY
+    ULONGEST tmp = val;
+    for (int i = (Bytes - 1); i >= 0; --i)
+      {
+	m_bytes[i] = (gdb_byte) tmp;
+	tmp >>= HOST_CHAR_BIT;
+      }
+#else
     m_val = val;
+#endif
 
     /* Ensure size and aligment are what we expect.  */
     gdb_static_assert (sizeof (packed) == Bytes);
@@ -62,11 +91,27 @@ struct ATTRIBUTE_GCC_STRUCT packed
 
   operator T () const noexcept
   {
+#if PACKED_USE_ARRAY
+    ULONGEST tmp = 0;
+    for (int i = 0;;)
+      {
+	tmp |= m_bytes[i];
+	if (++i == Bytes)
+	  break;
+	tmp <<= HOST_CHAR_BIT;
+      }
+    return (T) tmp;
+#else
     return m_val;
+#endif
   }
 
 private:
+#if PACKED_USE_ARRAY
+  gdb_byte m_bytes[Bytes];
+#else
   T m_val : (Bytes * HOST_CHAR_BIT) ATTRIBUTE_PACKED;
+#endif
 };
 
 /* Add some comparisons between std::atomic<packed<T>> and packed<T>
-- 
2.36.0

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Eli Zaretskii]

> From: Pedro Alves <pedro@palves.net>
> Date: Thu, 21 Jul 2022 16:21:30 +0100
> 
> The issue is that mingw gcc defaults to "-mms-bitfields", which
> affects how bitfields are laid out.  We can however tell GCC that we
> want the regular GCC layout instead using attribute gcc_struct.

Is that a good idea?  It means the code emitted by GCC for this source
file will be incompatible with any other library compiled with MinGW
that GDB uses.  So we are risking ABI incompatibilities here.  Right?

Can you tell why we must have the regular GCC layout of bitfields
here?

Thanks.

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Pedro Alves]

On 2022-07-21 5:03 p.m., Eli Zaretskii wrote:
>> From: Pedro Alves <pedro@palves.net>
>> Date: Thu, 21 Jul 2022 16:21:30 +0100
>>
>> The issue is that mingw gcc defaults to "-mms-bitfields", which
>> affects how bitfields are laid out.  We can however tell GCC that we
>> want the regular GCC layout instead using attribute gcc_struct.
> 
> Is that a good idea?  It means the code emitted by GCC for this source
> file will be incompatible with any other library compiled with MinGW
> that GDB uses.  So we are risking ABI incompatibilities here.  Right?
> 

No.  The attribute only changes the layout of that particular structure.

> Can you tell why we must have the regular GCC layout of bitfields
> here?

Because without it the struct won't really be packed.

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <pedro@palves.net>
> Date: Thu, 21 Jul 2022 18:05:55 +0100
> 
> On 2022-07-21 5:03 p.m., Eli Zaretskii wrote:
> >> From: Pedro Alves <pedro@palves.net>
> >> Date: Thu, 21 Jul 2022 16:21:30 +0100
> >>
> >> The issue is that mingw gcc defaults to "-mms-bitfields", which
> >> affects how bitfields are laid out.  We can however tell GCC that we
> >> want the regular GCC layout instead using attribute gcc_struct.
> > 
> > Is that a good idea?  It means the code emitted by GCC for this source
> > file will be incompatible with any other library compiled with MinGW
> > that GDB uses.  So we are risking ABI incompatibilities here.  Right?
> > 
> 
> No.  The attribute only changes the layout of that particular structure.

And we are 110% sure that structure will never be passed to any other
code?

> > Can you tell why we must have the regular GCC layout of bitfields
> > here?
> 
> Because without it the struct won't really be packed.

Can you tell why is that necessary?

In any case, I'm very uneasy about changes that break ABI
compatibility between parts of a program.

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Pedro Alves]

On 2022-07-21 6:40 p.m., Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <pedro@palves.net>
>> Date: Thu, 21 Jul 2022 18:05:55 +0100
>>
>> On 2022-07-21 5:03 p.m., Eli Zaretskii wrote:
>>>> From: Pedro Alves <pedro@palves.net>
>>>> Date: Thu, 21 Jul 2022 16:21:30 +0100
>>>>
>>>> The issue is that mingw gcc defaults to "-mms-bitfields", which
>>>> affects how bitfields are laid out.  We can however tell GCC that we
>>>> want the regular GCC layout instead using attribute gcc_struct.
>>>
>>> Is that a good idea?  It means the code emitted by GCC for this source
>>> file will be incompatible with any other library compiled with MinGW
>>> that GDB uses.  So we are risking ABI incompatibilities here.  Right?
>>>
>>
>> No.  The attribute only changes the layout of that particular structure.
> 
> And we are 110% sure that structure will never be passed to any other
> code?

What other code are you talking about?  struct packed is used in GDB's internal
structures.  Nothing outside GDB ever sees it.  GDB doesn't export a C api.
And if it did, we probably wouldn't use struct packed in exported structures.

> 
>>> Can you tell why we must have the regular GCC layout of bitfields
>>> here?
>>
>> Because without it the struct won't really be packed.
> 
> Can you tell why is that necessary?
> 
> In any case, I'm very uneasy about changes that break ABI
> compatibility between parts of a program.
> 

But no ABI compatibility is broken.

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Pedro Alves]

On 2022-07-21 6:40 p.m., Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <pedro@palves.net>

>> Because without it the struct won't really be packed.
> 
> Can you tell why is that necessary?

Somehow I missed this question.  It is necessary because that's the whole
point of "struct packed".  To wrap some other type and pack it.  We use it
in some size-sensitive structures, to make them are as small as possible.
This is described in the gdbsupport/packed.h header.

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <pedro@palves.net>
> Date: Thu, 21 Jul 2022 19:15:17 +0100
> 
> >> No.  The attribute only changes the layout of that particular structure.
> > 
> > And we are 110% sure that structure will never be passed to any other
> > code?
> 
> What other code are you talking about?  struct packed is used in GDB's internal
> structures.  Nothing outside GDB ever sees it.  GDB doesn't export a C api.
> And if it did, we probably wouldn't use struct packed in exported structures.

If this is supposed to be based on our vigilance and manual prevention
of exporting it, I think it's fragile and not very reliable.  If we
forget or miss something, we get a subtly broken build.

> >>> Can you tell why we must have the regular GCC layout of bitfields
> >>> here?
> >>
> >> Because without it the struct won't really be packed.
> > 
> > Can you tell why is that necessary?

Can you answer this question, please?

What I'm actually asking is whether there's any alternative which
would avoid overriding the defaults in this matter.

> > In any case, I'm very uneasy about changes that break ABI
> > compatibility between parts of a program.
> 
> But no ABI compatibility is broken.

But it could be, even if today it isn't.

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <pedro@palves.net>
> Date: Thu, 21 Jul 2022 19:18:20 +0100
> 
> On 2022-07-21 6:40 p.m., Eli Zaretskii wrote:
> >> Cc: gdb-patches@sourceware.org
> >> From: Pedro Alves <pedro@palves.net>
> 
> >> Because without it the struct won't really be packed.
> > 
> > Can you tell why is that necessary?
> 
> Somehow I missed this question.  It is necessary because that's the whole
> point of "struct packed".  To wrap some other type and pack it.  We use it
> in some size-sensitive structures, to make them are as small as possible.
> This is described in the gdbsupport/packed.h header.

You mean, this part:

  /* 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
     given scalar type.  This is typically used as alternative to
     bit-fields (and ENUM_BITFIELD), when the fields must have separate
     memory locations to avoid data races.  */

I don't think I see the rationale here, except "make them as small as
possible".  Is that the reason?  If so, what would happen if the size
is somewhat larger?  If the only result is less efficient GDB, I'd be
happier if we sustained the efficiency hit, but stayed compatible to
the ABI.

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Pedro Alves]

On 2022-07-21 7:23 p.m., Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <pedro@palves.net>
>> Date: Thu, 21 Jul 2022 19:15:17 +0100
>>
>>>> No.  The attribute only changes the layout of that particular structure.
>>>
>>> And we are 110% sure that structure will never be passed to any other
>>> code?
>>
>> What other code are you talking about?  struct packed is used in GDB's internal
>> structures.  Nothing outside GDB ever sees it.  GDB doesn't export a C api.
>> And if it did, we probably wouldn't use struct packed in exported structures.
> 
> If this is supposed to be based on our vigilance and manual prevention
> of exporting it, I think it's fragile and not very reliable.  If we
> forget or miss something, we get a subtly broken build.
> 

I'm sorry, but this isn't making a lot of sense.  If we ever exposed a public
C API, we'd have to be very careful with _all_ the types we expose, wrt to ABI stability.

>>>>> Can you tell why we must have the regular GCC layout of bitfields
>>>>> here?
>>>>
>>>> Because without it the struct won't really be packed.
>>>
>>> Can you tell why is that necessary?
> 
> Can you answer this question, please?

Yes, sorry, I missed it before.  I replied in another email.

> 
> What I'm actually asking is whether there's any alternative which
> would avoid overriding the defaults in this matter.
> 
>>> In any case, I'm very uneasy about changes that break ABI
>>> compatibility between parts of a program.
>>
>> But no ABI compatibility is broken.
> 
> But it could be, even if today it isn't.
> 

I could also write a ton of other bugs.  What concern is this?

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Pedro Alves]

On 2022-07-21 7:28 p.m., Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <pedro@palves.net>
>> Date: Thu, 21 Jul 2022 19:18:20 +0100
>>
>> On 2022-07-21 6:40 p.m., Eli Zaretskii wrote:
>>>> Cc: gdb-patches@sourceware.org
>>>> From: Pedro Alves <pedro@palves.net>
>>
>>>> Because without it the struct won't really be packed.
>>>
>>> Can you tell why is that necessary?
>>
>> Somehow I missed this question.  It is necessary because that's the whole
>> point of "struct packed".  To wrap some other type and pack it.  We use it
>> in some size-sensitive structures, to make them are as small as possible.
>> This is described in the gdbsupport/packed.h header.
> 
> You mean, this part:
> 
>   /* 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
>      given scalar type.  This is typically used as alternative to
>      bit-fields (and ENUM_BITFIELD), when the fields must have separate
>      memory locations to avoid data races.  */
> 
> I don't think I see the rationale here, except "make them as small as
> possible".  Is that the reason?  

Well, we use bit fields in structs that need to be as small as possible.
But using bit fields is problematic in cases that lead to data races.
Instead of fixing the problem in every stop that might need it, we added
struct packed, to abstract it out.  Keeping the type as small is possible
is part of the point, otherwise we wouldn't have struct packed at all.

> If so, what would happen if the size
> is somewhat larger?  If the only result is less efficient GDB, I'd be
> happier if we sustained the efficiency hit, but stayed compatible to
> the ABI.
> 

I'm sorry, but that doesn't make sense.  This is used in GDB internals.
There's no ABI worry here.  Every compilation of every C++ file in
GDB sees the same struct packed definition.  It must, otherwise,
it would be a C++ ODR violation, meaning we'd have an invalid C++ program.

Please stop worrying about ABI compatibility here.  It's not relevant here.

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Eli Zaretskii]

> Cc: gdb-patches@sourceware.org
> From: Pedro Alves <pedro@palves.net>
> Date: Thu, 21 Jul 2022 19:30:23 +0100
> 
> > If this is supposed to be based on our vigilance and manual prevention
> > of exporting it, I think it's fragile and not very reliable.  If we
> > forget or miss something, we get a subtly broken build.
> 
> I'm sorry, but this isn't making a lot of sense.

You know, Pedro, it has become very hard to talk to you about almost
anything here.  You become annoyed almost from the second sentence I
write, and your annoyance shows.  It doesn't matter if the subject is
some patch to documentation or something in the code, it is impossible
for me to conduct any serious discussion without triggering your
annoyance, with the resulting very unpleasant exchange.

I'm outta here.

Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows

[Pedro Alves]

On 2022-07-21 7:45 p.m., Eli Zaretskii wrote:
>> Cc: gdb-patches@sourceware.org
>> From: Pedro Alves <pedro@palves.net>
>> Date: Thu, 21 Jul 2022 19:30:23 +0100
>>
>>> If this is supposed to be based on our vigilance and manual prevention
>>> of exporting it, I think it's fragile and not very reliable.  If we
>>> forget or miss something, we get a subtly broken build.
>>
>> I'm sorry, but this isn't making a lot of sense.
> 
> You know, Pedro, it has become very hard to talk to you about almost
> anything here.  You become annoyed almost from the second sentence I
> write, and your annoyance shows.  It doesn't matter if the subject is
> some patch to documentation or something in the code, it is impossible
> for me to conduct any serious discussion without triggering your
> annoyance, with the resulting very unpleasant exchange.
> 

I'm sorry that you feel that way.  Maybe I should have chosen my words
a bit better above.  I apologize if they came out too strong.
We do have our heated debates once in a while, but we always reach some conclusion,
and typically better than what either of us originally proposed, and I appreciate that.
From my side, I always do my best to explain my view.  I have tried to do so in
this case as well.

It is really the case that we don't need to worry about ABI here.  I can
try to explain better if there's a concrete problem you're seeing.  (Problems
with some hypothetical external GDB API we can deal with if we ever get to
that.)

> I'm outta here.
> 

I'm sorry to hear that.

Re: [PATCH 0/3] struct packed and Windows ports (PR build/29373)

[Tom Tromey]

>>>>> "Pedro" == Pedro Alves <pedro@palves.net> writes:

Pedro> PR build/29373 points out that mingw GDB builds are brokenly current,
Pedro> failing static assertions in gdbsupport/packed.h.  This series fixes
Pedro> it, for both mingw + gcc, and mingw + clang.

Please check these in.  It would be good to have the Windows build
working again.

TBH I'm not sure any of this packed<> work is worth the effort,
especially after seeing what's necessary for clang.  The memory savings
seem modest to me, considering that normally there aren't all that many
CUs.  OTOH I'd prefer a fix to go in.

thanks,
Tom

Re: [PATCH 0/3] struct packed and Windows ports (PR build/29373)

[Pedro Alves]

On 2022-07-25 3:58 p.m., Tom Tromey wrote:

> Please check these in.  It would be good to have the Windows build
> working again.

Done.