[binutils-gdb] Introduce struct packed template

[binutils-gdb] Introduce struct packed template

[Tom de Vries]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=68c0faca76a9b179af1a098946d6020091f43e06

commit 68c0faca76a9b179af1a098946d6020091f43e06
Author: Pedro Alves <pedro@palves.net>
Date:   Tue Jul 12 12:20:13 2022 +0200

    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<dwarf_unit_type, 1> 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<std::packed<language, 1>> 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

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 <http://www.gnu.org/licenses/>.  */
+
+#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<typename T, size_t Bytes = sizeof (T)>
+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<packed>::value);
+    gdb_static_assert (std::is_copy_constructible<packed>::value);
+    gdb_static_assert (std::is_move_constructible<packed>::value);
+    gdb_static_assert (std::is_copy_assignable<packed>::value);
+    gdb_static_assert (std::is_move_assignable<packed>::value);
+  }
+
+  operator T () const noexcept
+  {
+    return m_val;
+  }
+
+private:
+  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;
+}
+
+template<typename T, size_t Bytes>
+bool operator!= (T lhs, const std::atomic<packed<T, Bytes>> &rhs)
+{
+  return !(lhs == rhs);
+}
+
+template<typename T, size_t Bytes>
+bool operator!= (const std::atomic<packed<T, Bytes>> &lhs, T rhs)
+{
+  return !(lhs == rhs);
+}
+
+#endif