public inbox for gdb-cvs@sourceware.org
help / color / mirror / Atom feed
* [binutils-gdb] [gdb/symtab] Fix data race on per_cu->dwarf_version
@ 2022-07-02 11:03 Tom de Vries
  0 siblings, 0 replies; only message in thread
From: Tom de Vries @ 2022-07-02 11:03 UTC (permalink / raw)
  To: gdb-cvs

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=33fd0a33639897e1c3403389de882cadd344a494

commit 33fd0a33639897e1c3403389de882cadd344a494
Author: Tom de Vries <tdevries@suse.de>
Date:   Sat Jul 2 13:03:34 2022 +0200

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

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 =
 	    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.  */


^ permalink raw reply	[flat|nested] only message in thread

only message in thread, other threads:[~2022-07-02 11:03 UTC | newest]

Thread overview: (only message) (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2022-07-02 11:03 [binutils-gdb] [gdb/symtab] Fix data race on per_cu->dwarf_version Tom de Vries

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).