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