From 83cb98ad5e2fadd77e7534e32746bdce667079c7 Mon Sep 17 00:00:00 2001 From: Pedro Alves Date: Wed, 6 Jul 2022 14:46:49 +0100 Subject: [PATCH 1/3] Introduce struct packed template When building gdb with -fsanitize=thread and gcc 12, and running test-case gdb.dwarf2/dwz.exp, we run into a few data races. For example, between: ... Write of size 1 at 0x7b200000300e by thread T4: #0 process_psymtab_comp_unit gdb/dwarf2/read.c:6789 (gdb+0x830720) ... and: ... Previous read of size 1 at 0x7b200000300e by main thread: #0 cutu_reader::cutu_reader(dwarf2_per_cu_data*, dwarf2_per_objfile*, \ abbrev_table*, dwarf2_cu*, bool, abbrev_cache*) gdb/dwarf2/read.c:6164 \ (gdb+0x82edab) ... In other words, between: ... this_cu->unit_type = DW_UT_partial; ... and: ... if (this_cu->reading_dwo_directly) ... 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. Making the written fields separate memory locations, like this: ... struct { ENUM_BITFIELD (dwarf_unit_type) unit_type : 8; }; ... 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: struct { ENUM_BITFIELD (dwarf_unit_type) unit_type : 8 __attribute__((packed)); }; 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: packed unit_type; 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. 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: std::atomic> m_lang; 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: if (m_lang == language_c) 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: 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; 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. 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: /* Ensure size and aligment are what we expect. */ gdb_static_assert (sizeof (packed) == Bytes); gdb_static_assert (alignof (packed) == 1); Change-Id: Ifa94f0a2cebfae5e8f6ddc73265f05e7fd9e1532 --- gdbsupport/packed.h | 90 +++++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 90 insertions(+) create mode 100644 gdbsupport/packed.h 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 = val; + + /* Ensure size and aligment are what we expect. */ + gdb_static_assert (sizeof (packed) == Bytes); + gdb_static_assert (alignof (packed) == 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== (T lhs, const std::atomic> &rhs) +{ + return lhs == rhs.load (); +} + +template +bool operator== (const std::atomic> &lhs, T rhs) +{ + return lhs.load () == rhs; +} + +template +bool operator!= (T lhs, const std::atomic> &rhs) +{ + return !(lhs == rhs); +} + +template +bool operator!= (const std::atomic> &lhs, T rhs) +{ + return !(lhs == rhs); +} + +#endif -- 2.36.0