From 33fd0a33639897e1c3403389de882cadd344a494 Mon Sep 17 00:00:00 2001 From: Tom de Vries Date: Sat, 2 Jul 2022 13:03:34 +0200 Subject: [PATCH] [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 d5088395fb1f..410cc909c260 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 51e02dfc4572..b7a03933aa52 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. */