From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: by sourceware.org (Postfix, from userid 2205) id DC6FF3858C2F; Sat, 2 Jul 2022 11:03:37 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org DC6FF3858C2F Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: quoted-printable From: Tom de Vries To: gdb-cvs@sourceware.org Subject: [binutils-gdb] [gdb/symtab] Fix data race on per_cu->dwarf_version X-Act-Checkin: binutils-gdb X-Git-Author: Tom de Vries X-Git-Refname: refs/heads/master X-Git-Oldrev: 47226049bb7cdbc93543db13e6305c0091f4f642 X-Git-Newrev: 33fd0a33639897e1c3403389de882cadd344a494 Message-Id: <20220702110337.DC6FF3858C2F@sourceware.org> Date: Sat, 2 Jul 2022 11:03:37 +0000 (GMT) X-BeenThere: gdb-cvs@sourceware.org X-Mailman-Version: 2.1.29 Precedence: list List-Id: Gdb-cvs mailing list List-Unsubscribe: , List-Archive: List-Help: List-Subscribe: , X-List-Received-Date: Sat, 02 Jul 2022 11:03:38 -0000 https://sourceware.org/git/gitweb.cgi?p=3Dbinutils-gdb.git;h=3D33fd0a336398= 97e1c3403389de882cadd344a494 commit 33fd0a33639897e1c3403389de882cadd344a494 Author: Tom de Vries Date: Sat Jul 2 13:03:34 2022 +0200 [gdb/symtab] Fix data race on per_cu->dwarf_version =20 When building gdb with -fsanitize=3Dthread and gcc 12, and running test= -case gdb.dwarf2/dwz.exp, we run into a data race between thread T2 and the m= ain 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:6= 252 \ (gdb+0x82f3b3)^M ... which is here: ... this_cu->dwarf_version =3D cu->header.version; ... =20 Both writes are called from the parallel for in dwarf2_build_psymtabs_h= ard, 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_objfil= e*, \ 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+0x85d= cdb)^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 ... =20 Fix this by setting the field earlier, in read_comp_units_from_section. =20 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, t= arget board cc-with-gdb-index). =20 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. =20 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 consist= enly, and make the field private in order to enforce access through the member functions, and rename it to m_dwarf_version. =20 While we're at it, make sure that the version is set before read, to av= oid say returning true for "per_cu.version () < 5" if "per_cu.version () = =3D=3D 0". =20 Tested on x86_64-linux. Diff: --- 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 =3D this_cu->sect_off + to_underlying (sig_type->type_offset_in_tu); =20 - this_cu->dwarf_version =3D 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 =3D cu->header.get_length (); else gdb_assert (this_cu->length =3D=3D cu->header.get_length ()); - this_cu->dwarf_version =3D cu->header.version; + this_cu->set_version (cu->header.version); } } =20 @@ -7206,6 +7206,10 @@ read_comp_units_from_section (dwarf2_per_objfile *pe= r_objfile, this_cu->length =3D cu_header.length + cu_header.initial_length_size; this_cu->is_dwz =3D is_dwz; this_cu->section =3D 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); =20 info_ptr =3D 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.in= fo, dwo_file->cus); =20 - 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 =3D 0; =20 +private: /* DWARF standard version this data has been read from (such as 4 or 5).= */ - unsigned char dwarf_version =3D 0; + unsigned char m_dwarf_version =3D 0; =20 +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 !=3D 0); + return m_dwarf_version; + } + + void set_version (short version) + { + if (m_dwarf_version =3D=3D 0) + /* Set if not set already. */ + m_dwarf_version =3D version; + else + /* If already set, verify that it's the same value. */ + gdb_assert (m_dwarf_version =3D=3D version); } =20 /* Free any cached file names. */