Re: [PATCH] Introduce struct packed template, fix -fsanitize=thread for per_cu fields

[Pedro Alves <pedro@palves.net>]

[-- Attachment #1: Type: text/plain, Size: 5956 bytes --]

On 2022-07-07 11:18 a.m., Tom de Vries wrote:
> On 7/6/22 21:20, Pedro Alves wrote:
>> On 2022-07-04 8:45 p.m., Tom de Vries via Gdb-patches wrote:
>>> On 7/4/22 20:32, Tom Tromey wrote:
>>>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>>>>
>>>> Tom>  /* The number of bits needed to represent all languages, with enough
>>>> Tom>     padding to allow for reasonable growth.  */
>>>> Tom> -#define LANGUAGE_BITS 5
>>>> Tom> +#define LANGUAGE_BITS 8
>>>>
>>>> This will negatively affect the size of symbols and so I think it should
>>>> be avoided.
>>>>
>>>
>>> Ack, Pedro suggested a way to avoid this:
>>> ...
>>> +  struct {
>>> +    /* The language of this CU.  */
>>> +    ENUM_BITFIELD (language) m_lang : LANGUAGE_BITS;
>>> +  };
>>> ...
>>>
>>
>> It actually doesn't avoid it in this case,
> 
> We were merely discussing the usage of LANGUAGE_BITS for general_symbol_info::m_language, and indeed using the "struct { ... };" approach avoids changing the LANGUAGE_BITS and introducing a penalty on symbol size (which is a more numerous entity than CUs).
> 

Yeah, sorry, I realized it after sending and decided I'd deserve the incoming cluebat.  :-)

> Still, of course it's also good to keep the dwarf2_per_cu_data struct as small as possible, so thanks for looking into this.

It's that, but also the desire to settle on some infrastructure or approach that we can reuse
going forward.


>> I have not actually tested this with -fsanitize=thread, though.  Would you
>> be up for testing that, Tom, if this approach looks reasonable?
>>
> 
> Yes, of course.
> 
> I've applied the patch and then started with my latest approach which avoid locks and uses atomics:

Thanks.

> ...
> diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
> index f98d8b27649..bc1af0ec2d3 100644
> --- a/gdb/dwarf2/read.h
> +++ b/gdb/dwarf2/read.h
> @@ -108,6 +108,7 @@ struct dwarf2_per_cu_data
>        m_header_read_in (false),
>        mark (false),
>        files_read (false),
> +      m_lang (language_unknown),
>        scanned (false)
>    {
>    }
> @@ -180,7 +181,7 @@ struct dwarf2_per_cu_data
>    packed<dwarf_unit_type, 1> m_unit_type = (dwarf_unit_type) 0;
> 
>    /* The language of this CU.  */
> -  packed<language, LANGUAGE_BYTES> m_lang = language_unknown;
> +  std::atomic<language> m_lang __attribute__((packed));
> 
>  public:
>    /* True if this CU has been scanned by the indexer; false if
> @@ -332,11 +333,13 @@ struct dwarf2_per_cu_data
> 
>    void set_lang (enum language lang)
>    {
> -    /* We'd like to be more strict here, similar to what is done in
> -       set_unit_type,  but currently a partial unit can go from unknown to
> -       minimal to ada to c.  */
> -    if (m_lang != lang)
> -      m_lang = lang;
> +    enum language nope = language_unknown;
> +    if (m_lang.compare_exchange_strong (nope, lang))
> +      return;
> +    nope = lang;
> +    if (m_lang.compare_exchange_strong (nope, lang))
> +      return;
> +    gdb_assert_not_reached ();
>    }
> 
>    /* Free any cached file names.  */
> ...
> 
> I've tried both:
> ...
>   packed<std::atomic<language>, LANGUAGE_BYTES> m_lang
>     = language_unknown;
> ...
> and:
> ...
>   std::atomic<packed<language, LANGUAGE_BYTES>> m_lang
>     = language_unknown;
> ...
> and both give compilation errors:
> ...
> src/gdb/dwarf2/read.h:184:58: error: could not convert ‘language_unknown’ from ‘language’ to ‘std::atomic<packed<language, 1> >’
>    std::atomic<packed<language, LANGUAGE_BYTES>> m_lang = language_unknown;
>                                                           ^~~~~~~~~~~~~~~~
> ...
> and:
> ...
> src/gdb/../gdbsupport/packed.h:84:47: error: bit-field ‘std::atomic<language> packed<std::atomic<language>, 1>::m_val’ with non-integral type
> ...
> 
> Maybe one of the two should work and the pack template needs further changes, I'm not sure.

Yes, I think std::atomic<packed<language, LANGUAGE_BYTES>> should work.  We need to write
the initialized using {}, like this:

  std::atomic<packed<language, LANGUAGE_BYTES>> m_lang {language_unknown};

and then we run into errors when comparing m_lang with enum language.  That is because
the preexisting operator==/operator!= would require converting from enum language to 
packed<language, LANGUAGE_BYTES>, and then from packed<language, LANGUAGE_BYTES> to
std::atomic<packed<language, LANGUAGE_BYTES>>.  That is two implicit conversions, but
C++ only does one automatically.  We can fix that by adding some operator==/operator!=
implementations.

I've done that in patch #1 attached.  I've also ditched the non-attribute-packed implementation.

> 
> Note btw that the attribute packed works here:
> ...
> +  std::atomic<language> m_lang __attribute__((packed));
> ...
> in the sense that it's got alignment 1:
> ...
>         struct atomic<language>    m_lang \
>           __attribute__((__aligned__(1))); /*    16     4 */
> ...
> but given that there's no LANGUAGE_BITS/BYTES, we're back to size 4 for the m_lang field, and size 128 overall.
> 
> So for now I've settled for:
> ...
> +  std::atomic<LANGUAGE_CONTAINER> m_lang;
> ...
> which does get me back to size 120.
> 
> WIP patch attached.

Please find attached 3 patches:

#1 - Introduce struct packed template
#2 - your original patch, but using struct packed, split to a separate patch. commit log updated.
#3 - a version of your std::atomic WIP patch that uses std::atomic<packed>

Patches #1 and #2 pass the testsuite cleanly for me.  Patch #3 compiles, but
runs into a couple regressions due to the gdb_assert_not_reached in set_lang
being reached.  I am not surprised since that set_lang code in your patch
looked WIP and I just blindly converted to the new approach to show the code
compiles.


[-- Attachment #2: 0003-std-atomic-packed.patch --]
[-- Type: text/x-patch, Size: 1596 bytes --]

From 26802a469ca457b56b7bc34b1cc1d1b1adc04409 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Thu, 7 Jul 2022 15:05:34 +0100
Subject: [PATCH 3/3] std::atomic + packed

Change-Id: Icde7883d8528fe3aa755a5d3f129fba08cc15dde
---
 gdb/dwarf2/read.h | 16 +++++++++-------
 1 file changed, 9 insertions(+), 7 deletions(-)

diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index f98d8b27649..7a33067d601 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -180,7 +180,7 @@ struct dwarf2_per_cu_data
   packed<dwarf_unit_type, 1> m_unit_type = (dwarf_unit_type) 0;
 
   /* The language of this CU.  */
-  packed<language, LANGUAGE_BYTES> m_lang = language_unknown;
+  std::atomic<packed<language, LANGUAGE_BYTES>> m_lang {language_unknown};
 
 public:
   /* True if this CU has been scanned by the indexer; false if
@@ -327,16 +327,18 @@ struct dwarf2_per_cu_data
   enum language lang () const
   {
     gdb_assert (m_lang != language_unknown);
-    return m_lang;
+    return m_lang.load ();
   }
 
   void set_lang (enum language lang)
   {
-    /* We'd like to be more strict here, similar to what is done in
-       set_unit_type,  but currently a partial unit can go from unknown to
-       minimal to ada to c.  */
-    if (m_lang != lang)
-      m_lang = lang;
+    packed<language, LANGUAGE_BYTES> nope = language_unknown;
+    if (m_lang.compare_exchange_strong (nope, lang))
+      return;
+    nope = lang;
+    if (m_lang.compare_exchange_strong (nope, lang))
+      return;
+    gdb_assert_not_reached ();
   }
 
   /* Free any cached file names.  */
-- 
2.36.0


[-- Attachment #3: 0002-gdb-symtab-Fix-fsanitize-thread-for-per_cu-fields.patch --]
[-- Type: text/x-patch, Size: 5150 bytes --]

From 5d4c83b208f15ad6e6df03709d5df401f048f1aa Mon Sep 17 00:00:00 2001
From: Tom de Vries <tdevries@suse.de>
Date: Wed, 6 Jul 2022 14:46:49 +0100
Subject: [PATCH 2/3] [gdb/symtab] Fix -fsanitize=thread for per_cu fields

When building gdb with -fsanitize=thread and gcc 12, and running test-case
gdb.dwarf2/dwz.exp, we run into a data race between:
...
  Read of size 1 at 0x7b200000300d by thread T2:^M
    #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+0x82ec95)^M
...
and:
...
  Previous write of size 1 at 0x7b200000300d by main thread:^M
    #0 prepare_one_comp_unit gdb/dwarf2/read.c:23588 (gdb+0x86f973)^M
...

In other words, between:
...
  if (this_cu->reading_dwo_directly)
...
and:
...
    cu->per_cu->lang = pretend_language;
...

Likewise, we run into a data race 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)
...

Likewise for the write to addresses_seen in cooked_indexer::check_bounds and a
read from is_dwz in dwarf2_find_containing_comp_unit for test-case
gdb.dwarf2/dw2-dir-file-name.exp and target board cc-with-dwz-m.

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, using the new
struct packed template fixes this.

The set of fields has been established experimentally to be the
minimal set to get rid of this type of -fsanitize=thread errors, but
more fields might require the same treatment.

Looking at the properties of the lang field, unlike dwarf_version it's
not available in the unit header, so it will be set the first time
during the parallel cooked index reading.  The same holds for
unit_type, and likewise for addresses_seen.

dwarf2_per_cu_data::addresses_seen is moved so that the bitfields that
currently follow it can be merged in the same memory location as the
bitfields that currently precede it, for better packing.

Tested on x86_64-linux.

Co-Authored-By: Pedro Alves <pedro@palves.net>

Change-Id: Ifa94f0a2cebfae5e8f6ddc73265f05e7fd9e1532
---
 gdb/defs.h        |  3 +++
 gdb/dwarf2/read.h | 20 +++++++++++---------
 2 files changed, 14 insertions(+), 9 deletions(-)

diff --git a/gdb/defs.h b/gdb/defs.h
index 99bfdd526ff..19f379d6588 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -232,6 +232,9 @@ enum language
 #define LANGUAGE_BITS 5
 gdb_static_assert (nr_languages <= (1 << LANGUAGE_BITS));
 
+/* The number of bytes needed to represent all languages.  */
+#define LANGUAGE_BYTES ((LANGUAGE_BITS + HOST_CHAR_BIT - 1) / HOST_CHAR_BIT)
+
 enum precision_type
   {
     single_precision,
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 1d9c66aafad..f98d8b27649 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -33,6 +33,7 @@
 #include "gdbsupport/gdb_obstack.h"
 #include "gdbsupport/hash_enum.h"
 #include "gdbsupport/function-view.h"
+#include "gdbsupport/packed.h"
 
 /* Hold 'maintenance (set|show) dwarf' commands.  */
 extern struct cmd_list_element *set_dwarf_cmdlist;
@@ -105,11 +106,8 @@ struct dwarf2_per_cu_data
       reading_dwo_directly (false),
       tu_read (false),
       m_header_read_in (false),
-      addresses_seen (false),
       mark (false),
       files_read (false),
-      m_unit_type {},
-      m_lang (language_unknown),
       scanned (false)
   {
   }
@@ -161,10 +159,6 @@ struct dwarf2_per_cu_data
      it private at the moment.  */
   mutable bool m_header_read_in : 1;
 
-  /* If addresses have been read for this CU (usually from
-     .debug_aranges), then this flag is set.  */
-  bool addresses_seen : 1;
-
   /* A temporary mark bit used when iterating over all CUs in
      expand_symtabs_matching.  */
   unsigned int mark : 1;
@@ -173,12 +167,20 @@ struct dwarf2_per_cu_data
      point in trying to read it again next time.  */
   bool files_read : 1;
 
+  /* Wrap the following in struct packed instead of bitfields to avoid
+     data races when the bitfields end up on the same memory location
+     (per C++ memory model).  */
+
+  /* If addresses have been read for this CU (usually from
+     .debug_aranges), then this flag is set.  */
+  packed<bool, 1> addresses_seen = false;
+
 private:
   /* The unit type of this CU.  */
-  ENUM_BITFIELD (dwarf_unit_type) m_unit_type : 8;
+  packed<dwarf_unit_type, 1> m_unit_type = (dwarf_unit_type) 0;
 
   /* The language of this CU.  */
-  ENUM_BITFIELD (language) m_lang : LANGUAGE_BITS;
+  packed<language, LANGUAGE_BYTES> m_lang = language_unknown;
 
 public:
   /* True if this CU has been scanned by the indexer; false if
-- 
2.36.0


[-- Attachment #4: 0001-Introduce-struct-packed-template.patch --]
[-- Type: text/x-patch, Size: 7098 bytes --]

From 83cb98ad5e2fadd77e7534e32746bdce667079c7 Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
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<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
---
 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 <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
-- 
2.36.0