[PATCH 1/5] [COVER-LETTER, RFC] Fix some fsanitize=thread issues in gdb's cooked index

[PATCH 1/5] [COVER-LETTER, RFC] Fix some fsanitize=thread issues in gdb's cooked index

[Tom de Vries]

This is an attempt at fixing some gdb thread sanitizer issues.

I used gcc 12 and -fsanitize=thread.

The patches address problems in the parallel reading of the cooked index.

There are a few problems remaining when testing:
- FAILs in test-case gdb.base/early-init-file.exp, submitted test-case patch
  at https://sourceware.org/pipermail/gdb-patches/2022-June/190343.html
- we have a heap-use-after-free in gdb.dwarf2/dw2-icc-opaque.exp.
  Not thread-related, filed at
  https://sourceware.org/bugzilla/show_bug.cgi?id=29295.
- we run into timeouts in several test-cases:
  - gdb.gdb/python-helper.exp
  - gdb.multi/multi-target-thread-find.exp
  - gdb.threads/break-while-running.exp
  - gdb.threads/detach-step-over.exp
  - gdb.threads/fork-thread-pending.exp
  - gdb.threads/next-fork-other-thread.exp
  - gdb.threads/pending-fork-event-detach.exp
  - gdb.threads/wp-replication.exp
  presumably due to sanitizer overhead.
- with tui tests, at the point of calling newterm we run into
  "WARNING: ThreadSanitizer: unlock of an unlocked mutex (or by a wrong
  thread)".  This is reproducible in a standalone test-case calling newterm,
  so not related to gdb.  Affects:
  - gdb.python/tui-window.exp
  - gdb.tui/basic.exp
  - gdb.tui/empty.exp
  - gdb.tui/list.exp
  - gdb.tui/new-layout.exp
  - gdb.tui/regs.exp
  - gdb.tui/tui-missing-src.exp
  - gdb.tui/winheight.exp
  - gdb.tui/winwidth.exp
- We run into 4 reported data races in gdb.base/gdb-sigterm.exp, filed at
  https://sourceware.org/bugzilla/show_bug.cgi?id=29297.
---
 gdb/COVER-LETTER | 0
 1 file changed, 0 insertions(+), 0 deletions(-)
 create mode 100644 gdb/COVER-LETTER

diff --git a/gdb/COVER-LETTER b/gdb/COVER-LETTER
new file mode 100644
index 00000000000..e69de29bb2d

base-commit: 9117c7b452ef76304f4394a97c887d0c4af439f5
-- 
2.35.3

[PATCH 2/5] [gdb/symtab] Fix data race on per_cu->dwarf_version

[Tom de Vries]

When building gdb with -fsanitize=thread and gcc 12, and running test-case
gdb.dwarf2/dwz.exp, we run into a data race between thread T2 and the main
thread in the same write:
...
Write of size 1 at 0x7b200000300c:^M
    #0 cutu_reader::cutu_reader(dwarf2_per_cu_data*, dwarf2_per_objfile*, \
    abbrev_table*, dwarf2_cu*, bool, abbrev_cache*) gdb/dwarf2/read.c:6252 \
    (gdb+0x82f3b3)^M
...
which is here:
...
         this_cu->dwarf_version = cu->header.version;
...

Both writes are called from the parallel for in dwarf2_build_psymtabs_hard,
this one directly:
...
    #1 process_psymtab_comp_unit gdb/dwarf2/read.c:6774 (gdb+0x8304d7)^M
    #2 operator() gdb/dwarf2/read.c:7098 (gdb+0x8317be)^M
    #3 operator() gdbsupport/parallel-for.h:163 (gdb+0x872380)^M
...
and this via the PU import:
...
    #1 cooked_indexer::ensure_cu_exists(cutu_reader*, dwarf2_per_objfile*, \
    sect_offset, bool,  bool) gdb/dwarf2/read.c:17964 (gdb+0x85c43b)^M
    #2 cooked_indexer::index_imported_unit(cutu_reader*, unsigned char const*, \
    abbrev_info const*) gdb/dwarf2/read.c:18248 (gdb+0x85d8ff)^M
    #3 cooked_indexer::index_dies(cutu_reader*, unsigned char const*, \
    cooked_index_entry const*, bool) gdb/dwarf2/read.c:18302 (gdb+0x85dcdb)^M
    #4 cooked_indexer::make_index(cutu_reader*) gdb/dwarf2/read.c:18443 \
    (gdb+0x85e68a)^M
    #5 process_psymtab_comp_unit gdb/dwarf2/read.c:6812 (gdb+0x830879)^M
    #6 operator() gdb/dwarf2/read.c:7098 (gdb+0x8317be)^M
    #7 operator() gdbsupport/parallel-for.h:171 (gdb+0x8723e2)^M
...

Fix this by setting the field earlier, in read_comp_units_from_section.

The write in cutu_reader::cutu_reader() is still needed, in case
read_comp_units_from_section is not used.

Make the write conditional, such that it doesn't trigger if the field is
already set by read_comp_units_from_section.

Tested on x86_64-linux.
---
 gdb/dwarf2/read.c | 9 ++++++++-
 1 file changed, 8 insertions(+), 1 deletion(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d5088395fb1..67702d43aff 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6249,7 +6249,11 @@ cutu_reader::cutu_reader (dwarf2_per_cu_data *this_cu,
 	    this_cu->length = cu->header.get_length ();
 	  else
 	    gdb_assert (this_cu->length == cu->header.get_length ());
-	  this_cu->dwarf_version = cu->header.version;
+	  /* Init this_cu->dwarf_version lazily.  */
+	  if (this_cu->dwarf_version == 0)
+	    this_cu->dwarf_version = cu->header.version;
+	  else
+	    gdb_assert (this_cu->dwarf_version == cu->header.version);
 	}
     }
 
@@ -7206,6 +7210,9 @@ read_comp_units_from_section (dwarf2_per_objfile *per_objfile,
       this_cu->length = cu_header.length + cu_header.initial_length_size;
       this_cu->is_dwz = is_dwz;
       this_cu->section = section;
+      /* Init this asap, to make sure the lazy init (which may be run in
+	 parallel for the cooked index case) isn't necessary.  */
+      this_cu->dwarf_version = cu_header.version;
 
       info_ptr = info_ptr + this_cu->length;
       per_objfile->per_bfd->all_comp_units.push_back (std::move (this_cu));
-- 
2.35.3

[PATCH 3/5] [gdb/symtab] Work around fsanitize=address false positive for per_cu->lang

[Tom de Vries]

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;
...

Both fields are part of the same bitfield, and writing to one field while
reading from another is not a problem, so this is a false positive.

An easy way to get rid of the false positive when compiling with thread
sanitizer is to do this:
...
  #ifdef __SANITIZE_THREAD__
    language lang;
  #else
    ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
  #endif
...
but that also inhibits the detection of parallel writing to different fields
in the same bitfield, which is a problem.

Fix this instead by moving the lang field out of the bitfield.

In the bitfield, storing the lang field required LANGUAGE_BITS == 5 bits.

Set the underlying type of enum lang to char, to require only 8 bits outside
the bitfield.

Due to a compilation error with gcc 7.5.0, that also requires us to set
LANGUAGE_BITS to 8.

The size of struct dwarf2_per_cu_data remains the same (at least for -m64).

Tested on x86_64-linux.
---
 gdb/defs.h        |  4 ++--
 gdb/dwarf2/read.h | 10 +++++-----
 2 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/gdb/defs.h b/gdb/defs.h
index 99bfdd526ff..d5bf7cb2778 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -207,7 +207,7 @@ extern void quit_serial_event_clear (void);
    ada_sniff_from_mangled_name).  (Keep this order in sync with the
    'languages' array in language.c.)  */
 
-enum language
+enum language : char
   {
     language_unknown,		/* Language not known */
     language_auto,		/* Placeholder for automatic setting */
@@ -229,7 +229,7 @@ enum language
 
 /* The number of bits needed to represent all languages, with enough
    padding to allow for reasonable growth.  */
-#define LANGUAGE_BITS 5
+#define LANGUAGE_BITS 8
 gdb_static_assert (nr_languages <= (1 << LANGUAGE_BITS));
 
 enum precision_type
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 51e02dfc457..db300b19621 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -99,7 +99,8 @@ typedef std::unique_ptr<dwarf2_per_cu_data, dwarf2_per_cu_data_deleter>
 struct dwarf2_per_cu_data
 {
   dwarf2_per_cu_data ()
-    : queued (false),
+    : lang (language_unknown),
+      queued (false),
       is_debug_types (false),
       is_dwz (false),
       reading_dwo_directly (false),
@@ -109,7 +110,6 @@ struct dwarf2_per_cu_data
       mark (false),
       files_read (false),
       unit_type {},
-      lang (language_unknown),
       scanned (false)
   {
   }
@@ -125,6 +125,9 @@ struct dwarf2_per_cu_data
   /* DWARF standard version this data has been read from (such as 4 or 5).  */
   unsigned char dwarf_version = 0;
 
+  /* The language of this CU.  */
+  language lang;
+
   /* Flag indicating this compilation unit will be read in before
      any of the current compilation units are processed.  */
   unsigned int queued : 1;
@@ -174,9 +177,6 @@ struct dwarf2_per_cu_data
   /* The unit type of this CU.  */
   ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
 
-  /* The language of this CU.  */
-  ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
-
   /* True if this CU has been scanned by the indexer; false if
      not.  */
   std::atomic<bool> scanned;
-- 
2.35.3

[PATCH 4/5] [gdb/symtab] Work around fsanitize=address false positive for per_cu->unit_type

[Tom de Vries]

When building gdb with -fsanitize=thread and gcc 12, and running test-case
gdb.dwarf2/dwz.exp, 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)
...

Both fields are part of the same bitfield, and writing to one field while
reading from another is not a problem, so this is a false positive.

Fix this by moving the unit_type field out of the bitfield.

Use type unsigned char instead of enum dwarf_unit_type to keep requiring only
8 bits.

The size of struct dwarf2_per_cu_data remains the same (at least for -m64).

Tested on x86_64-linux.
---
 gdb/dwarf2/read.h | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index db300b19621..abce4f83f39 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -100,6 +100,7 @@ struct dwarf2_per_cu_data
 {
   dwarf2_per_cu_data ()
     : lang (language_unknown),
+      unit_type {},
       queued (false),
       is_debug_types (false),
       is_dwz (false),
@@ -109,7 +110,6 @@ struct dwarf2_per_cu_data
       addresses_seen (false),
       mark (false),
       files_read (false),
-      unit_type {},
       scanned (false)
   {
   }
@@ -128,6 +128,10 @@ struct dwarf2_per_cu_data
   /* The language of this CU.  */
   language lang;
 
+  /* The unit type of this CU.  We'd like to use dwarf_unit_type but that
+     requires 'int' storage. */
+  unsigned char unit_type;
+
   /* Flag indicating this compilation unit will be read in before
      any of the current compilation units are processed.  */
   unsigned int queued : 1;
@@ -174,9 +178,6 @@ struct dwarf2_per_cu_data
      point in trying to read it again next time.  */
   bool files_read : 1;
 
-  /* The unit type of this CU.  */
-  ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
-
   /* True if this CU has been scanned by the indexer; false if
      not.  */
   std::atomic<bool> scanned;
-- 
2.35.3

[PATCH 5/5] [gdb/symtab] Fix data race on per_cu->lang

[Tom de Vries]

When building gdb with -fsanitize=thread and gcc 12, and running test-case
gdb.dwarf2/inlined_subroutine-inheritance.exp, we run into a data race
between thread T4 and the main thread:
...
  Write of size 1 at 0x7b200000308d:
    #0 prepare_one_comp_unit gdb/dwarf2/read.c:23586 (gdb+0x86f859)
...
which is here:
...
    cu->per_cu->lang = dwarf_lang_to_enum_language (attr->constant_value (0));
...

Both writes are called from the parallel for in dwarf2_build_psymtabs_hard,
this one directly:
...
    #1 process_psymtab_comp_unit gdb/dwarf2/read.c:6812 (gdb+0x830912)
    #2 operator() gdb/dwarf2/read.c:7102 (gdb+0x831902)
    #3 operator() gdb/../gdbsupport/parallel-for.h:171 (gdb+0x8723a8)
...
and this one when handling cross-CU refs:
...
    #1 cooked_indexer::ensure_cu_exists(cutu_reader*, dwarf2_per_objfile*, \
    sect_offset, bool, bool) gdb/dwarf2/read.c:17973 (gdb+0x85c522)
    #2 cooked_indexer::scan_attributes(dwarf2_per_cu_data*, cutu_reader*, \
    unsigned char const*, unsigned char const*, abbrev_info const*, \
    char const**, char const**, enum_flags<cooked_index_flag_enum>*, \
    sect_offset*, cooked_index_entry const**, unsigned long*, bool) \
    gdb/dwarf2/read.c:18148 (gdb+0x85d079)
    #3 cooked_indexer::index_dies(cutu_reader*, unsigned char const*, \
    cooked_index_entry const*, bool) gdb/dwarf2/read.c:18327 (gdb+0x85df65)
    #4 cooked_indexer::make_index(cutu_reader*) gdb/dwarf2/read.c:18450 \
    (gdb+0x85e72c)
    #5 process_psymtab_comp_unit gdb/dwarf2/read.c:6816 (gdb+0x8309c1)
    #6 operator() gdb/dwarf2/read.c:7102 (gdb+0x831902)
    #7 operator() gdbsupport/parallel-for.h:163 (gdb+0x872346)
...

Fix this by guarding the write with a lock.
---
 gdb/dwarf2/read.c | 31 ++++++++++++++++++++++++++-----
 1 file changed, 26 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 67702d43aff..a36f25f4e62 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6766,6 +6766,10 @@ class cooked_indexer
   std::vector<deferred_entry> m_deferred_entries;
 };
 
+#if CXX_STD_THREAD
+static std::mutex per_cu_long_lock;
+#endif
+
 /* Subroutine of dwarf2_build_psymtabs_hard to simplify it.
    Process compilation unit THIS_CU for a psymtab.  */
 
@@ -23567,6 +23571,8 @@ prepare_one_comp_unit (struct dwarf2_cu *cu, struct die_info *comp_unit_die,
 
   /* Set the language we're debugging.  */
   attr = dwarf2_attr (comp_unit_die, DW_AT_language, cu);
+
+  enum language lang;
   if (cu->producer != nullptr
       && strstr (cu->producer, "IBM XL C for OpenCL") != NULL)
     {
@@ -23574,19 +23580,34 @@ prepare_one_comp_unit (struct dwarf2_cu *cu, struct die_info *comp_unit_die,
 	 attribute is not standardised yet.  As a workaround for the
 	 language detection we fall back to the DW_AT_producer
 	 string.  */
-      cu->per_cu->lang = language_opencl;
+      lang = language_opencl;
     }
   else if (cu->producer != nullptr
 	   && strstr (cu->producer, "GNU Go ") != NULL)
     {
       /* Similar hack for Go.  */
-      cu->per_cu->lang = language_go;
+      lang = language_go;
     }
   else if (attr != nullptr)
-    cu->per_cu->lang = dwarf_lang_to_enum_language (attr->constant_value (0));
+    lang = dwarf_lang_to_enum_language (attr->constant_value (0));
   else
-    cu->per_cu->lang = pretend_language;
-  cu->language_defn = language_def (cu->per_cu->lang);
+    lang = pretend_language;
+
+  {
+#if CXX_STD_THREAD
+    std::lock_guard<std::mutex> guard (per_cu_long_lock);
+#endif
+    /* Assign cu->per_cu->lang lazily.  Note: we're not doing here:
+         if (cu->per_cu->lang == language_unknown)
+           cu->per_cu->lang = lang;
+         else
+           gdb_assert (cu->per_cu->lang == lang);
+       because language may go from unknown to minimal to c.  */
+    if (cu->per_cu->lang != lang)
+      cu->per_cu->lang = lang;
+  }
+
+  cu->language_defn = language_def (lang);
 }
 
 /* See read.h.  */
-- 
2.35.3

Re: [PATCH 3/5] [gdb/symtab] Work around fsanitize=address false positive for per_cu->lang

[Pedro Alves]

On 2022-06-29 16:29, Tom de Vries via Gdb-patches wrote:
> 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;
> ...
> 
> Both fields are part of the same bitfield, and writing to one field while
> reading from another is not a problem, so this is a false positive.

I don't understand this sentence.  Particularly "same bitfield", or
really "Both fields are part of the same bitfield,".  How can two bitfields
be part of the same bitfield?

Anyhow, both bitfields are part of a sequence of contiguous bitfields, here
stripped of comments:

  unsigned int queued : 1;
  unsigned int is_debug_types : 1;
  unsigned int is_dwz : 1;
  unsigned int reading_dwo_directly : 1;
  unsigned int tu_read : 1;
  mutable bool m_header_read_in : 1;
  bool addresses_seen : 1;
  unsigned int mark : 1;
  bool files_read : 1;
  ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
  ENUM_BITFIELD (language) lang : LANGUAGE_BITS;

Per C++11, they're all part of the same _memory location_.  From N3253 (C++11), intro.memory:

 "A memory location is either an object of scalar type or a maximal sequence of adjacent bit-fields all having
 non-zero width. (...)  Two threads of execution (1.10) can update and access separate memory locations
  without interfering with each other.
 (...)
 [ Note: Thus a bit-field and an adjacent non-bit-field are in separate memory locations, and therefore can be
 concurrently updated by two threads of execution without interference. The same applies to two bit-fields,
 if one is declared inside a nested struct declaration and the other is not, or if the two are separated by
 a zero-length bit-field declaration, or if they are separated by a non-bit-field declaration. It is not safe to
 concurrently update two bit-fields in the same struct if all fields between them are also bit-fields of non-zero
 width. — end note ]"

And while it is true that in practice writing to one bit-field from one thread and reading from another,
if they reside on the same location, is OK in practice, it is still undefined behavior.

Note the escape hatch mentioned above:

   "if the two are separated by a zero-length bit-field declaration"

Thus, a change like this:

  unsigned int queued : 1;
  unsigned int is_debug_types : 1;
  unsigned int is_dwz : 1;
  unsigned int reading_dwo_directly : 1;
  unsigned int tu_read : 1;
  mutable bool m_header_read_in : 1;
  bool addresses_seen : 1;
  unsigned int mark : 1;
  bool files_read : 1;
  ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
+
+  /* Ensure lang is a separate memory location, so we can update
+     it concurrently with other bitfields.  */
+  char :0;
+
  ENUM_BITFIELD (language) lang : LANGUAGE_BITS;


... should work.

Re: [PATCH 3/5] [gdb/symtab] Work around fsanitize=address false positive for per_cu->lang

[Pedro Alves]

On 2022-06-29 18:38, Pedro Alves wrote:
> On 2022-06-29 16:29, Tom de Vries via Gdb-patches wrote:
>> 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;
>> ...
>>
>> Both fields are part of the same bitfield, and writing to one field while
>> reading from another is not a problem, so this is a false positive.
> 
> I don't understand this sentence.  Particularly "same bitfield", or
> really "Both fields are part of the same bitfield,".  How can two bitfields
> be part of the same bitfield?
> 
> Anyhow, both bitfields are part of a sequence of contiguous bitfields, here
> stripped of comments:
> 
>   unsigned int queued : 1;
>   unsigned int is_debug_types : 1;
>   unsigned int is_dwz : 1;
>   unsigned int reading_dwo_directly : 1;
>   unsigned int tu_read : 1;
>   mutable bool m_header_read_in : 1;
>   bool addresses_seen : 1;
>   unsigned int mark : 1;
>   bool files_read : 1;
>   ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
>   ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
> 
> Per C++11, they're all part of the same _memory location_.  From N3253 (C++11), intro.memory:
> 
>  "A memory location is either an object of scalar type or a maximal sequence of adjacent bit-fields all having
>  non-zero width. (...)  Two threads of execution (1.10) can update and access separate memory locations
>   without interfering with each other.
>  (...)
>  [ Note: Thus a bit-field and an adjacent non-bit-field are in separate memory locations, and therefore can be
>  concurrently updated by two threads of execution without interference. The same applies to two bit-fields,
>  if one is declared inside a nested struct declaration and the other is not, or if the two are separated by
>  a zero-length bit-field declaration, or if they are separated by a non-bit-field declaration. It is not safe to
>  concurrently update two bit-fields in the same struct if all fields between them are also bit-fields of non-zero
>  width. — end note ]"
> 
> And while it is true that in practice writing to one bit-field from one thread and reading from another,
> if they reside on the same location, is OK in practice, it is still undefined behavior.
> 
> Note the escape hatch mentioned above:
> 
>    "if the two are separated by a zero-length bit-field declaration"
> 
> Thus, a change like this:
> 
>   unsigned int queued : 1;
>   unsigned int is_debug_types : 1;
>   unsigned int is_dwz : 1;
>   unsigned int reading_dwo_directly : 1;
>   unsigned int tu_read : 1;
>   mutable bool m_header_read_in : 1;
>   bool addresses_seen : 1;
>   unsigned int mark : 1;
>   bool files_read : 1;
>   ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
> +
> +  /* Ensure lang is a separate memory location, so we can update
> +     it concurrently with other bitfields.  */
> +  char :0;
> +
>   ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
> 
> 
> ... should work.

The "if one is declared inside a nested struct declaration and the other
is not" escape hatch may be interesting too, as in, we'd write:

   struct {
     ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
   };

... and since the struct is anonymous, nothing else needs to change.

In patch #4, you'd just do this too:

  struct {
     ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
  };

The "wrapping" syntax seems to read a bit better, particularly since this
way you don't have to worry about putting a :0 bitfield before and
another after.

Re: [PATCH 3/5] [gdb/symtab] Work around fsanitize=address false positive for per_cu->lang

[Pedro Alves]

On 2022-06-29 19:25, Pedro Alves wrote:
> On 2022-06-29 18:38, Pedro Alves wrote:
>> On 2022-06-29 16:29, Tom de Vries via Gdb-patches wrote:
>>> 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;
>>> ...
>>>
>>> Both fields are part of the same bitfield, and writing to one field while
>>> reading from another is not a problem, so this is a false positive.
>>
>> I don't understand this sentence.  Particularly "same bitfield", or
>> really "Both fields are part of the same bitfield,".  How can two bitfields
>> be part of the same bitfield?
>>
>> Anyhow, both bitfields are part of a sequence of contiguous bitfields, here
>> stripped of comments:
>>
>>   unsigned int queued : 1;
>>   unsigned int is_debug_types : 1;
>>   unsigned int is_dwz : 1;
>>   unsigned int reading_dwo_directly : 1;
>>   unsigned int tu_read : 1;
>>   mutable bool m_header_read_in : 1;
>>   bool addresses_seen : 1;
>>   unsigned int mark : 1;
>>   bool files_read : 1;
>>   ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
>>   ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
>>
>> Per C++11, they're all part of the same _memory location_.  From N3253 (C++11), intro.memory:
>>
>>  "A memory location is either an object of scalar type or a maximal sequence of adjacent bit-fields all having
>>  non-zero width. (...)  Two threads of execution (1.10) can update and access separate memory locations
>>   without interfering with each other.
>>  (...)
>>  [ Note: Thus a bit-field and an adjacent non-bit-field are in separate memory locations, and therefore can be
>>  concurrently updated by two threads of execution without interference. The same applies to two bit-fields,
>>  if one is declared inside a nested struct declaration and the other is not, or if the two are separated by
>>  a zero-length bit-field declaration, or if they are separated by a non-bit-field declaration. It is not safe to
>>  concurrently update two bit-fields in the same struct if all fields between them are also bit-fields of non-zero
>>  width. — end note ]"
>>
>> And while it is true that in practice writing to one bit-field from one thread and reading from another,
>> if they reside on the same location, is OK in practice, it is still undefined behavior.
>>
>> Note the escape hatch mentioned above:
>>
>>    "if the two are separated by a zero-length bit-field declaration"
>>
>> Thus, a change like this:
>>
>>   unsigned int queued : 1;
>>   unsigned int is_debug_types : 1;
>>   unsigned int is_dwz : 1;
>>   unsigned int reading_dwo_directly : 1;
>>   unsigned int tu_read : 1;
>>   mutable bool m_header_read_in : 1;
>>   bool addresses_seen : 1;
>>   unsigned int mark : 1;
>>   bool files_read : 1;
>>   ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
>> +
>> +  /* Ensure lang is a separate memory location, so we can update
>> +     it concurrently with other bitfields.  */
>> +  char :0;
>> +
>>   ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
>>
>>
>> ... should work.
> 
> The "if one is declared inside a nested struct declaration and the other
> is not" escape hatch may be interesting too, as in, we'd write:
> 
>    struct {
>      ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
>    };
> 
> ... and since the struct is anonymous, nothing else needs to change.
> 
> In patch #4, you'd just do this too:
> 
>   struct {
>      ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
>   };
> 
> The "wrapping" syntax seems to read a bit better, particularly since this
> way you don't have to worry about putting a :0 bitfield before and
> another after.

I keep coming back, sorry...  :-P

Another thought is that in both patches #3 and #4, it's reading_dwo_directly
that is racing with two other bitfields.  So I wonder whether it's _that_ one
that should be moved to a separate memory location.

Re: [PATCH 2/5] [gdb/symtab] Fix data race on per_cu->dwarf_version

[Tom de Vries]

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

On 6/29/22 17:29, Tom de Vries via Gdb-patches wrote:
> When building gdb with -fsanitize=thread and gcc 12, and running test-case
> gdb.dwarf2/dwz.exp, we run into a data race between thread T2 and the main
> thread in the same write:
> ...
> Write of size 1 at 0x7b200000300c:^M
>      #0 cutu_reader::cutu_reader(dwarf2_per_cu_data*, dwarf2_per_objfile*, \
>      abbrev_table*, dwarf2_cu*, bool, abbrev_cache*) gdb/dwarf2/read.c:6252 \
>      (gdb+0x82f3b3)^M
> ...
> which is here:
> ...
>           this_cu->dwarf_version = cu->header.version;
> ...
> 
> Both writes are called from the parallel for in dwarf2_build_psymtabs_hard,
> this one directly:
> ...
>      #1 process_psymtab_comp_unit gdb/dwarf2/read.c:6774 (gdb+0x8304d7)^M
>      #2 operator() gdb/dwarf2/read.c:7098 (gdb+0x8317be)^M
>      #3 operator() gdbsupport/parallel-for.h:163 (gdb+0x872380)^M
> ...
> and this via the PU import:
> ...
>      #1 cooked_indexer::ensure_cu_exists(cutu_reader*, dwarf2_per_objfile*, \
>      sect_offset, bool,  bool) gdb/dwarf2/read.c:17964 (gdb+0x85c43b)^M
>      #2 cooked_indexer::index_imported_unit(cutu_reader*, unsigned char const*, \
>      abbrev_info const*) gdb/dwarf2/read.c:18248 (gdb+0x85d8ff)^M
>      #3 cooked_indexer::index_dies(cutu_reader*, unsigned char const*, \
>      cooked_index_entry const*, bool) gdb/dwarf2/read.c:18302 (gdb+0x85dcdb)^M
>      #4 cooked_indexer::make_index(cutu_reader*) gdb/dwarf2/read.c:18443 \
>      (gdb+0x85e68a)^M
>      #5 process_psymtab_comp_unit gdb/dwarf2/read.c:6812 (gdb+0x830879)^M
>      #6 operator() gdb/dwarf2/read.c:7098 (gdb+0x8317be)^M
>      #7 operator() gdbsupport/parallel-for.h:171 (gdb+0x8723e2)^M
> ...
> 
> Fix this by setting the field earlier, in read_comp_units_from_section.
> 
> The write in cutu_reader::cutu_reader() is still needed, in case
> read_comp_units_from_section is not used.
> 
> Make the write conditional, such that it doesn't trigger if the field is
> already set by read_comp_units_from_section.
> 

I realized there's a similar write in the this_cu->is_debug_types == 
true clause in cutu_reader::cutu_reader(), which I missed.  So I 
introduced a member function set_version to make sure it's set in a 
consistent way.

Currently testing.

Thanks,
- Tom

[-- Attachment #2: 0002-gdb-symtab-Fix-data-race-on-per_cu-dwarf_version.patch --]
[-- Type: text/x-patch, Size: 5101 bytes --]

[gdb/symtab] Fix data race on per_cu->dwarf_version

When building gdb with -fsanitize=thread and gcc 12, and running test-case
gdb.dwarf2/dwz.exp, we run into a data race between thread T2 and the main
thread in the same write:
...
Write of size 1 at 0x7b200000300c:^M
    #0 cutu_reader::cutu_reader(dwarf2_per_cu_data*, dwarf2_per_objfile*, \
    abbrev_table*, dwarf2_cu*, bool, abbrev_cache*) gdb/dwarf2/read.c:6252 \
    (gdb+0x82f3b3)^M
...
which is here:
...
         this_cu->dwarf_version = cu->header.version;
...

Both writes are called from the parallel for in dwarf2_build_psymtabs_hard,
this one directly:
...
    #1 process_psymtab_comp_unit gdb/dwarf2/read.c:6774 (gdb+0x8304d7)^M
    #2 operator() gdb/dwarf2/read.c:7098 (gdb+0x8317be)^M
    #3 operator() gdbsupport/parallel-for.h:163 (gdb+0x872380)^M
...
and this via the PU import:
...
    #1 cooked_indexer::ensure_cu_exists(cutu_reader*, dwarf2_per_objfile*, \
    sect_offset, bool,  bool) gdb/dwarf2/read.c:17964 (gdb+0x85c43b)^M
    #2 cooked_indexer::index_imported_unit(cutu_reader*, unsigned char const*, \
    abbrev_info const*) gdb/dwarf2/read.c:18248 (gdb+0x85d8ff)^M
    #3 cooked_indexer::index_dies(cutu_reader*, unsigned char const*, \
    cooked_index_entry const*, bool) gdb/dwarf2/read.c:18302 (gdb+0x85dcdb)^M
    #4 cooked_indexer::make_index(cutu_reader*) gdb/dwarf2/read.c:18443 \
    (gdb+0x85e68a)^M
    #5 process_psymtab_comp_unit gdb/dwarf2/read.c:6812 (gdb+0x830879)^M
    #6 operator() gdb/dwarf2/read.c:7098 (gdb+0x8317be)^M
    #7 operator() gdbsupport/parallel-for.h:171 (gdb+0x8723e2)^M
...

Fix this by setting the field earlier, in read_comp_units_from_section.

The write in cutu_reader::cutu_reader() is still needed, in case
read_comp_units_from_section is not used (run the test-case with say, target
board cc-with-gdb-index).

Make the write conditional, such that it doesn't trigger if the field is
already set by read_comp_units_from_section.  Instead, verify that the
field has already has the value that we're trying to set it to.

Move this logic into into a member function set_version (in analogy to the
already present member function version) to make sure it's used consistenly,
and make the field private in order to enforce access through the member
functions.

Tested on x86_64-linux.

---
 gdb/dwarf2/read.c | 10 +++++++---
 gdb/dwarf2/read.h | 12 ++++++++++++
 2 files changed, 19 insertions(+), 3 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d5088395fb1..410cc909c26 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6235,7 +6235,7 @@ cutu_reader::cutu_reader (dwarf2_per_cu_data *this_cu,
 	  sig_type->type_offset_in_section =
 	    this_cu->sect_off + to_underlying (sig_type->type_offset_in_tu);
 
-	  this_cu->dwarf_version = cu->header.version;
+	  this_cu->set_version (cu->header.version);
 	}
       else
 	{
@@ -6249,7 +6249,7 @@ cutu_reader::cutu_reader (dwarf2_per_cu_data *this_cu,
 	    this_cu->length = cu->header.get_length ();
 	  else
 	    gdb_assert (this_cu->length == cu->header.get_length ());
-	  this_cu->dwarf_version = cu->header.version;
+	  this_cu->set_version (cu->header.version);
 	}
     }
 
@@ -7206,6 +7206,10 @@ read_comp_units_from_section (dwarf2_per_objfile *per_objfile,
       this_cu->length = cu_header.length + cu_header.initial_length_size;
       this_cu->is_dwz = is_dwz;
       this_cu->section = section;
+      /* Init this asap, to avoid a data race in the set_version in
+	 cutu_reader::cutu_reader (which may be run in parallel for the cooked
+	 index case).  */
+      this_cu->set_version (cu_header.version);
 
       info_ptr = info_ptr + this_cu->length;
       per_objfile->per_bfd->all_comp_units.push_back (std::move (this_cu));
@@ -11222,7 +11226,7 @@ open_and_init_dwo_file (dwarf2_cu *cu, const char *dwo_name,
   create_cus_hash_table (per_objfile, cu, *dwo_file, dwo_file->sections.info,
 			 dwo_file->cus);
 
-  if (cu->per_cu->dwarf_version < 5)
+  if (cu->per_cu->version () < 5)
     {
       create_debug_types_hash_table (per_objfile, dwo_file.get (),
 				     dwo_file->sections.types, dwo_file->tus);
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 51e02dfc457..f6f3d72a81b 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -122,9 +122,11 @@ struct dwarf2_per_cu_data
   sect_offset sect_off {};
   unsigned int length = 0;
 
+private:
   /* DWARF standard version this data has been read from (such as 4 or 5).  */
   unsigned char dwarf_version = 0;
 
+public:
   /* Flag indicating this compilation unit will be read in before
      any of the current compilation units are processed.  */
   unsigned int queued : 1;
@@ -290,6 +292,16 @@ struct dwarf2_per_cu_data
     return dwarf_version;
   }
 
+  void set_version (short version)
+  {
+    if (dwarf_version == 0)
+      /* Set if not set already.  */
+      dwarf_version = version;
+    else
+      /* If already set, verify that it's the same value.  */
+      gdb_assert (dwarf_version == version);
+  }
+
   /* Free any cached file names.  */
   void free_cached_file_names ();
 };

Re: [PATCH 2/5] [gdb/symtab] Fix data race on per_cu->dwarf_version

[Tom de Vries]

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

On 7/1/22 13:16, Tom de Vries via Gdb-patches wrote:
> On 6/29/22 17:29, Tom de Vries via Gdb-patches wrote:
>> When building gdb with -fsanitize=thread and gcc 12, and running 
>> test-case
>> gdb.dwarf2/dwz.exp, we run into a data race between thread T2 and the 
>> main
>> thread in the same write:
>> ...
>> Write of size 1 at 0x7b200000300c:^M
>>      #0 cutu_reader::cutu_reader(dwarf2_per_cu_data*, 
>> dwarf2_per_objfile*, \
>>      abbrev_table*, dwarf2_cu*, bool, abbrev_cache*) 
>> gdb/dwarf2/read.c:6252 \
>>      (gdb+0x82f3b3)^M
>> ...
>> which is here:
>> ...
>>           this_cu->dwarf_version = cu->header.version;
>> ...
>>
>> Both writes are called from the parallel for in 
>> dwarf2_build_psymtabs_hard,
>> this one directly:
>> ...
>>      #1 process_psymtab_comp_unit gdb/dwarf2/read.c:6774 (gdb+0x8304d7)^M
>>      #2 operator() gdb/dwarf2/read.c:7098 (gdb+0x8317be)^M
>>      #3 operator() gdbsupport/parallel-for.h:163 (gdb+0x872380)^M
>> ...
>> and this via the PU import:
>> ...
>>      #1 cooked_indexer::ensure_cu_exists(cutu_reader*, 
>> dwarf2_per_objfile*, \
>>      sect_offset, bool,  bool) gdb/dwarf2/read.c:17964 (gdb+0x85c43b)^M
>>      #2 cooked_indexer::index_imported_unit(cutu_reader*, unsigned 
>> char const*, \
>>      abbrev_info const*) gdb/dwarf2/read.c:18248 (gdb+0x85d8ff)^M
>>      #3 cooked_indexer::index_dies(cutu_reader*, unsigned char const*, \
>>      cooked_index_entry const*, bool) gdb/dwarf2/read.c:18302 
>> (gdb+0x85dcdb)^M
>>      #4 cooked_indexer::make_index(cutu_reader*) 
>> gdb/dwarf2/read.c:18443 \
>>      (gdb+0x85e68a)^M
>>      #5 process_psymtab_comp_unit gdb/dwarf2/read.c:6812 (gdb+0x830879)^M
>>      #6 operator() gdb/dwarf2/read.c:7098 (gdb+0x8317be)^M
>>      #7 operator() gdbsupport/parallel-for.h:171 (gdb+0x8723e2)^M
>> ...
>>
>> Fix this by setting the field earlier, in read_comp_units_from_section.
>>
>> The write in cutu_reader::cutu_reader() is still needed, in case
>> read_comp_units_from_section is not used.
>>
>> Make the write conditional, such that it doesn't trigger if the field is
>> already set by read_comp_units_from_section.
>>
> 
> I realized there's a similar write in the this_cu->is_debug_types == 
> true clause in cutu_reader::cutu_reader(), which I missed.  So I 
> introduced a member function set_version to make sure it's set in a 
> consistent way.
> 

This is what I ended up committing.

Further changes:
- add assert in set_version
- rename field to m_dwarf_version.

Thanks,
- Tom



[-- Attachment #2: 0001-gdb-symtab-Fix-data-race-on-per_cu-dwarf_version.patch --]
[-- Type: text/x-patch, Size: 5484 bytes --]

[gdb/symtab] Fix data race on per_cu->dwarf_version

When building gdb with -fsanitize=thread and gcc 12, and running test-case
gdb.dwarf2/dwz.exp, we run into a data race between thread T2 and the main
thread in the same write:
...
Write of size 1 at 0x7b200000300c:^M
    #0 cutu_reader::cutu_reader(dwarf2_per_cu_data*, dwarf2_per_objfile*, \
    abbrev_table*, dwarf2_cu*, bool, abbrev_cache*) gdb/dwarf2/read.c:6252 \
    (gdb+0x82f3b3)^M
...
which is here:
...
         this_cu->dwarf_version = cu->header.version;
...

Both writes are called from the parallel for in dwarf2_build_psymtabs_hard,
this one directly:
...
    #1 process_psymtab_comp_unit gdb/dwarf2/read.c:6774 (gdb+0x8304d7)^M
    #2 operator() gdb/dwarf2/read.c:7098 (gdb+0x8317be)^M
    #3 operator() gdbsupport/parallel-for.h:163 (gdb+0x872380)^M
...
and this via the PU import:
...
    #1 cooked_indexer::ensure_cu_exists(cutu_reader*, dwarf2_per_objfile*, \
    sect_offset, bool,  bool) gdb/dwarf2/read.c:17964 (gdb+0x85c43b)^M
    #2 cooked_indexer::index_imported_unit(cutu_reader*, unsigned char const*, \
    abbrev_info const*) gdb/dwarf2/read.c:18248 (gdb+0x85d8ff)^M
    #3 cooked_indexer::index_dies(cutu_reader*, unsigned char const*, \
    cooked_index_entry const*, bool) gdb/dwarf2/read.c:18302 (gdb+0x85dcdb)^M
    #4 cooked_indexer::make_index(cutu_reader*) gdb/dwarf2/read.c:18443 \
    (gdb+0x85e68a)^M
    #5 process_psymtab_comp_unit gdb/dwarf2/read.c:6812 (gdb+0x830879)^M
    #6 operator() gdb/dwarf2/read.c:7098 (gdb+0x8317be)^M
    #7 operator() gdbsupport/parallel-for.h:171 (gdb+0x8723e2)^M
...

Fix this by setting the field earlier, in read_comp_units_from_section.

The write in cutu_reader::cutu_reader() is still needed, in case
read_comp_units_from_section is not used (run the test-case with say, target
board cc-with-gdb-index).

Make the write conditional, such that it doesn't trigger if the field is
already set by read_comp_units_from_section.  Instead, verify that the
field already has the value that we're trying to set it to.

Move this logic into into a member function set_version (in analogy to the
already present member function version) to make sure it's used consistenly,
and make the field private in order to enforce access through the member
functions, and rename it to m_dwarf_version.

While we're at it, make sure that the version is set before read, to avoid
say returning true for "per_cu.version () < 5" if "per_cu.version () == 0".

Tested on x86_64-linux.

---
 gdb/dwarf2/read.c | 10 +++++++---
 gdb/dwarf2/read.h | 18 ++++++++++++++++--
 2 files changed, 23 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index d5088395fb1..410cc909c26 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6235,7 +6235,7 @@ cutu_reader::cutu_reader (dwarf2_per_cu_data *this_cu,
 	  sig_type->type_offset_in_section =
 	    this_cu->sect_off + to_underlying (sig_type->type_offset_in_tu);
 
-	  this_cu->dwarf_version = cu->header.version;
+	  this_cu->set_version (cu->header.version);
 	}
       else
 	{
@@ -6249,7 +6249,7 @@ cutu_reader::cutu_reader (dwarf2_per_cu_data *this_cu,
 	    this_cu->length = cu->header.get_length ();
 	  else
 	    gdb_assert (this_cu->length == cu->header.get_length ());
-	  this_cu->dwarf_version = cu->header.version;
+	  this_cu->set_version (cu->header.version);
 	}
     }
 
@@ -7206,6 +7206,10 @@ read_comp_units_from_section (dwarf2_per_objfile *per_objfile,
       this_cu->length = cu_header.length + cu_header.initial_length_size;
       this_cu->is_dwz = is_dwz;
       this_cu->section = section;
+      /* Init this asap, to avoid a data race in the set_version in
+	 cutu_reader::cutu_reader (which may be run in parallel for the cooked
+	 index case).  */
+      this_cu->set_version (cu_header.version);
 
       info_ptr = info_ptr + this_cu->length;
       per_objfile->per_bfd->all_comp_units.push_back (std::move (this_cu));
@@ -11222,7 +11226,7 @@ open_and_init_dwo_file (dwarf2_cu *cu, const char *dwo_name,
   create_cus_hash_table (per_objfile, cu, *dwo_file, dwo_file->sections.info,
 			 dwo_file->cus);
 
-  if (cu->per_cu->dwarf_version < 5)
+  if (cu->per_cu->version () < 5)
     {
       create_debug_types_hash_table (per_objfile, dwo_file.get (),
 				     dwo_file->sections.types, dwo_file->tus);
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 51e02dfc457..b7a03933aa5 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -122,9 +122,11 @@ struct dwarf2_per_cu_data
   sect_offset sect_off {};
   unsigned int length = 0;
 
+private:
   /* DWARF standard version this data has been read from (such as 4 or 5).  */
-  unsigned char dwarf_version = 0;
+  unsigned char m_dwarf_version = 0;
 
+public:
   /* Flag indicating this compilation unit will be read in before
      any of the current compilation units are processed.  */
   unsigned int queued : 1;
@@ -287,7 +289,19 @@ struct dwarf2_per_cu_data
   /* Return DWARF version number of this CU.  */
   short version () const
   {
-    return dwarf_version;
+    /* Make sure it's set already.  */
+    gdb_assert (m_dwarf_version != 0);
+    return m_dwarf_version;
+  }
+
+  void set_version (short version)
+  {
+    if (m_dwarf_version == 0)
+      /* Set if not set already.  */
+      m_dwarf_version = version;
+    else
+      /* If already set, verify that it's the same value.  */
+      gdb_assert (m_dwarf_version == version);
   }
 
   /* Free any cached file names.  */

Re: [PATCH 3/5] [gdb/symtab] Work around fsanitize=address false positive for per_ cu->lang

[Tom de Vries]

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

On 6/29/22 20:28, Pedro Alves wrote:
> On 2022-06-29 19:25, Pedro Alves wrote:
>> On 2022-06-29 18:38, Pedro Alves wrote:
>>> On 2022-06-29 16:29, Tom de Vries via Gdb-patches wrote:
>>>> 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;
>>>> ...
>>>>
>>>> Both fields are part of the same bitfield, and writing to one field while
>>>> reading from another is not a problem, so this is a false positive.
>>>
>>> I don't understand this sentence.  Particularly "same bitfield", or
>>> really "Both fields are part of the same bitfield,".  How can two bitfields
>>> be part of the same bitfield?
>>>
>>> Anyhow, both bitfields are part of a sequence of contiguous bitfields, here
>>> stripped of comments:
>>>
>>>    unsigned int queued : 1;
>>>    unsigned int is_debug_types : 1;
>>>    unsigned int is_dwz : 1;
>>>    unsigned int reading_dwo_directly : 1;
>>>    unsigned int tu_read : 1;
>>>    mutable bool m_header_read_in : 1;
>>>    bool addresses_seen : 1;
>>>    unsigned int mark : 1;
>>>    bool files_read : 1;
>>>    ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
>>>    ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
>>>
>>> Per C++11, they're all part of the same _memory location_.  From N3253 (C++11), intro.memory:
>>>
>>>   "A memory location is either an object of scalar type or a maximal sequence of adjacent bit-fields all having
>>>   non-zero width. (...)  Two threads of execution (1.10) can update and access separate memory locations
>>>    without interfering with each other.
>>>   (...)
>>>   [ Note: Thus a bit-field and an adjacent non-bit-field are in separate memory locations, and therefore can be
>>>   concurrently updated by two threads of execution without interference. The same applies to two bit-fields,
>>>   if one is declared inside a nested struct declaration and the other is not, or if the two are separated by
>>>   a zero-length bit-field declaration, or if they are separated by a non-bit-field declaration. It is not safe to
>>>   concurrently update two bit-fields in the same struct if all fields between them are also bit-fields of non-zero
>>>   width. — end note ]"
>>>
>>> And while it is true that in practice writing to one bit-field from one thread and reading from another,
>>> if they reside on the same location, is OK in practice, it is still undefined behavior.

Ack, thanks for pointing that out, I was not aware of this.

I've reformulated things in terms of "memory location".

>>>
>>> Note the escape hatch mentioned above:
>>>
>>>     "if the two are separated by a zero-length bit-field declaration"
>>>
>>> Thus, a change like this:
>>>
>>>    unsigned int queued : 1;
>>>    unsigned int is_debug_types : 1;
>>>    unsigned int is_dwz : 1;
>>>    unsigned int reading_dwo_directly : 1;
>>>    unsigned int tu_read : 1;
>>>    mutable bool m_header_read_in : 1;
>>>    bool addresses_seen : 1;
>>>    unsigned int mark : 1;
>>>    bool files_read : 1;
>>>    ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
>>> +
>>> +  /* Ensure lang is a separate memory location, so we can update
>>> +     it concurrently with other bitfields.  */
>>> +  char :0;
>>> +
>>>    ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
>>>
>>>
>>> ... should work.
>>
>> The "if one is declared inside a nested struct declaration and the other
>> is not" escape hatch may be interesting too, as in, we'd write:
>>
>>     struct {
>>       ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
>>     };
>>
>> ... and since the struct is anonymous, nothing else needs to change.
>>
>> In patch #4, you'd just do this too:
>>
>>    struct {
>>       ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
>>    };
>>
>> The "wrapping" syntax seems to read a bit better, particularly since this
>> way you don't have to worry about putting a :0 bitfield before and
>> another after.
> 

Done.

> I keep coming back, sorry...  :-P
> 
> Another thought is that in both patches #3 and #4, it's reading_dwo_directly
> that is racing with two other bitfields.  So I wonder whether it's _that_ one
> that should be moved to a separate memory location.
	
I've also tried that, but I got similar errors back, with the same 
writes but different reads.

Also added field addresses_seen, which I found using testing with board 
cc-with-dwz-m.

Thanks,
- Tom

[-- Attachment #2: 0001-gdb-symtab-Fix-fsanitize-address-errors-for-per_cu-fields.patch --]
[-- Type: text/x-patch, Size: 3637 bytes --]

[gdb/symtab] Fix fsanitize=address errors 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 fixes it:
...
  struct {
    ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
  };
  struct {
    ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
  };
  struct {
    bool addresses_seen : 1;
  };
...

This increases the size of struct dwarf2_per_cu_data from 120 to 128 (for -m64).

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.

Tested on x86_64-linux.

---
 gdb/dwarf2/read.h | 17 ++++++++++++-----
 1 file changed, 12 insertions(+), 5 deletions(-)

diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index b7a03933aa5..c4b007d064d 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -163,7 +163,9 @@ struct dwarf2_per_cu_data
 
   /* If addresses have been read for this CU (usually from
      .debug_aranges), then this flag is set.  */
-  bool addresses_seen : 1;
+  struct {
+    bool addresses_seen : 1;
+  };
 
   /* A temporary mark bit used when iterating over all CUs in
      expand_symtabs_matching.  */
@@ -173,11 +175,16 @@ struct dwarf2_per_cu_data
      point in trying to read it again next time.  */
   bool files_read : 1;
 
-  /* The unit type of this CU.  */
-  ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
+  /* Put this in a struct to ensure a separate memory location.  */
+  struct {
+    /* The unit type of this CU.  */
+    ENUM_BITFIELD (dwarf_unit_type) unit_type : 8;
+  };
 
-  /* The language of this CU.  */
-  ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
+  struct {
+    /* The language of this CU.  */
+    ENUM_BITFIELD (language) lang : LANGUAGE_BITS;
+  };
 
   /* True if this CU has been scanned by the indexer; false if
      not.  */

Re: [PATCH 5/5] [gdb/symtab] Fix data race on per_cu->lang

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Both writes are called from the parallel for in dwarf2_build_psymtabs_hard,
Tom> this one directly:
Tom> ...
Tom>     #1 process_psymtab_comp_unit gdb/dwarf2/read.c:6812 (gdb+0x830912)
Tom>     #2 operator() gdb/dwarf2/read.c:7102 (gdb+0x831902)
Tom>     #3 operator() gdb/../gdbsupport/parallel-for.h:171 (gdb+0x8723a8)
Tom> ...
Tom> and this one when handling cross-CU refs:
Tom> ...
Tom>     #1 cooked_indexer::ensure_cu_exists(cutu_reader*, dwarf2_per_objfile*, \
Tom>     sect_offset, bool, bool) gdb/dwarf2/read.c:17973 (gdb+0x85c522)

This method tries to ensure that a CU isn't processed twice, using the
'scanned' field.  Do you know why this isn't working?

Tom> Fix this by guarding the write with a lock.

I would rather we avoid locks.  Ideally the existing exclusion mechanism
should be made to work, but if that fails, perhaps we can use another
atomic.

Tom

Re: [PATCH 3/5] [gdb/symtab] Work around fsanitize=address false positive for per_cu->lang

[Tom Tromey]

>>>>> "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.

Tom

Re: [PATCH 2/5] [gdb/symtab] Fix data race on per_cu->dwarf_version

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> Fix this by setting the field earlier, in read_comp_units_from_section.

Thanks.

Tom> The write in cutu_reader::cutu_reader() is still needed, in case
Tom> read_comp_units_from_section is not used (run the test-case with say, target
Tom> board cc-with-gdb-index).

I wonder if we should just change this code to always scan the DWARF
just enough to read the headers.  Like, would that really be so much
slower?  (Eons ago I thought so but now I don't really know.)  If it
would simplify the code it seems like it would be a net win.

Tom

Re: [PATCH 2/5] [gdb/symtab] Fix data race on per_cu->dwarf_version

[Tom de Vries]

On 7/4/22 20:51, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Fix this by setting the field earlier, in read_comp_units_from_section.
> 
> Thanks.
> 
> Tom> The write in cutu_reader::cutu_reader() is still needed, in case
> Tom> read_comp_units_from_section is not used (run the test-case with say, target
> Tom> board cc-with-gdb-index).
> 
> I wonder if we should just change this code to always scan the DWARF
> just enough to read the headers.  Like, would that really be so much
> slower?  (Eons ago I thought so but now I don't really know.)  If it
> would simplify the code it seems like it would be a net win.

I wondered the same, but then including the top-level die.  That would 
allow us to get the language before going into the parallel for.  But I 
suppose that's more complex than just the headers.

Thanks,
- Tom

Re: [PATCH 3/5] [gdb/symtab] Work around fsanitize=address false positive for per_cu->lang

[Tom de Vries]

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;
+  };
...

Thanks,
- Tom

Re: [PATCH 2/5] [gdb/symtab] Fix data race on per_cu->dwarf_version

[Tom Tromey]

>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:

Tom> On 7/4/22 20:51, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
Tom> Fix this by setting the field earlier, in
>> read_comp_units_from_section.
>> Thanks.
Tom> The write in cutu_reader::cutu_reader() is still needed, in
>> case
Tom> read_comp_units_from_section is not used (run the test-case with say, target
Tom> board cc-with-gdb-index).
>> I wonder if we should just change this code to always scan the DWARF
>> just enough to read the headers.  Like, would that really be so much
>> slower?  (Eons ago I thought so but now I don't really know.)  If it
>> would simplify the code it seems like it would be a net win.

Tom> I wondered the same, but then including the top-level die.  That would
Tom> allow us to get the language before going into the parallel for.  But
Tom> I suppose that's more complex than just the headers.

Yeah, that's harder since it would have to read abbrevs, etc.
This part of the reader is a real mess.  There is a seemingly redundant
copy of the header info as well, used by DWO.  This is surprisingly hard
to untangle :(

Tom

Re: [PATCH 5/5] [gdb/symtab] Fix data race on per_cu->lang

[Tom de Vries]

On 7/4/22 20:30, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> 
> Tom> Both writes are called from the parallel for in dwarf2_build_psymtabs_hard,
> Tom> this one directly:
> Tom> ...
> Tom>     #1 process_psymtab_comp_unit gdb/dwarf2/read.c:6812 (gdb+0x830912)
> Tom>     #2 operator() gdb/dwarf2/read.c:7102 (gdb+0x831902)
> Tom>     #3 operator() gdb/../gdbsupport/parallel-for.h:171 (gdb+0x8723a8)
> Tom> ...
> Tom> and this one when handling cross-CU refs:
> Tom> ...
> Tom>     #1 cooked_indexer::ensure_cu_exists(cutu_reader*, dwarf2_per_objfile*, \
> Tom>     sect_offset, bool, bool) gdb/dwarf2/read.c:17973 (gdb+0x85c522)
> 
> This method tries to ensure that a CU isn't processed twice, using the
> 'scanned' field.  Do you know why this isn't working?
> 

Yes, because for_scanning == false, so this:
...
   /* When scanning, we only want to visit a given CU a single time. 

      Doing this check here avoids self-imports as well.  */
   if (for_scanning)
     {
       bool nope = false;
       if (!per_cu->scanned.compare_exchange_strong (nope, true))
         return nullptr;
     }
...
is not actived.

A quick experiment of setting it to true at the call site:
...
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 23fe5679cbd..f85263564cb 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -18143,7 +18143,7 @@ cooked_indexer::scan_attributes 
(dwarf2_per_cu_data *scanning_
per_cu,
      {
        cutu_reader *new_reader
         = ensure_cu_exists (reader, reader->cu->per_objfile, origin_offset,
-                           origin_is_dwz, false);
+                           origin_is_dwz, true);
        if (new_reader != nullptr)
         {
           const gdb_byte *new_info_ptr = (new_reader->buffer
...
makes thread sanitizer stop complaining, but also makes the test-case fail:
...
FAIL: gdb.dwarf2/inlined_subroutine-inheritance.exp: gdb_breakpoint: set 
breakpoint at bytes_repeat
...

Thanks,
- Tom

> Tom> Fix this by guarding the write with a lock.
> 
> I would rather we avoid locks.  Ideally the existing exclusion mechanism
> should be made to work, but if that fails, perhaps we can use another
> atomic.
> 
> Tom

Re: [PATCH 5/5] [gdb/symtab] Fix data race on per_cu->lang

[Tom de Vries]

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

On 7/4/22 20:30, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
> Tom> Fix this by guarding the write with a lock.
> 
> I would rather we avoid locks.  Ideally the existing exclusion mechanism
> should be made to work, but if that fails, perhaps we can use another
> atomic.

I gave a different approach a try: eliminating the field.

That seems to work reasonably well, apart from one specific usage that I 
disabled (mentioned in commit message).

I'm not sure this is a good idea though, but I thought I could at least 
post it.  Currently testing.

Thanks,
- Tom


[-- Attachment #2: 0002-gdb-symtab-Eliminate-per_cu-lang.patch --]
[-- Type: text/x-patch, Size: 44427 bytes --]

[gdb/symtab] Eliminate per_cu->lang

[ Note: written on top of this (
https://sourceware.org/pipermail/gdb-patches/2022-July/190476.html ) patch. ]

Eliminate the field dwarf2_per_cu_data::lang.

Fix most uses by replacing cu->per_cu->lang () with cu->lang ().

The remaining uses are in the cooked index code.  Fix those by some sort of
side-table approach, where we define a new struct:
...
struct per_cu_lang
{
  dwarf2_per_cu_data *per_cu;
  enum language lang;
};
...
and replace dwarf2_per_cu_data handling with per_cu_lang handling in the code.

In order not to allocate identical per_cu_lang objects, we enter new entries
in a map.  Making the map thread_local fixes the data races we set out to fix
in the first place.

The only casualty is this code in process_imported_unit_die:
...
      /* We're importing a C++ compilation unit with tag DW_TAG_compile_unit
	 into another compilation unit, at root level.  Regard this as a hint,
	 and ignore it.  */
      if (die->parent && die->parent->parent == NULL
	  && per_cu->unit_type () == DW_UT_compile
	  && per_cu->lang () == language_cplus)
	return;
...
which is disabled because dwarf2_find_containing_comp_unit returns a
dwarf2_per_cu_data and it's not clear how to get at the language from
there.

---
 gdb/dwarf2/cooked-index.c |  20 +++--
 gdb/dwarf2/cooked-index.h |  39 ++++++--
 gdb/dwarf2/cu.c           |   2 +-
 gdb/dwarf2/cu.h           |   7 ++
 gdb/dwarf2/index-write.c  |   8 +-
 gdb/dwarf2/read.c         | 223 +++++++++++++++++++++++-----------------------
 gdb/dwarf2/read.h         |  21 -----
 7 files changed, 168 insertions(+), 152 deletions(-)

diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index adb0046609e..7f98b301c8f 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -54,7 +54,7 @@ cooked_index_entry::full_name (struct obstack *storage) const
     return canonical;
 
   const char *sep = nullptr;
-  switch (per_cu->lang ())
+  switch (m_per_cu_lang->lang)
     {
     case language_cplus:
     case language_rust:
@@ -94,17 +94,18 @@ const cooked_index_entry *
 cooked_index::add (sect_offset die_offset, enum dwarf_tag tag,
 		   cooked_index_flag flags, const char *name,
 		   const cooked_index_entry *parent_entry,
-		   dwarf2_per_cu_data *per_cu)
+		   dwarf2_per_cu_data *per_cu,
+		   enum language lang)
 {
   cooked_index_entry *result = create (die_offset, tag, flags, name,
-				       parent_entry, per_cu);
+				       parent_entry, per_cu, lang);
   m_entries.push_back (result);
 
   /* An explicitly-tagged main program should always override the
      implicit "main" discovery.  */
   if ((flags & IS_MAIN) != 0)
     m_main = result;
-  else if (per_cu->lang () != language_ada
+  else if (lang != language_ada
 	   && m_main == nullptr
 	   && strcmp (name, "main") == 0)
     m_main = result;
@@ -146,13 +147,14 @@ cooked_index::handle_gnat_encoded_entry (cooked_index_entry *entry,
       /* CUs are processed in order, so we only need to check the most
 	 recent entry.  */
       cooked_index_entry *last = (cooked_index_entry *) *slot;
-      if (last == nullptr || last->per_cu != entry->per_cu)
+      if (last == nullptr || last->m_per_cu_lang->per_cu != entry->m_per_cu_lang->per_cu)
 	{
 	  gdb::unique_xmalloc_ptr<char> new_name
 	    = make_unique_xstrndup (name.data (), name.length ());
 	  last = create (entry->die_offset, DW_TAG_namespace,
 			 0, new_name.get (), parent,
-			 entry->per_cu);
+			 entry->m_per_cu_lang->per_cu,
+			 entry->m_per_cu_lang->lang);
 	  last->canonical = last->name;
 	  m_names.push_back (std::move (new_name));
 	  *slot = last;
@@ -196,13 +198,13 @@ cooked_index::do_finalize ()
   for (cooked_index_entry *entry : m_entries)
     {
       gdb_assert (entry->canonical == nullptr);
-      if ((entry->per_cu->lang () != language_cplus
-	   && entry->per_cu->lang () != language_ada)
+      if ((entry->m_per_cu_lang->lang != language_cplus
+	   && entry->m_per_cu_lang->lang != language_ada)
 	  || (entry->flags & IS_LINKAGE) != 0)
 	entry->canonical = entry->name;
       else
 	{
-	  if (entry->per_cu->lang () == language_ada)
+	  if (entry->m_per_cu_lang->lang == language_ada)
 	    {
 	      gdb::unique_xmalloc_ptr<char> canon_name
 		= handle_gnat_encoded_entry (entry, gnat_entries.get ());
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 439cbb19fa7..761b4d1266a 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -34,9 +34,21 @@
 #include "dwarf2/mapped-index.h"
 #include "dwarf2/tag.h"
 #include "gdbsupport/range-chain.h"
+#include <utility>
+#include <map>
 
 struct dwarf2_per_cu_data;
 
+struct per_cu_lang
+{
+  per_cu_lang (dwarf2_per_cu_data *_per_cu, enum language _lang)
+    : per_cu(_per_cu), lang(_lang)
+  {}
+
+  dwarf2_per_cu_data *per_cu;
+  enum language lang;
+};
+
 /* Flags that describe an entry in the index.  */
 enum cooked_index_flag_enum : unsigned char
 {
@@ -63,14 +75,24 @@ struct cooked_index_entry : public allocate_on_obstack
   cooked_index_entry (sect_offset die_offset_, enum dwarf_tag tag_,
 		      cooked_index_flag flags_, const char *name_,
 		      const cooked_index_entry *parent_entry_,
-		      dwarf2_per_cu_data *per_cu_)
+		      dwarf2_per_cu_data *per_cu_,
+		      enum language lang_)
     : name (name_),
       tag (tag_),
       flags (flags_),
       die_offset (die_offset_),
-      parent_entry (parent_entry_),
-      per_cu (per_cu_)
+      parent_entry (parent_entry_)
   {
+    static thread_local std::map<std::pair<dwarf2_per_cu_data *, enum language>, per_cu_lang *> map;
+    auto it = map.find (std::make_pair (per_cu_, lang_));
+    if (it != map.end())
+      {
+	m_per_cu_lang = it->second;
+	return;
+      }
+
+    m_per_cu_lang = new per_cu_lang (per_cu_, lang_);
+    map[std::make_pair (per_cu_, lang_)] = m_per_cu_lang;
   }
 
   /* Return true if this entry matches SEARCH_FLAGS.  */
@@ -154,7 +176,7 @@ struct cooked_index_entry : public allocate_on_obstack
      class.  */
   const cooked_index_entry *parent_entry;
   /* The CU from which this entry originates.  */
-  dwarf2_per_cu_data *per_cu;
+  per_cu_lang* m_per_cu_lang;
 
 private:
 
@@ -182,7 +204,8 @@ class cooked_index
 				 cooked_index_flag flags,
 				 const char *name,
 				 const cooked_index_entry *parent_entry,
-				 dwarf2_per_cu_data *per_cu);
+				 dwarf2_per_cu_data *per_cu,
+				 enum language lang);
 
   /* Install a new fixed addrmap from the given mutable addrmap.  */
   void install_addrmap (addrmap_mutable *map)
@@ -243,11 +266,13 @@ class cooked_index
 			      cooked_index_flag flags,
 			      const char *name,
 			      const cooked_index_entry *parent_entry,
-			      dwarf2_per_cu_data *per_cu)
+			      dwarf2_per_cu_data *per_cu,
+			      enum language lang)
   {
     return new (&m_storage) cooked_index_entry (die_offset, tag, flags,
 						name, parent_entry,
-						per_cu);
+						per_cu,
+						lang);
   }
 
   /* GNAT only emits mangled ("encoded") names in the DWARF, and does
diff --git a/gdb/dwarf2/cu.c b/gdb/dwarf2/cu.c
index 8cbd97661a5..d40dd094b09 100644
--- a/gdb/dwarf2/cu.c
+++ b/gdb/dwarf2/cu.c
@@ -62,7 +62,7 @@ dwarf2_cu::start_compunit_symtab (const char *name, const char *comp_dir,
 
   m_builder.reset (new struct buildsym_compunit
 		   (this->per_objfile->objfile,
-		    name, comp_dir, per_cu->lang (), low_pc));
+		    name, comp_dir, lang (), low_pc));
 
   list_in_scope = get_builder ()->get_file_symbols ();
 
diff --git a/gdb/dwarf2/cu.h b/gdb/dwarf2/cu.h
index 6b72ec234bf..23cb3d21b2e 100644
--- a/gdb/dwarf2/cu.h
+++ b/gdb/dwarf2/cu.h
@@ -23,6 +23,7 @@
 #include "buildsym.h"
 #include "dwarf2/comp-unit-head.h"
 #include "gdbsupport/gdb_optional.h"
+#include "language.h"
 
 /* Type used for delaying computation of method physnames.
    See comments for compute_delayed_physnames.  */
@@ -105,6 +106,12 @@ struct dwarf2_cu
   /* The language we are debugging.  */
   const struct language_defn *language_defn = nullptr;
 
+  enum language lang () const
+  {
+    gdb_assert (language_defn != language_def (language_unknown));
+    return language_defn->la_language;
+  }
+
   const char *producer = nullptr;
 
 private:
diff --git a/gdb/dwarf2/index-write.c b/gdb/dwarf2/index-write.c
index c3aad8dd999..7ee8f721171 100644
--- a/gdb/dwarf2/index-write.c
+++ b/gdb/dwarf2/index-write.c
@@ -567,7 +567,7 @@ class debug_names
 
   void insert (const cooked_index_entry *entry)
   {
-    const auto it = m_cu_index_htab.find (entry->per_cu);
+    const auto it = m_cu_index_htab.find (entry->m_per_cu_lang->per_cu);
     gdb_assert (it != m_cu_index_htab.cend ());
     const char *name = entry->full_name (&m_string_obstack);
 
@@ -582,8 +582,8 @@ class debug_names
       tag = DW_TAG_variable;
 
     insert (tag, name, it->second, (entry->flags & IS_STATIC) != 0,
-	    entry->per_cu->is_debug_types ? unit_kind::tu : unit_kind::cu,
-	    entry->per_cu->lang ());
+	    entry->m_per_cu_lang->per_cu->is_debug_types ? unit_kind::tu : unit_kind::cu,
+	    entry->m_per_cu_lang->lang);
   }
 
   /* Build all the tables.  All symbols must be already inserted.
@@ -1122,7 +1122,7 @@ write_cooked_index (cooked_index_vector *table,
       if ((entry->flags & IS_LINKAGE) != 0)
 	continue;
 
-      const auto it = cu_index_htab.find (entry->per_cu);
+      const auto it = cu_index_htab.find (entry->m_per_cu_lang->per_cu);
       gdb_assert (it != cu_index_htab.cend ());
 
       const char *name = entry->full_name (&symtab->m_string_obstack);
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 23fe5679cbd..bc19f36e7b3 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -6594,9 +6594,10 @@ class cooked_index_storage
 				 cooked_index_flag flags,
 				 const char *name,
 				 const cooked_index_entry *parent_entry,
-				 dwarf2_per_cu_data *per_cu)
+				 dwarf2_per_cu_data *per_cu,
+				 enum language lang)
   {
-    return m_index->add (die_offset, tag, flags, name, parent_entry, per_cu);
+    return m_index->add (die_offset, tag, flags, name, parent_entry, per_cu, lang);
   }
 
   /* Install the current addrmap into the index being constructed,
@@ -6808,7 +6809,7 @@ process_psymtab_comp_unit (dwarf2_per_cu_data *this_cu,
 	  prepare_one_comp_unit (reader.cu, reader.comp_unit_die,
 				 language_minimal);
 	  gdb_assert (storage != nullptr);
-	  cooked_indexer indexer (storage, this_cu, reader.cu->per_cu->lang ());
+	  cooked_indexer indexer (storage, this_cu, reader.cu->lang ());
 	  indexer.make_index (&reader);
 	}
     }
@@ -6832,7 +6833,7 @@ build_type_psymtabs_reader (cutu_reader *reader,
   prepare_one_comp_unit (cu, type_unit_die, language_minimal);
 
   gdb_assert (storage != nullptr);
-  cooked_indexer indexer (storage, per_cu, cu->per_cu->lang ());
+  cooked_indexer indexer (storage, per_cu, cu->lang ());
   indexer.make_index (reader);
 }
 
@@ -7142,7 +7143,7 @@ dwarf2_build_psymtabs_hard (dwarf2_per_objfile *per_objfile)
   const cooked_index_entry *main_entry = vec->get_main ();
   if (main_entry != nullptr)
     set_objfile_main_name (objfile, main_entry->name,
-			   main_entry->per_cu->lang ());
+			   main_entry->m_per_cu_lang->lang);
 
   dwarf_read_debug_printf ("Done building psymtabs of %s",
 			   objfile_name (objfile));
@@ -7745,7 +7746,7 @@ compute_delayed_physnames (struct dwarf2_cu *cu)
   /* Only C++ delays computing physnames.  */
   if (cu->method_list.empty ())
     return;
-  gdb_assert (cu->per_cu->lang () == language_cplus);
+  gdb_assert (cu->lang () == language_cplus);
 
   for (const delayed_method_info &mi : cu->method_list)
     {
@@ -8174,7 +8175,7 @@ quirk_rust_enum (struct type *type, struct objfile *objfile)
 static void
 rust_union_quirks (struct dwarf2_cu *cu)
 {
-  gdb_assert (cu->per_cu->lang () == language_rust);
+  gdb_assert (cu->lang () == language_rust);
   for (type *type_ : cu->rust_unions)
     quirk_rust_enum (type_, cu->per_objfile->objfile);
   /* We don't need this any more.  */
@@ -8368,7 +8369,7 @@ process_full_comp_unit (dwarf2_cu *cu, enum language pretend_language)
   process_die (cu->dies, cu);
 
   /* For now fudge the Go package.  */
-  if (cu->per_cu->lang () == language_go)
+  if (cu->lang () == language_go)
     fixup_go_packaging (cu);
 
   /* Now that we have processed all the DIEs in the CU, all the types
@@ -8376,7 +8377,7 @@ process_full_comp_unit (dwarf2_cu *cu, enum language pretend_language)
      physnames.  */
   compute_delayed_physnames (cu);
 
-  if (cu->per_cu->lang () == language_rust)
+  if (cu->lang () == language_rust)
     rust_union_quirks (cu);
 
   /* Some compilers don't define a DW_AT_high_pc attribute for the
@@ -8405,9 +8406,9 @@ process_full_comp_unit (dwarf2_cu *cu, enum language pretend_language)
       /* Set symtab language to language from DW_AT_language.  If the
 	 compilation is from a C file generated by language preprocessors, do
 	 not set the language if it was already deduced by start_subfile.  */
-      if (!(cu->per_cu->lang () == language_c
+      if (!(cu->lang () == language_c
 	    && cust->primary_filetab ()->language () != language_unknown))
-	cust->primary_filetab ()->set_language (cu->per_cu->lang ());
+	cust->primary_filetab ()->set_language (cu->lang ());
 
       /* GCC-4.0 has started to support -fvar-tracking.  GCC-3.x still can
 	 produce DW_AT_location with location lists but it can be possibly
@@ -8461,7 +8462,7 @@ process_full_type_unit (dwarf2_cu *cu,
   process_die (cu->dies, cu);
 
   /* For now fudge the Go package.  */
-  if (cu->per_cu->lang () == language_go)
+  if (cu->lang () == language_go)
     fixup_go_packaging (cu);
 
   /* Now that we have processed all the DIEs in the CU, all the types
@@ -8469,7 +8470,7 @@ process_full_type_unit (dwarf2_cu *cu,
      physnames.  */
   compute_delayed_physnames (cu);
 
-  if (cu->per_cu->lang () == language_rust)
+  if (cu->lang () == language_rust)
     rust_union_quirks (cu);
 
   /* TUs share symbol tables.
@@ -8490,9 +8491,9 @@ process_full_type_unit (dwarf2_cu *cu,
 	     compilation is from a C file generated by language preprocessors,
 	     do not set the language if it was already deduced by
 	     start_subfile.  */
-	  if (!(cu->per_cu->lang () == language_c
+	  if (!(cu->lang () == language_c
 		&& cust->primary_filetab ()->language () != language_c))
-	    cust->primary_filetab ()->set_language (cu->per_cu->lang ());
+	    cust->primary_filetab ()->set_language (cu->lang ());
 	}
     }
   else
@@ -8532,6 +8533,7 @@ process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu)
 	= dwarf2_find_containing_comp_unit (sect_off, is_dwz,
 					    per_objfile->per_bfd);
 
+#if 0
       /* We're importing a C++ compilation unit with tag DW_TAG_compile_unit
 	 into another compilation unit, at root level.  Regard this as a hint,
 	 and ignore it.  */
@@ -8539,12 +8541,13 @@ process_imported_unit_die (struct die_info *die, struct dwarf2_cu *cu)
 	  && per_cu->unit_type () == DW_UT_compile
 	  && per_cu->lang () == language_cplus)
 	return;
+#endif
 
       /* If necessary, add it to the queue and load its DIEs.  */
       if (maybe_queue_comp_unit (cu, per_cu, per_objfile,
-				 cu->per_cu->lang ()))
+				 cu->lang ()))
 	load_full_comp_unit (per_cu, per_objfile, per_objfile->get_cu (per_cu),
-			     false, cu->per_cu->lang ());
+			     false, cu->lang ());
 
       cu->per_cu->imported_symtabs_push (per_cu);
     }
@@ -8602,7 +8605,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu)
       break;
     case DW_TAG_subprogram:
       /* Nested subprograms in Fortran get a prefix.  */
-      if (cu->per_cu->lang () == language_fortran
+      if (cu->lang () == language_fortran
 	  && die->parent != NULL
 	  && die->parent->tag == DW_TAG_subprogram)
 	cu->processing_has_namespace_info = true;
@@ -8646,7 +8649,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu)
       /* We only need to handle this case for Ada -- in other
 	 languages, it's normal for the compiler to emit a typedef
 	 instead.  */
-      if (cu->per_cu->lang () != language_ada)
+      if (cu->lang () != language_ada)
 	break;
       /* FALLTHROUGH */
     case DW_TAG_base_type:
@@ -8680,7 +8683,7 @@ process_die (struct die_info *die, struct dwarf2_cu *cu)
     case DW_TAG_imported_module:
       cu->processing_has_namespace_info = true;
       if (die->child != NULL && (die->tag == DW_TAG_imported_declaration
-				 || cu->per_cu->lang () != language_fortran))
+				 || cu->lang () != language_fortran))
 	complaint (_("Tag '%s' has unexpected children"),
 		   dwarf_tag_name (die->tag));
       read_import_statement (die, cu);
@@ -8792,7 +8795,7 @@ dw2_linkage_name (struct die_info *die, struct dwarf2_cu *cu)
 
   /* rustc emits invalid values for DW_AT_linkage_name.  Ignore these.
      See https://github.com/rust-lang/rust/issues/32925.  */
-  if (cu->per_cu->lang () == language_rust && linkage_name != NULL
+  if (cu->lang () == language_rust && linkage_name != NULL
       && strchr (linkage_name, '{') != NULL)
     linkage_name = NULL;
 
@@ -8824,7 +8827,7 @@ dwarf2_compute_name (const char *name,
   if (name == NULL)
     name = dwarf2_name (die, cu);
 
-  enum language lang = cu->per_cu->lang ();
+  enum language lang = cu->lang ();
 
   /* For Fortran GDB prefers DW_AT_*linkage_name for the physname if present
      but otherwise compute it by typename_concat inside GDB.
@@ -9073,7 +9076,7 @@ dwarf2_physname (const char *name, struct die_info *die, struct dwarf2_cu *cu)
   if (!die_needs_namespace (die, cu))
     return dwarf2_compute_name (name, die, cu, 1);
 
-  if (cu->per_cu->lang () != language_rust)
+  if (cu->lang () != language_rust)
     mangled = dw2_linkage_name (die, cu);
 
   /* DW_AT_linkage_name is missing in some cases - depend on what GDB
@@ -9230,7 +9233,7 @@ read_alias (struct die_info *die, struct dwarf2_cu *cu)
 static struct using_direct **
 using_directives (struct dwarf2_cu *cu)
 {
-  if (cu->per_cu->lang () == language_ada
+  if (cu->lang () == language_ada
       && cu->get_builder ()->outermost_context_p ())
     return cu->get_builder ()->get_global_using_directives ();
   else
@@ -9321,7 +9324,7 @@ read_import_statement (struct die_info *die, struct dwarf2_cu *cu)
   else if (strlen (imported_name_prefix) > 0)
     canonical_name = obconcat (&objfile->objfile_obstack,
 			       imported_name_prefix,
-			       (cu->per_cu->lang () == language_d
+			       (cu->lang () == language_d
 				? "."
 				: "::"),
 			       imported_name, (char *) NULL);
@@ -9329,7 +9332,7 @@ read_import_statement (struct die_info *die, struct dwarf2_cu *cu)
     canonical_name = imported_name;
 
   if (die->tag == DW_TAG_imported_module
-      && cu->per_cu->lang () == language_fortran)
+      && cu->lang () == language_fortran)
     for (child_die = die->child; child_die && child_die->tag;
 	 child_die = child_die->sibling)
       {
@@ -9555,7 +9558,7 @@ read_file_scope (struct die_info *die, struct dwarf2_cu *cu)
   struct die_info *child_die;
   CORE_ADDR baseaddr;
 
-  prepare_one_comp_unit (cu, die, cu->per_cu->lang ());
+  prepare_one_comp_unit (cu, die, cu->lang ());
   baseaddr = objfile->text_section_offset ();
 
   get_scope_pc_bounds (die, &lowpc, &highpc, cu);
@@ -11717,7 +11720,7 @@ queue_and_load_dwo_tu (void **slot, void *info)
 	 a real dependency of PER_CU on SIG_TYPE.  That is detected later
 	 while processing PER_CU.  */
       if (maybe_queue_comp_unit (NULL, sig_type, cu->per_objfile,
-				 cu->per_cu->lang ()))
+				 cu->lang ()))
 	load_full_type_unit (sig_type, cu->per_objfile);
       cu->per_cu->imported_symtabs_push (sig_type);
     }
@@ -12007,7 +12010,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
 
   if (dwarf2_flag_true_p (die, DW_AT_main_subprogram, cu))
     set_objfile_main_name (objfile, newobj->name->linkage_name (),
-			   cu->per_cu->lang ());
+			   cu->lang ());
 
   /* If there is a location expression for DW_AT_frame_base, record
      it.  */
@@ -12052,7 +12055,7 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
   /* If we have a DW_AT_specification, we might need to import using
      directives from the context of the specification DIE.  See the
      comment in determine_prefix.  */
-  if (cu->per_cu->lang () == language_cplus
+  if (cu->lang () == language_cplus
       && dwarf2_attr (die, DW_AT_specification, cu))
     {
       struct dwarf2_cu *spec_cu = cu;
@@ -12080,10 +12083,10 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
 				     cstk.static_link, lowpc, highpc);
 
   /* For C++, set the block's scope.  */
-  if ((cu->per_cu->lang () == language_cplus
-       || cu->per_cu->lang () == language_fortran
-       || cu->per_cu->lang () == language_d
-       || cu->per_cu->lang () == language_rust)
+  if ((cu->lang () == language_cplus
+       || cu->lang () == language_fortran
+       || cu->lang () == language_d
+       || cu->lang () == language_rust)
       && cu->processing_has_namespace_info)
     block_set_scope (block, determine_prefix (die, cu),
 		     &objfile->objfile_obstack);
@@ -12583,7 +12586,7 @@ read_variable (struct die_info *die, struct dwarf2_cu *cu)
 {
   struct rust_vtable_symbol *storage = NULL;
 
-  if (cu->per_cu->lang () == language_rust)
+  if (cu->lang () == language_rust)
     {
       struct type *containing_type = rust_containing_type (die, cu);
 
@@ -13097,7 +13100,7 @@ dwarf2_get_subprogram_pc_bounds (struct die_info *die,
 
   /* If the language does not allow nested subprograms (either inside
      subprograms or lexical blocks), we're done.  */
-  if (cu->per_cu->lang () != language_ada)
+  if (cu->lang () != language_ada)
     return;
 
   /* Check all the children of the given DIE.  If it contains nested
@@ -13902,7 +13905,7 @@ dwarf2_attach_fields_to_type (struct field_info *fip, struct type *type,
   type->set_fields
     ((struct field *) TYPE_ZALLOC (type, sizeof (struct field) * nfields));
 
-  if (fip->non_public_fields && cu->per_cu->lang () != language_ada)
+  if (fip->non_public_fields && cu->lang () != language_ada)
     {
       ALLOCATE_CPLUS_STRUCT_TYPE (type);
 
@@ -13921,7 +13924,7 @@ dwarf2_attach_fields_to_type (struct field_info *fip, struct type *type,
 
   /* If the type has baseclasses, allocate and clear a bit vector for
      TYPE_FIELD_VIRTUAL_BITS.  */
-  if (!fip->baseclasses.empty () && cu->per_cu->lang () != language_ada)
+  if (!fip->baseclasses.empty () && cu->lang () != language_ada)
     {
       int num_bytes = B_BYTES (fip->baseclasses.size ());
       unsigned char *pointer;
@@ -13947,12 +13950,12 @@ dwarf2_attach_fields_to_type (struct field_info *fip, struct type *type,
       switch (field.accessibility)
 	{
 	case DW_ACCESS_private:
-	  if (cu->per_cu->lang () != language_ada)
+	  if (cu->lang () != language_ada)
 	    SET_TYPE_FIELD_PRIVATE (type, i);
 	  break;
 
 	case DW_ACCESS_protected:
-	  if (cu->per_cu->lang () != language_ada)
+	  if (cu->lang () != language_ada)
 	    SET_TYPE_FIELD_PROTECTED (type, i);
 	  break;
 
@@ -13973,7 +13976,7 @@ dwarf2_attach_fields_to_type (struct field_info *fip, struct type *type,
 	    {
 	    case DW_VIRTUALITY_virtual:
 	    case DW_VIRTUALITY_pure_virtual:
-	      if (cu->per_cu->lang () == language_ada)
+	      if (cu->lang () == language_ada)
 		error (_("unexpected virtuality in component of Ada type"));
 	      SET_TYPE_FIELD_VIRTUAL (type, i);
 	      break;
@@ -14024,7 +14027,7 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
   const char *fieldname;
   struct type *this_type;
 
-  if (cu->per_cu->lang () == language_ada)
+  if (cu->lang () == language_ada)
     error (_("unexpected member function in Ada type"));
 
   /* Get name of member function.  */
@@ -14057,7 +14060,7 @@ dwarf2_add_member_fn (struct field_info *fip, struct die_info *die,
   fnp = &flp->fnfields.back ();
 
   /* Delay processing of the physname until later.  */
-  if (cu->per_cu->lang () == language_cplus)
+  if (cu->lang () == language_cplus)
     add_to_method_list (type, i, flp->fnfields.size () - 1, fieldname,
 			die, cu);
   else
@@ -14212,7 +14215,7 @@ static void
 dwarf2_attach_fn_fields_to_type (struct field_info *fip, struct type *type,
 				 struct dwarf2_cu *cu)
 {
-  if (cu->per_cu->lang () == language_ada)
+  if (cu->lang () == language_ada)
     error (_("unexpected member functions in Ada type"));
 
   ALLOCATE_CPLUS_STRUCT_TYPE (type);
@@ -14355,7 +14358,7 @@ static void
 quirk_ada_thick_pointer_struct (struct die_info *die, struct dwarf2_cu *cu,
 				struct type *type)
 {
-  gdb_assert (cu->per_cu->lang () == language_ada);
+  gdb_assert (cu->lang () == language_ada);
 
   /* Check for a structure with two children.  */
   if (type->code () != TYPE_CODE_STRUCT || type->num_fields () != 2)
@@ -14534,9 +14537,9 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
   name = dwarf2_name (die, cu);
   if (name != NULL)
     {
-      if (cu->per_cu->lang  () == language_cplus
-	  || cu->per_cu->lang () == language_d
-	  || cu->per_cu->lang () == language_rust)
+      if (cu->lang  () == language_cplus
+	  || cu->lang () == language_d
+	  || cu->lang () == language_rust)
 	{
 	  const char *full_name = dwarf2_full_name (name, die, cu);
 
@@ -14572,7 +14575,7 @@ read_structure_type (struct die_info *die, struct dwarf2_cu *cu)
       type->set_code (TYPE_CODE_STRUCT);
     }
 
-  if (cu->per_cu->lang () == language_cplus && die->tag == DW_TAG_class_type)
+  if (cu->lang () == language_cplus && die->tag == DW_TAG_class_type)
     type->set_is_declared_class (true);
 
   /* Store the calling convention in the type if it's available in
@@ -14784,7 +14787,7 @@ handle_struct_member_die (struct die_info *child_die, struct type *type,
       /* Rust doesn't have member functions in the C++ sense.
 	 However, it does emit ordinary functions as children
 	 of a struct DIE.  */
-      if (cu->per_cu->lang () == language_rust)
+      if (cu->lang () == language_rust)
 	read_func_scope (child_die, cu);
       else
 	{
@@ -14948,7 +14951,7 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
       /* Copy fi.nested_types_list linked list elements content into the
 	 allocated array TYPE_NESTED_TYPES_ARRAY (type).  */
       if (!fi.nested_types_list.empty ()
-	  && cu->per_cu->lang () != language_ada)
+	  && cu->lang () != language_ada)
 	{
 	  int count = fi.nested_types_list.size ();
 
@@ -14964,9 +14967,9 @@ process_structure_scope (struct die_info *die, struct dwarf2_cu *cu)
     }
 
   quirk_gcc_member_function_pointer (type, objfile);
-  if (cu->per_cu->lang () == language_rust && die->tag == DW_TAG_union_type)
+  if (cu->lang () == language_rust && die->tag == DW_TAG_union_type)
     cu->rust_unions.push_back (type);
-  else if (cu->per_cu->lang () == language_ada)
+  else if (cu->lang () == language_ada)
     quirk_ada_thick_pointer_struct (die, cu, type);
 
   /* NOTE: carlton/2004-03-16: GCC 3.4 (or at least one of its
@@ -15670,7 +15673,7 @@ read_array_type (struct die_info *die, struct dwarf2_cu *cu)
   maybe_set_alignment (cu, die, type);
 
   struct type *replacement_type = nullptr;
-  if (cu->per_cu->lang () == language_ada)
+  if (cu->lang () == language_ada)
     {
       replacement_type = quirk_ada_thick_pointer (die, cu, type);
       if (replacement_type != nullptr)
@@ -15707,7 +15710,7 @@ read_array_order (struct die_info *die, struct dwarf2_cu *cu)
      FIXME: dsl/2004-8-20: If G77 is ever fixed, this will also need
      version checking.  */
 
-  if (cu->per_cu->lang () == language_fortran
+  if (cu->lang () == language_fortran
       && cu->producer && strstr (cu->producer, "GNU F77"))
     {
       return DW_ORD_row_major;
@@ -16446,9 +16449,9 @@ prototyped_function_p (struct die_info *die, struct dwarf2_cu *cu)
      languages that allow unprototyped functions (Eg: Objective C).
      For all other languages, assume that functions are always
      prototyped.  */
-  if (cu->per_cu->lang () != language_c
-      && cu->per_cu->lang () != language_objc
-      && cu->per_cu->lang () != language_opencl)
+  if (cu->lang () != language_c
+      && cu->lang () != language_objc
+      && cu->lang () != language_opencl)
     return 1;
 
   /* RealView does not emit DW_AT_prototyped.  We can not distinguish
@@ -16573,7 +16576,7 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
 	      /* RealView does not mark THIS as const, which the testsuite
 		 expects.  GCC marks THIS as const in method definitions,
 		 but not in the class specifications (GCC PR 43053).  */
-	      if (cu->per_cu->lang () == language_cplus
+	      if (cu->lang () == language_cplus
 		  && !TYPE_CONST (arg_type)
 		  && TYPE_FIELD_ARTIFICIAL (ftype, iparams))
 		{
@@ -17009,7 +17012,7 @@ dwarf2_init_complex_target_type (struct dwarf2_cu *cu,
   /* Try to find a suitable floating point builtin type of size BITS.
      We're going to use the name of this type as the name for the complex
      target type that we are about to create.  */
-  switch (cu->per_cu->lang ())
+  switch (cu->lang ())
     {
     case language_fortran:
       switch (bits)
@@ -17099,7 +17102,7 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
     }
 
   if ((encoding == DW_ATE_signed_fixed || encoding == DW_ATE_unsigned_fixed)
-      && cu->per_cu->lang () == language_ada
+      && cu->lang () == language_ada
       && has_zero_over_zero_small_attribute (die, cu))
     {
       /* brobecker/2018-02-24: This is a fixed point type for which
@@ -17121,7 +17124,7 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
      than an "else if".  */
   const char *gnat_encoding_suffix = nullptr;
   if ((encoding == DW_ATE_signed || encoding == DW_ATE_unsigned)
-      && cu->per_cu->lang () == language_ada
+      && cu->lang () == language_ada
       && name != nullptr)
     {
       gnat_encoding_suffix = gnat_encoded_fixed_point_type_info (name);
@@ -17178,7 +17181,7 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
 	type = dwarf2_init_integer_type (cu, objfile, bits, 0, name);
 	break;
       case DW_ATE_unsigned:
-	if (cu->per_cu->lang () == language_fortran
+	if (cu->lang () == language_fortran
 	    && name
 	    && startswith (name, "character("))
 	  type = init_character_type (objfile, bits, 1, name);
@@ -17186,20 +17189,20 @@ read_base_type (struct die_info *die, struct dwarf2_cu *cu)
 	  type = dwarf2_init_integer_type (cu, objfile, bits, 1, name);
 	break;
       case DW_ATE_signed_char:
-	if (cu->per_cu->lang () == language_ada
-	    || cu->per_cu->lang () == language_m2
-	    || cu->per_cu->lang () == language_pascal
-	    || cu->per_cu->lang () == language_fortran)
+	if (cu->lang () == language_ada
+	    || cu->lang () == language_m2
+	    || cu->lang () == language_pascal
+	    || cu->lang () == language_fortran)
 	  type = init_character_type (objfile, bits, 0, name);
 	else
 	  type = dwarf2_init_integer_type (cu, objfile, bits, 0, name);
 	break;
       case DW_ATE_unsigned_char:
-	if (cu->per_cu->lang () == language_ada
-	    || cu->per_cu->lang () == language_m2
-	    || cu->per_cu->lang () == language_pascal
-	    || cu->per_cu->lang () == language_fortran
-	    || cu->per_cu->lang () == language_rust)
+	if (cu->lang () == language_ada
+	    || cu->lang () == language_m2
+	    || cu->lang () == language_pascal
+	    || cu->lang () == language_fortran
+	    || cu->lang () == language_rust)
 	  type = init_character_type (objfile, bits, 1, name);
 	else
 	  type = dwarf2_init_integer_type (cu, objfile, bits, 1, name);
@@ -17497,7 +17500,7 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
 
   /* Set LOW_DEFAULT_IS_VALID if current language and DWARF version allow
      omitting DW_AT_lower_bound.  */
-  switch (cu->per_cu->lang ())
+  switch (cu->lang ())
     {
     case language_c:
     case language_cplus:
@@ -17633,7 +17636,7 @@ read_subrange_type (struct die_info *die, struct dwarf2_cu *cu)
     range_type->bounds ()->flag_upper_bound_is_count = 1;
 
   /* Ada expects an empty array on no boundary attributes.  */
-  if (attr == NULL && cu->per_cu->lang () != language_ada)
+  if (attr == NULL && cu->lang () != language_ada)
     range_type->bounds ()->high.set_undefined ();
 
   name = dwarf2_name (die, cu);
@@ -17666,7 +17669,7 @@ read_unspecified_type (struct die_info *die, struct dwarf2_cu *cu)
      of the type is deferred to a different unit.  When encountering
      such a type, we treat it as a stub, and try to resolve it later on,
      when needed.  */
-  if (cu->per_cu->lang () == language_ada)
+  if (cu->lang () == language_ada)
     type->set_is_stub (true);
 
   return set_die_type (die, type, cu);
@@ -18347,7 +18350,7 @@ cooked_indexer::index_dies (cutu_reader *reader,
 	  else
 	    this_entry = m_index_storage->add (this_die, abbrev->tag, flags,
 					       name, this_parent_entry,
-					       m_per_cu);
+					       m_per_cu, m_language);
 	}
 
       if (linkage_name != nullptr)
@@ -18364,7 +18367,7 @@ cooked_indexer::index_dies (cutu_reader *reader,
 	          && abbrev->tag != DW_TAG_entry_point))
 	    flags = flags | IS_LINKAGE;
 	  m_index_storage->add (this_die, abbrev->tag, flags,
-				linkage_name, nullptr, m_per_cu);
+				linkage_name, nullptr, m_per_cu, m_language);
 	}
 
       if (abbrev->has_children)
@@ -18452,7 +18455,7 @@ cooked_indexer::make_index (cutu_reader *reader)
       cooked_index_entry *parent
 	= (cooked_index_entry *) m_die_range_map.find (key);
       m_index_storage->add (entry.die_offset, entry.tag, entry.flags,
-			    entry.name, parent, m_per_cu);
+			    entry.name, parent, m_per_cu, m_language);
     }
 }
 
@@ -18566,7 +18569,7 @@ cooked_index_functions::expand_matching_symbols
 	continue;
 
       if (name_match (entry->canonical, lookup_name, nullptr))
-	dw2_instantiate_symtab (entry->per_cu, per_objfile, false);
+	dw2_instantiate_symtab (entry->m_per_cu_lang->per_cu, per_objfile, false);
     }
 }
 
@@ -18632,12 +18635,12 @@ cooked_index_functions::expand_symtabs_matching
 							  completing))
 	{
 	  /* No need to consider symbols from expanded CUs.  */
-	  if (per_objfile->symtab_set_p (entry->per_cu))
+	  if (per_objfile->symtab_set_p (entry->m_per_cu_lang->per_cu))
 	    continue;
 
 	  /* If file-matching was done, we don't need to consider
 	     symbols from unmarked CUs.  */
-	  if (file_matcher != nullptr && !entry->per_cu->mark)
+	  if (file_matcher != nullptr && !entry->m_per_cu_lang->per_cu->mark)
 	    continue;
 
 	  /* See if the symbol matches the type filter.  */
@@ -18690,7 +18693,8 @@ cooked_index_functions::expand_symtabs_matching
 		continue;
 	    }
 
-	  if (!dw2_expand_symtabs_matching_one (entry->per_cu, per_objfile,
+	  if (!dw2_expand_symtabs_matching_one (entry->m_per_cu_lang->per_cu,
+						per_objfile,
 						file_matcher,
 						expansion_notify))
 	    return false;
@@ -20685,16 +20689,16 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
       OBJSTAT (objfile, n_syms++);
 
       /* Cache this symbol's name and the name's demangled form (if any).  */
-      sym->set_language (cu->per_cu->lang (), &objfile->objfile_obstack);
+      sym->set_language (cu->lang (), &objfile->objfile_obstack);
       /* Fortran does not have mangling standard and the mangling does differ
 	 between gfortran, iFort etc.  */
       const char *physname
-	= (cu->per_cu->lang () == language_fortran
+	= (cu->lang () == language_fortran
 	   ? dwarf2_full_name (name, die, cu)
 	   : dwarf2_physname (name, die, cu));
       const char *linkagename = dw2_linkage_name (die, cu);
 
-      if (linkagename == nullptr || cu->per_cu->lang () == language_ada)
+      if (linkagename == nullptr || cu->lang () == language_ada)
 	sym->set_linkage_name (physname);
       else
 	{
@@ -20766,8 +20770,8 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 	  sym->set_aclass_index (LOC_BLOCK);
 	  attr2 = dwarf2_attr (die, DW_AT_external, cu);
 	  if ((attr2 != nullptr && attr2->as_boolean ())
-	      || cu->per_cu->lang () == language_ada
-	      || cu->per_cu->lang () == language_fortran)
+	      || cu->lang () == language_ada
+	      || cu->lang () == language_fortran)
 	    {
 	      /* Subprograms marked external are stored as a global symbol.
 		 Ada and Fortran subprograms, whether marked external or
@@ -20832,7 +20836,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 
 	      /* Fortran explicitly imports any global symbols to the local
 		 scope by DW_TAG_common_block.  */
-	      if (cu->per_cu->lang () == language_fortran && die->parent
+	      if (cu->lang () == language_fortran && die->parent
 		  && die->parent->tag == DW_TAG_common_block)
 		attr2 = NULL;
 
@@ -20886,7 +20890,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 
 	      /* Fortran explicitly imports any global symbols to the local
 		 scope by DW_TAG_common_block.  */
-	      if (cu->per_cu->lang () == language_fortran && die->parent
+	      if (cu->lang () == language_fortran && die->parent
 		  && die->parent->tag == DW_TAG_common_block)
 		{
 		  /* SYMBOL_CLASS doesn't matter here because
@@ -20980,16 +20984,16 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 		buildsym_compunit *builder = cu->get_builder ();
 		list_to_add
 		  = (cu->list_in_scope == builder->get_file_symbols ()
-		     && cu->per_cu->lang () == language_cplus
+		     && cu->lang () == language_cplus
 		     ? builder->get_global_symbols ()
 		     : cu->list_in_scope);
 
 		/* The semantics of C++ state that "struct foo {
 		   ... }" also defines a typedef for "foo".  */
-		if (cu->per_cu->lang () == language_cplus
-		    || cu->per_cu->lang () == language_ada
-		    || cu->per_cu->lang () == language_d
-		    || cu->per_cu->lang () == language_rust)
+		if (cu->lang () == language_cplus
+		    || cu->lang () == language_ada
+		    || cu->lang () == language_d
+		    || cu->lang () == language_rust)
 		  {
 		    /* The symbol's name is already allocated along
 		       with this objfile, so we don't need to
@@ -21025,7 +21029,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 
 	    list_to_add
 	      = (cu->list_in_scope == cu->get_builder ()->get_file_symbols ()
-		 && cu->per_cu->lang () == language_cplus
+		 && cu->lang () == language_cplus
 		 ? cu->get_builder ()->get_global_symbols ()
 		 : cu->list_in_scope);
 	  }
@@ -21068,7 +21072,7 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
       /* For the benefit of old versions of GCC, check for anonymous
 	 namespaces based on the demangled name.  */
       if (!cu->processing_has_namespace_info
-	  && cu->per_cu->lang () == language_cplus)
+	  && cu->lang () == language_cplus)
 	cp_scan_for_anonymous_namespaces (cu->get_builder (), sym, objfile);
     }
   return (sym);
@@ -21280,7 +21284,7 @@ need_gnat_info (struct dwarf2_cu *cu)
 {
   /* Assume that the Ada compiler was GNAT, which always produces
      the auxiliary information.  */
-  return (cu->per_cu->lang () == language_ada);
+  return (cu->lang () == language_ada);
 }
 
 /* Return the auxiliary type of the die in question using its
@@ -21657,10 +21661,10 @@ determine_prefix (struct die_info *die, struct dwarf2_cu *cu)
   struct type *parent_type;
   const char *retval;
 
-  if (cu->per_cu->lang () != language_cplus
-      && cu->per_cu->lang () != language_fortran
-      && cu->per_cu->lang () != language_d
-      && cu->per_cu->lang () != language_rust)
+  if (cu->lang () != language_cplus
+      && cu->lang () != language_fortran
+      && cu->lang () != language_d
+      && cu->lang () != language_rust)
     return "";
 
   retval = anonymous_struct_prefix (die, cu);
@@ -21748,7 +21752,7 @@ determine_prefix (struct die_info *die, struct dwarf2_cu *cu)
 	/* GCC 4.0 and 4.1 had a bug (PR c++/28460) where they generated bogus
 	   DW_TAG_namespace DIEs with a name of "::" for the global namespace.
 	   Work around this problem here.  */
-	if (cu->per_cu->lang () == language_cplus
+	if (cu->lang () == language_cplus
 	    && strcmp (parent_type->name (), "::") == 0)
 	  return "";
 	/* We give a name to even anonymous namespaces.  */
@@ -21769,7 +21773,7 @@ determine_prefix (struct die_info *die, struct dwarf2_cu *cu)
       case DW_TAG_compile_unit:
       case DW_TAG_partial_unit:
 	/* gcc-4.5 -gdwarf-4 can drop the enclosing namespace.  Cope.  */
-	if (cu->per_cu->lang () == language_cplus
+	if (cu->lang () == language_cplus
 	    && !per_objfile->per_bfd->types.empty ()
 	    && die->child != NULL
 	    && (die->tag == DW_TAG_class_type
@@ -21784,7 +21788,7 @@ determine_prefix (struct die_info *die, struct dwarf2_cu *cu)
       case DW_TAG_subprogram:
 	/* Nested subroutines in Fortran get a prefix with the name
 	   of the parent's subroutine.  */
-	if (cu->per_cu->lang () == language_fortran)
+	if (cu->lang () == language_fortran)
 	  {
 	    if ((die->tag ==  DW_TAG_subprogram)
 		&& (dwarf2_name (parent, cu) != NULL))
@@ -21823,7 +21827,7 @@ typename_concat (struct obstack *obs, const char *prefix, const char *suffix,
   if (suffix == NULL || suffix[0] == '\0'
       || prefix == NULL || prefix[0] == '\0')
     sep = "";
-  else if (cu->per_cu->lang () == language_d)
+  else if (cu->lang () == language_d)
     {
       /* For D, the 'main' function could be defined in any module, but it
 	 should never be prefixed.  */
@@ -21835,7 +21839,7 @@ typename_concat (struct obstack *obs, const char *prefix, const char *suffix,
       else
 	sep = ".";
     }
-  else if (cu->per_cu->lang () == language_fortran && physname)
+  else if (cu->lang () == language_fortran && physname)
     {
       /* This is gfortran specific mangling.  Normally DW_AT_linkage_name or
 	 DW_AT_MIPS_linkage_name is preferred and used instead.  */
@@ -21876,7 +21880,7 @@ static const char *
 dwarf2_canonicalize_name (const char *name, struct dwarf2_cu *cu,
 			  struct objfile *objfile)
 {
-  if (name && cu->per_cu->lang () == language_cplus)
+  if (name && cu->lang () == language_cplus)
     {
       gdb::unique_xmalloc_ptr<char> canon_name
 	= cp_canonicalize_string (name);
@@ -22253,10 +22257,10 @@ follow_die_offset (sect_offset sect_off, int offset_in_dwz,
 	 Even if maybe_queue_comp_unit doesn't require us to load the CU's DIEs,
 	 it doesn't mean they are currently loaded.  Since we require them
 	 to be loaded, we must check for ourselves.  */
-      if (maybe_queue_comp_unit (cu, per_cu, per_objfile, cu->per_cu->lang ())
+      if (maybe_queue_comp_unit (cu, per_cu, per_objfile, cu->lang ())
 	  || per_objfile->get_cu (per_cu) == nullptr)
 	load_full_comp_unit (per_cu, per_objfile, per_objfile->get_cu (per_cu),
-			     false, cu->per_cu->lang ());
+			     false, cu->lang ());
 
       target_cu = per_objfile->get_cu (per_cu);
       gdb_assert (target_cu != nullptr);
@@ -23585,7 +23589,6 @@ prepare_one_comp_unit (struct dwarf2_cu *cu, struct die_info *comp_unit_die,
   else
     lang = pretend_language;
 
-  cu->per_cu->set_lang (lang);
   cu->language_defn = language_def (lang);
 }
 
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index 22b4dc2f867..b73ac0f8661 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -109,7 +109,6 @@ struct dwarf2_per_cu_data
       mark (false),
       files_read (false),
       m_unit_type {},
-      m_lang (language_unknown),
       scanned (false)
   {
   }
@@ -182,11 +181,6 @@ struct dwarf2_per_cu_data
     ENUM_BITFIELD (dwarf_unit_type) m_unit_type : 8;
   };
 
-  struct { 
-    /* The language of this CU.  */
-    ENUM_BITFIELD (language) m_lang : LANGUAGE_BITS;
-  };
-
 public:
   /* True if this CU has been scanned by the indexer; false if
      not.  */
@@ -329,21 +323,6 @@ struct dwarf2_per_cu_data
       gdb_assert (m_unit_type == unit_type);
   }
 
-  enum language lang () const
-  {
-    gdb_assert (m_lang != language_unknown);
-    return m_lang;
-  }
-
-  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;
-  }
-
   /* Free any cached file names.  */
   void free_cached_file_names ();
 };

Re: [PATCH 5/5] [gdb/symtab] Fix data race on per_cu->lang

[Tom de Vries]

On 7/5/22 17:19, Tom de Vries wrote:
> On 7/4/22 20:30, Tom Tromey wrote:
>>>>>>> "Tom" == Tom de Vries <tdevries@suse.de> writes:
>> Tom> Fix this by guarding the write with a lock.
>>
>> I would rather we avoid locks.  Ideally the existing exclusion mechanism
>> should be made to work, but if that fails, perhaps we can use another
>> atomic.
> 
> I gave a different approach a try: eliminating the field.
> 
> That seems to work reasonably well, apart from one specific usage that I 
> disabled (mentioned in commit message).
> 

Hmm, that same piece of code is where I run into 
https://sourceware.org/bugzilla/show_bug.cgi?id=29321 .  And, it's a 
piece of code from a commit where I added per_cu->lang/unit_type.  So, 
this starts to look like the actual cause of problems.

Thanks,
- Tom

> I'm not sure this is a good idea though, but I thought I could at least 
> post it.  Currently testing.
> 
> Thanks,
> - Tom
> 

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

[Pedro Alves]

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, as the following field will end up
moved to the next byte, so if LANGUAGE_BITS is 5, we'll end up with 3 bits gap.

Actually, it's worse than that -- it will align m_lang to ENUM_BITFIELD(language)'s
natural alignment, so it can introduce byte padding before and after too.  :-/  :-(

We can see it with "ptype /o" after applying your patch using the struct{}
trick.  Note the "3-byte padding" below:

 ...
 /*     13: 5   |       1 */    bool m_header_read_in : 1;
 /* XXX  2-bit hole       */
 /*     14      |       1 */    struct {
 /*     14: 0   |       1 */        bool addresses_seen : 1;
 /* XXX  7-bit padding    */
 
                                    /* total size (bytes):    1 */
                                };
 /*     15: 0   |       4 */    unsigned int mark : 1;
 /*     15: 1   |       1 */    bool files_read : 1;
 /* XXX  6-bit hole       */
 /*     16      |       4 */    struct {
 /*     16: 0   |       4 */        dwarf_unit_type unit_type : 8;
 /* XXX  3-byte padding   */                                               <<<<<< 3 bytes
 
                                    /* total size (bytes):    4 */
                                };
 /*     20      |       4 */    struct {
 /*     20: 0   |       4 */        language lang : 5;
 /* XXX  3-bit padding    */
 /* XXX  3-byte padding   */                                               <<<<<< 3 bytes

                                    /* total size (bytes):    4 */
                                };
 ...



So, maybe we really want something else...  How about this alternative patch below?

I wrote the new struct packed template using the array for storage before I
wrote the alternative to use __attribute__((packed)), so I left the array
version in there, as a pure standards-conforming implementation.  We can just
remove that array implementation completely, and use the ATTRIBUTE_PACKED macro
if you don't think it's worth it.  All compilers worth their salt support attribute
packed, or something like it, I believe.  The resulting gdbsupport/pack.h would
be quite smaller.

I tested this on x86-64 Ubuntu 20.04, both attribute packed no attribute
versions, saw no regressions.  Also smoke-tested with Clang (which uses the
attribute packed implementation too, as it defines __GNUC__).

I have not actually tested this with -fsanitize=thread, though.  Would you
be up for testing that, Tom, if this approach looks reasonable?

Note that with this, struct dwarf2_per_cu_data is still 120 bytes, there's no growth:

(top-gdb) ptype /o dwarf2_per_cu_data
/* offset      |    size */  type = struct dwarf2_per_cu_data {
                             public:
/*      0      |       8 */    sect_offset sect_off;
/*      8      |       4 */    unsigned int length;
                             private:
/*     12      |       1 */    unsigned char m_dwarf_version;
                             public:
/*     13: 0   |       4 */    unsigned int queued : 1;
/*     13: 1   |       4 */    unsigned int is_debug_types : 1;
/*     13: 2   |       4 */    unsigned int is_dwz : 1;
/*     13: 3   |       4 */    unsigned int reading_dwo_directly : 1;
/*     13: 4   |       4 */    unsigned int tu_read : 1;
/*     13: 5   |       1 */    bool m_header_read_in : 1;
/*     13: 6   |       4 */    unsigned int mark : 1;
/*     13: 7   |       1 */    bool files_read : 1;
/*     14      |       1 */    struct packed<bool, 1> [with T = bool] {
                                 private:
/*     14      |       1 */        struct {
/*     14: 0   |       1 */            T m_val : 8;

                                       /* total size (bytes):    1 */
                                   };

                                   /* total size (bytes):    1 */
                               } addresses_seen;
                             private:
/*     15      |       1 */    struct packed<dwarf_unit_type, 1> [with T = dwarf_unit_type] {
                                 private:
/*     15      |       1 */        struct {
/*     15: 0   |       4 */            T m_val : 8;

                                       /* total size (bytes):    1 */
                                   };

                                   /* total size (bytes):    1 */
                               } m_unit_type;
/*     16      |       1 */    struct packed<language, 1> [with T = language] {
                                 private:
/*     16      |       1 */        struct {
/*     16: 0   |       4 */            T m_val : 8;

                                       /* total size (bytes):    1 */
                                   };

                                   /* total size (bytes):    1 */
                               } m_lang;
                             public:
/*     17      |       1 */    struct std::atomic<bool> [with _Tp = bool] {
                                 private:
/*     17      |       1 */        struct std::__atomic_base<_Tp> [with _IntTp = _Tp] {
                                     private:
                                       static const int _S_alignment;
/*     17      |       1 */            __int_type _M_i;

                                       /* total size (bytes):    1 */
                                   } _M_base;

                                   /* total size (bytes):    1 */
                               } scanned;
/* XXX  2-byte hole      */
/*     20      |       4 */    unsigned int index;
/*     24      |       8 */    dwarf2_section_info *section;
/*     32      |       8 */    dwarf2_per_bfd *per_bfd;
/*     40      |      56 */    struct comp_unit_head {
/*     40      |       4 */        unsigned int length;
/*     44      |       1 */        unsigned char version;
/*     45      |       1 */        unsigned char addr_size;
/*     46      |       1 */        unsigned char signed_addr_p;
/* XXX  1-byte hole      */
/*     48      |       8 */        sect_offset abbrev_sect_off;
/*     56      |       4 */        unsigned int offset_size;
/*     60      |       4 */        unsigned int initial_length_size;
/*     64      |       4 */        dwarf_unit_type unit_type;
/*     68      |       4 */        cu_offset first_die_cu_offset;
/*     72      |       8 */        sect_offset sect_off;
/*     80      |       4 */        cu_offset type_cu_offset_in_tu;
/* XXX  4-byte hole      */
/*     88      |       8 */        ULONGEST signature;

                                   /* total size (bytes):   56 */
                               } m_header;
/*     96      |       8 */    std::unique_ptr<file_and_directory> fnd;
/*    104      |       8 */    During symbol reading: Child DIE 0x1d6f34b and its abstract origin 0x1d6f380 have different parents
quick_file_names *file_names;
/*    112      |       8 */    std::vector<dwarf2_per_cu_data*> *imported_symtabs;

                               /* total size (bytes):  120 */
                             }
(top-gdb) 


Patch follows:

From c65b4a71a08835dd46c16443f84793e982efdc8a Mon Sep 17 00:00:00 2001
From: Pedro Alves <pedro@palves.net>
Date: Wed, 6 Jul 2022 14:46:49 +0100
Subject: [PATCH] Introduce struct packed template, fix -fsanitize=thread for
 per_cu fields

Change-Id: Ifa94f0a2cebfae5e8f6ddc73265f05e7fd9e1532
---
 gdb/defs.h          |  3 ++
 gdb/dwarf2/read.h   | 20 +++++-----
 gdbsupport/packed.h | 92 +++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 106 insertions(+), 9 deletions(-)
 create mode 100644 gdbsupport/packed.h

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
diff --git a/gdbsupport/packed.h b/gdbsupport/packed.h
new file mode 100644
index 00000000000..a41c408a44d
--- /dev/null
+++ b/gdbsupport/packed.h
@@ -0,0 +1,92 @@
+/* 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.  */
+
+/* Use __attribute__((packed)) when possible to make it more
+   convenient to print the field when debugging GDB.  */
+#ifdef __GNUC__
+# define PACKED_USE_ATTRIBUTE_PACKED 1
+#else
+# define PACKED_USE_ATTRIBUTE_PACKED 0
+#endif
+
+#if !PACKED_USE_ATTRIBUTE_PACKED
+
+template<size_t bytes> struct bytes_to_type;
+
+template<> struct bytes_to_type<1>
+{
+  typedef uint8_t type;
+};
+
+template<> struct bytes_to_type<2>
+{
+  typedef uint16_t type;
+};
+
+template<> struct bytes_to_type<4>
+{
+  typedef uint32_t type;
+};
+#endif
+
+template<typename T, size_t Bytes = sizeof (T)>
+struct packed
+{
+public:
+  packed (T val)
+  {
+#if PACKED_USE_ATTRIBUTE_PACKED
+    m_val = val;
+#else
+    typename bytes_to_type<Bytes>::type tmp;
+    tmp = val;
+    memcpy (m_val, &tmp, Bytes);
+#endif
+  }
+
+  operator T () const
+  {
+#if PACKED_USE_ATTRIBUTE_PACKED
+    return (T) m_val;
+#else
+    typename bytes_to_type<Bytes>::type tmp;
+    memcpy (&tmp, m_val, Bytes);
+    return (T) tmp;
+#endif
+  }
+
+private:
+#if PACKED_USE_ATTRIBUTE_PACKED
+  T m_val : Bytes * 8  __attribute__ ((packed));
+#else
+  unsigned char m_val[Bytes];
+  static_assert (sizeof (typename bytes_to_type<Bytes>::type) == sizeof (m_val),
+		 "sizes must match");
+#endif
+};
+
+#endif

base-commit: 8bd915b770e12eff67f6218f2d727069d04d5752
-- 
2.36.0

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

[Tom de Vries]

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

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).

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.

> as the following field will end up
> moved to the next byte, so if LANGUAGE_BITS is 5, we'll end up with 3 bits gap.
> 
> Actually, it's worse than that -- it will align m_lang to ENUM_BITFIELD(language)'s
> natural alignment, so it can introduce byte padding before and after too.  :-/  :-(
> 
> We can see it with "ptype /o" after applying your patch using the struct{}
> trick.  Note the "3-byte padding" below:
> 
>   ...
>   /*     13: 5   |       1 */    bool m_header_read_in : 1;
>   /* XXX  2-bit hole       */
>   /*     14      |       1 */    struct {
>   /*     14: 0   |       1 */        bool addresses_seen : 1;
>   /* XXX  7-bit padding    */
>   
>                                      /* total size (bytes):    1 */
>                                  };
>   /*     15: 0   |       4 */    unsigned int mark : 1;
>   /*     15: 1   |       1 */    bool files_read : 1;
>   /* XXX  6-bit hole       */
>   /*     16      |       4 */    struct {
>   /*     16: 0   |       4 */        dwarf_unit_type unit_type : 8;
>   /* XXX  3-byte padding   */                                               <<<<<< 3 bytes
>   
>                                      /* total size (bytes):    4 */
>                                  };
>   /*     20      |       4 */    struct {
>   /*     20: 0   |       4 */        language lang : 5;
>   /* XXX  3-bit padding    */
>   /* XXX  3-byte padding   */                                               <<<<<< 3 bytes
> 
>                                      /* total size (bytes):    4 */
>                                  };
>   ...
> 
> 
> 
> So, maybe we really want something else...  How about this alternative patch below?
> 
> I wrote the new struct packed template using the array for storage before I
> wrote the alternative to use __attribute__((packed)), so I left the array
> version in there, as a pure standards-conforming implementation.  We can just
> remove that array implementation completely, and use the ATTRIBUTE_PACKED macro
> if you don't think it's worth it.  All compilers worth their salt support attribute
> packed, or something like it, I believe.  The resulting gdbsupport/pack.h would
> be quite smaller.
> 
> I tested this on x86-64 Ubuntu 20.04, both attribute packed no attribute
> versions, saw no regressions.  Also smoke-tested with Clang (which uses the
> attribute packed implementation too, as it defines __GNUC__).
> 
> 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:
...
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.

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.

Thanks,
- Tom


[-- Attachment #2: 0002-fix.patch --]
[-- Type: text/x-patch, Size: 2202 bytes --]

fix

---
 gdb/defs.h        |  3 +++
 gdb/dwarf2/read.h | 22 ++++++++++++++--------
 2 files changed, 17 insertions(+), 8 deletions(-)

diff --git a/gdb/defs.h b/gdb/defs.h
index 19f379d6588..c129bf633a1 100644
--- a/gdb/defs.h
+++ b/gdb/defs.h
@@ -235,6 +235,9 @@ 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)
 
+#define LANGUAGE_CONTAINER unsigned char
+gdb_static_assert (sizeof (LANGUAGE_CONTAINER) >= LANGUAGE_BYTES);
+
 enum precision_type
   {
     single_precision,
diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index f98d8b27649..f4362fe3ede 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_CONTAINER> m_lang;
 
 public:
   /* True if this CU has been scanned by the indexer; false if
@@ -326,17 +327,22 @@ struct dwarf2_per_cu_data
 
   enum language lang () const
   {
-    gdb_assert (m_lang != language_unknown);
-    return m_lang;
+    LANGUAGE_CONTAINER lc = m_lang;
+    enum language l = (enum language)lc;
+    gdb_assert (l != language_unknown);
+    return l;
   }
 
   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;
+    LANGUAGE_CONTAINER lc = (LANGUAGE_CONTAINER)lang;
+    LANGUAGE_CONTAINER nope = (LANGUAGE_CONTAINER)language_unknown;
+    if (m_lang.compare_exchange_strong (nope, lc))
+      return;
+    nope = lang;
+    if (m_lang.compare_exchange_strong (nope, lc))
+      return;
+    gdb_assert_not_reached ();
   }
 
   /* Free any cached file names.  */

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

[Pedro Alves]

[-- 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

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

[Tom de Vries]

On 7/7/22 17:26, Pedro Alves wrote:
> 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.  :-)
> 

Heh :)

>> 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.
> 

Sure.

> 
>>> 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.
> 

Ack.

>>
>> 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.
> 

I've rebased #1 and #2 on master, and then applied my current WIP set, 
copying the style that I found in #3.

I'm currently testing it, I've pushed to 
https://github.com/vries/gdb/commits/sanitize-thread-7 if you're interested.

#1 and #2 LGTM.

Thanks a lot for all the help :)

- Tom

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

[Tom de Vries]

On 7/8/22 16:54, Tom de Vries wrote:
> On 7/7/22 17:26, Pedro Alves wrote:
>> 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.  :-)
>>
> 
> Heh :)
> 
>>> 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.
>>
> 
> Sure.
> 
>>
>>>> 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.
>>
> 
> Ack.
> 
>>>
>>> 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.
>>
> 
> I've rebased #1 and #2 on master, and then applied my current WIP set, 
> copying the style that I found in #3.
> 
> I'm currently testing it, I've pushed to 
> https://github.com/vries/gdb/commits/sanitize-thread-7 if you're 
> interested.
> 
> #1 and #2 LGTM.
> 

So, pushed.

Thanks,
- Tom

> Thanks a lot for all the help :)
> 
> - Tom