From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2205) id B9EF73858C50; Tue, 12 Jul 2022 10:20:17 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B9EF73858C50 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Tom de Vries To: gdb-cvs@sourceware.org Subject: [binutils-gdb] Introduce struct packed template X-Act-Checkin: binutils-gdb X-Git-Author: Pedro Alves X-Git-Refname: refs/heads/master X-Git-Oldrev: a14413ddff3009418711b9f78a7d3dc7ac10dab0 X-Git-Newrev: 68c0faca76a9b179af1a098946d6020091f43e06 Message-Id: <20220712102017.B9EF73858C50@sourceware.org> Date: Tue, 12 Jul 2022 10:20:17 +0000 (GMT) X-BeenThere: gdb-cvs@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Tue, 12 Jul 2022 10:20:17 -0000 https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3D68c0faca76a9= b179af1a098946d6020091f43e06 commit 68c0faca76a9b179af1a098946d6020091f43e06 Author: Pedro Alves Date: Tue Jul 12 12:20:13 2022 +0200 Introduce struct packed template =20 When building gdb with -fsanitize=3Dthread and gcc 12, and running test= -case gdb.dwarf2/dwz.exp, we run into a few data races. For example, between: =20 ... Write of size 1 at 0x7b200000300e by thread T4: #0 process_psymtab_comp_unit gdb/dwarf2/read.c:6789 (gdb+0x830720) ... =20 and: =20 ... Previous read of size 1 at 0x7b200000300e by main thread: #0 cutu_reader::cutu_reader(dwarf2_per_cu_data*, dwarf2_per_objfil= e*, \ abbrev_table*, dwarf2_cu*, bool, abbrev_cache*) gdb/dwarf2/read.c:= 6164 \ (gdb+0x82edab) ... =20 In other words, between: =20 ... this_cu->unit_type =3D DW_UT_partial; ... =20 and: =20 ... if (this_cu->reading_dwo_directly) ... =20 The problem is that the written fields are part of the same memory location as the read fields, so executing a read and write in different threads is undefined behavour. =20 Making the written fields separate memory locations, like this: =20 ... struct { ENUM_BITFIELD (dwarf_unit_type) unit_type : 8; }; ... =20 fixes it, however that also increases the size of struct dwarf2_per_cu_data, because it introduces padding due to alignment of these new structs, which align on the natural alignment of the specified type of their fields. We can fix that with __attribute__((packed)), like so: =20 struct { ENUM_BITFIELD (dwarf_unit_type) unit_type : 8 __attribute__((packed= )); }; =20 but to avoid having to write that in several places and add suitable comments explaining how that concoction works, introduce a new struct packed template that wraps/hides this. Instead of the above, we'll be able to write: =20 packed unit_type; =20 Note that we can't change the type of dwarf_unit_type, as that is defined in include/, and shared with other projects, some of those written in C. =20 This patch just adds the struct packed type. Following patches will make use of it. One of those patches will want to wrap a struct packed in an std::atomic, like: =20 std::atomic> m_lang; =20 so the new gdbsupport/packed.h header adds some operators to make comparisions between that std::atomic and the type that the wrapped struct packed wraps work, like in: =20 if (m_lang =3D=3D language_c) =20 It would be possible to implement struct packed without using __attribute__((packed)), by having it store an array of bytes of the appropriate size instead, however that would make it less convenient to debug GDB. The way it's implemented, printing a struct packed variable just prints its field using its natural type, which is particularly useful if the type is an enum. I believe that __attribute__((packed)) is supported by all compilers that are able to build GDB. Even a few BFD headers use on ATTRIBUTE_PACKED on external types: =20 include/coff/external.h: } ATTRIBUTE_PACKED include/coff/external.h:} ATTRIBUTE_PACKED ; include/coff/external.h:} ATTRIBUTE_PACKED ; include/coff/pe.h:} ATTRIBUTE_PACKED ; include/coff/pe.h:} ATTRIBUTE_PACKED; include/elf/external.h:} ATTRIBUTE_PACKED Elf_External_Versym; =20 It is not possible to build GDB with MSVC today, but if it could, that would be one compiler that doesn't support this attribute. However, it supports packing via pragmas, so there's a way to cross that bridge if we ever get to it. I believe any compiler worth its salt supports some way of packing. =20 In any case, the worse that happens without the attribute is that some types become larger than ideal. Regardless, I've added a couple static assertions to catch such compilers in action: =20 /* Ensure size and aligment are what we expect. */ gdb_static_assert (sizeof (packed) =3D=3D Bytes); gdb_static_assert (alignof (packed) =3D=3D 1); =20 Change-Id: Ifa94f0a2cebfae5e8f6ddc73265f05e7fd9e1532 Diff: --- gdbsupport/packed.h | 90 +++++++++++++++++++++++++++++++++++++++++++++++++= ++++ 1 file changed, 90 insertions(+) diff --git a/gdbsupport/packed.h b/gdbsupport/packed.h new file mode 100644 index 00000000000..ebc66c0cb1a --- /dev/null +++ b/gdbsupport/packed.h @@ -0,0 +1,90 @@ +/* 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 . = */ + +#ifndef PACKED_H +#define PACKED_H + +/* 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. */ + +template +struct packed +{ +public: + packed (T val) + { + m_val =3D val; + + /* Ensure size and aligment are what we expect. */ + gdb_static_assert (sizeof (packed) =3D=3D Bytes); + gdb_static_assert (alignof (packed) =3D=3D 1); + + /* Make sure packed can be wrapped with std::atomic. */ + gdb_static_assert (std::is_trivially_copyable::value); + gdb_static_assert (std::is_copy_constructible::value); + gdb_static_assert (std::is_move_constructible::value); + gdb_static_assert (std::is_copy_assignable::value); + gdb_static_assert (std::is_move_assignable::value); + } + + operator T () const noexcept + { + return m_val; + } + +private: + T m_val : (Bytes * HOST_CHAR_BIT) ATTRIBUTE_PACKED; +}; + +/* Add some comparisons between std::atomic> and T. We need + this because the regular comparisons would require two implicit + conversions to go from T to std::atomic>: + + T -> packed + packed -> std::atomic> + + and C++ only does one. */ + +template +bool operator=3D=3D (T lhs, const std::atomic> &rhs) +{ + return lhs =3D=3D rhs.load (); +} + +template +bool operator=3D=3D (const std::atomic> &lhs, T rhs) +{ + return lhs.load () =3D=3D rhs; +} + +template +bool operator!=3D (T lhs, const std::atomic> &rhs) +{ + return !(lhs =3D=3D rhs); +} + +template +bool operator!=3D (const std::atomic> &lhs, T rhs) +{ + return !(lhs =3D=3D rhs); +} + +#endif