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

[Pedro Alves <pedro@palves.net>]

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.