* [PATCH 0/3] struct packed and Windows ports (PR build/29373) @ 2022-07-21 15:21 Pedro Alves 2022-07-21 15:21 ` [PATCH 1/3] struct packed: Use gcc_struct on Windows Pedro Alves ` (3 more replies) 0 siblings, 4 replies; 17+ messages in thread From: Pedro Alves @ 2022-07-21 15:21 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 15:21 [PATCH 0/3] struct packed and Windows ports (PR build/29373) Pedro Alves @ 2022-07-21 15:21 ` Pedro Alves 2022-07-21 16:03 ` Eli Zaretskii 2022-07-21 15:21 ` [PATCH 2/3] struct packed: Unit tests and more operators Pedro Alves ` (2 subsequent siblings) 3 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2022-07-21 15:21 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 15:21 ` [PATCH 1/3] struct packed: Use gcc_struct on Windows Pedro Alves @ 2022-07-21 16:03 ` Eli Zaretskii 2022-07-21 17:05 ` Pedro Alves 0 siblings, 1 reply; 17+ messages in thread From: Eli Zaretskii @ 2022-07-21 16:03 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 16:03 ` Eli Zaretskii @ 2022-07-21 17:05 ` Pedro Alves 2022-07-21 17:40 ` Eli Zaretskii 0 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2022-07-21 17:05 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 17:05 ` Pedro Alves @ 2022-07-21 17:40 ` Eli Zaretskii 2022-07-21 18:15 ` Pedro Alves 2022-07-21 18:18 ` Pedro Alves 0 siblings, 2 replies; 17+ messages in thread From: Eli Zaretskii @ 2022-07-21 17:40 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 17:40 ` Eli Zaretskii @ 2022-07-21 18:15 ` Pedro Alves 2022-07-21 18:23 ` Eli Zaretskii 2022-07-21 18:18 ` Pedro Alves 1 sibling, 1 reply; 17+ messages in thread From: Pedro Alves @ 2022-07-21 18:15 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 18:15 ` Pedro Alves @ 2022-07-21 18:23 ` Eli Zaretskii 2022-07-21 18:30 ` Pedro Alves 0 siblings, 1 reply; 17+ messages in thread From: Eli Zaretskii @ 2022-07-21 18:23 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 18:23 ` Eli Zaretskii @ 2022-07-21 18:30 ` Pedro Alves 2022-07-21 18:45 ` Eli Zaretskii 0 siblings, 1 reply; 17+ messages in thread From: Pedro Alves @ 2022-07-21 18:30 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches 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? ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 18:30 ` Pedro Alves @ 2022-07-21 18:45 ` Eli Zaretskii 2022-07-21 18:57 ` Pedro Alves 0 siblings, 1 reply; 17+ messages in thread From: Eli Zaretskii @ 2022-07-21 18:45 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 18:45 ` Eli Zaretskii @ 2022-07-21 18:57 ` Pedro Alves 0 siblings, 0 replies; 17+ messages in thread From: Pedro Alves @ 2022-07-21 18:57 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 17:40 ` Eli Zaretskii 2022-07-21 18:15 ` Pedro Alves @ 2022-07-21 18:18 ` Pedro Alves 2022-07-21 18:28 ` Eli Zaretskii 1 sibling, 1 reply; 17+ messages in thread From: Pedro Alves @ 2022-07-21 18:18 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 18:18 ` Pedro Alves @ 2022-07-21 18:28 ` Eli Zaretskii 2022-07-21 18:38 ` Pedro Alves 0 siblings, 1 reply; 17+ messages in thread From: Eli Zaretskii @ 2022-07-21 18:28 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches > 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 1/3] struct packed: Use gcc_struct on Windows 2022-07-21 18:28 ` Eli Zaretskii @ 2022-07-21 18:38 ` Pedro Alves 0 siblings, 0 replies; 17+ messages in thread From: Pedro Alves @ 2022-07-21 18:38 UTC (permalink / raw) To: Eli Zaretskii; +Cc: gdb-patches 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 2/3] struct packed: Unit tests and more operators 2022-07-21 15:21 [PATCH 0/3] struct packed and Windows ports (PR build/29373) Pedro Alves 2022-07-21 15:21 ` [PATCH 1/3] struct packed: Use gcc_struct on Windows Pedro Alves @ 2022-07-21 15:21 ` Pedro Alves 2022-07-21 15:21 ` [PATCH 3/3] struct packed: Add fallback byte array implementation Pedro Alves 2022-07-25 14:58 ` [PATCH 0/3] struct packed and Windows ports (PR build/29373) Tom Tromey 3 siblings, 0 replies; 17+ messages in thread From: Pedro Alves @ 2022-07-21 15:21 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* [PATCH 3/3] struct packed: Add fallback byte array implementation 2022-07-21 15:21 [PATCH 0/3] struct packed and Windows ports (PR build/29373) Pedro Alves 2022-07-21 15:21 ` [PATCH 1/3] struct packed: Use gcc_struct on Windows Pedro Alves 2022-07-21 15:21 ` [PATCH 2/3] struct packed: Unit tests and more operators Pedro Alves @ 2022-07-21 15:21 ` Pedro Alves 2022-07-25 14:58 ` [PATCH 0/3] struct packed and Windows ports (PR build/29373) Tom Tromey 3 siblings, 0 replies; 17+ messages in thread From: Pedro Alves @ 2022-07-21 15:21 UTC (permalink / raw) To: gdb-patches 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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] struct packed and Windows ports (PR build/29373) 2022-07-21 15:21 [PATCH 0/3] struct packed and Windows ports (PR build/29373) Pedro Alves ` (2 preceding siblings ...) 2022-07-21 15:21 ` [PATCH 3/3] struct packed: Add fallback byte array implementation Pedro Alves @ 2022-07-25 14:58 ` Tom Tromey 2022-07-25 15:18 ` Pedro Alves 3 siblings, 1 reply; 17+ messages in thread From: Tom Tromey @ 2022-07-25 14:58 UTC (permalink / raw) To: Pedro Alves; +Cc: gdb-patches >>>>> "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 ^ permalink raw reply [flat|nested] 17+ messages in thread
* Re: [PATCH 0/3] struct packed and Windows ports (PR build/29373) 2022-07-25 14:58 ` [PATCH 0/3] struct packed and Windows ports (PR build/29373) Tom Tromey @ 2022-07-25 15:18 ` Pedro Alves 0 siblings, 0 replies; 17+ messages in thread From: Pedro Alves @ 2022-07-25 15:18 UTC (permalink / raw) To: Tom Tromey; +Cc: gdb-patches 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. ^ permalink raw reply [flat|nested] 17+ messages in thread
end of thread, other threads:[~2022-07-25 15:18 UTC | newest] Thread overview: 17+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2022-07-21 15:21 [PATCH 0/3] struct packed and Windows ports (PR build/29373) Pedro Alves 2022-07-21 15:21 ` [PATCH 1/3] struct packed: Use gcc_struct on Windows Pedro Alves 2022-07-21 16:03 ` Eli Zaretskii 2022-07-21 17:05 ` Pedro Alves 2022-07-21 17:40 ` Eli Zaretskii 2022-07-21 18:15 ` Pedro Alves 2022-07-21 18:23 ` Eli Zaretskii 2022-07-21 18:30 ` Pedro Alves 2022-07-21 18:45 ` Eli Zaretskii 2022-07-21 18:57 ` Pedro Alves 2022-07-21 18:18 ` Pedro Alves 2022-07-21 18:28 ` Eli Zaretskii 2022-07-21 18:38 ` Pedro Alves 2022-07-21 15:21 ` [PATCH 2/3] struct packed: Unit tests and more operators Pedro Alves 2022-07-21 15:21 ` [PATCH 3/3] struct packed: Add fallback byte array implementation Pedro Alves 2022-07-25 14:58 ` [PATCH 0/3] struct packed and Windows ports (PR build/29373) Tom Tromey 2022-07-25 15:18 ` Pedro Alves
This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).