[Bug symtab/30672] New: [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[Bug symtab/30672] New: [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30672

            Bug ID: 30672
           Summary: [gdb/symtab] data race in
                    cooked_index_shard::do_finalize() /
                    cooked_index_entry::write_scope
           Product: gdb
           Version: HEAD
            Status: NEW
          Severity: normal
          Priority: P2
         Component: symtab
          Assignee: unassigned at sourceware dot org
          Reporter: vries at gcc dot gnu.org
  Target Milestone: ---

Created attachment 15002
  --> https://sourceware.org/bugzilla/attachment.cgi?id=15002&action=edit
gdb.log

I build gdb with -O2 -fsanitize=thread and gcc 13.1.1, at commit 8a9da63e407
("gdb: two changes to linux_nat_debug_printf calls in linux-nat.c"), and ran
into:
...
(gdb) file
/data/vries/gdb/tw/build/gdb/testsuite/outputs/gdb.rust/dwindex/dwindex^M
Reading symbols from
/data/vries/gdb/tw/build/gdb/testsuite/outputs/gdb.rust/dwindex/dwindex...^M
warning: ==================^M
^[[1m^[[31mWARNING: ThreadSanitizer: data race (pid=16737)^M
^[[1m^[[0m^[[1m^[[34m  Write of size 8 at 0x7b80000b0018 by thread T2:^M
^[[1m^[[0m    #0 cooked_index_shard::do_finalize()
/data/vries/gdb/src/gdb/dwarf2/cooked-index.c:391 (gdb+0x714ad2) (BuildId:
f6fc8a5c2c3b42f3e3a24e54b8b2df127faee329)^M
  ...
^[[1m^[[34m  Previous read of size 8 at 0x7b80000b0018 by main thread:^M
^[[1m^[[0m    #0 cooked_index_entry::write_scope(obstack*, char const*, bool)
const /data/vries/gdb/src/gdb/dwarf2/cooked-index.c:224 (gdb+0x71290d)
(BuildId: f6fc8a5c2c3b42f3e3a24e54b8b2df127faee329)^M
...

391:
...
        entry->canonical = entry->name;
...

224:
...
  const char *local_name = for_main ? name : canonical;
...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/30672] [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30672

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
                 CC|                            |tromey at sourceware dot org

--- Comment #1 from Tom de Vries <vries at gcc dot gnu.org> ---
Tentative patch:
...
diff --git a/gdb/dwarf2/cooked-index.c b/gdb/dwarf2/cooked-index.c
index 25635d9b72e..8134f8dbbfc 100644
--- a/gdb/dwarf2/cooked-index.c
+++ b/gdb/dwarf2/cooked-index.c
@@ -68,6 +68,25 @@ language_requires_canonicalization (enum language lang)

 /* See cooked-index.h.  */

+cooked_index_entry::cooked_index_entry (sect_offset die_offset_,
+                                       enum dwarf_tag tag_,
+                                       cooked_index_flag flags_,
+                                       const char *name_,
+                                       const cooked_index_entry
*parent_entry_,

+                                       dwarf2_per_cu_data *per_cu_)
+  : name (name_),
+    tag (tag_),
+    flags (flags_),
+    die_offset (die_offset_),
+    parent_entry (parent_entry_),
+    per_cu (per_cu_)
+{
+  if (!language_requires_canonicalization (per_cu->lang ()))
+    canonical = name_;
+}
+
+/* See cooked-index.h.  */
+
 int
 cooked_index_entry::compare (const char *stra, const char *strb,
                             comparison_mode mode)
@@ -343,8 +362,9 @@ cooked_index_shard::do_finalize ()

   for (cooked_index_entry *entry : m_entries)
     {
-      /* Note that this code must be kept in sync with
-        language_requires_canonicalization.  */
+      if (!language_requires_canonicalization (entry->per_cu->lang ()))
+       continue;
+
       gdb_assert (entry->canonical == nullptr);
       if ((entry->flags & IS_LINKAGE) != 0)
        entry->canonical = entry->name;
@@ -388,7 +408,7 @@ cooked_index_shard::do_finalize ()
            }
        }
       else
-       entry->canonical = entry->name;
+       gdb_assert_not_reached ("Unhandled canonicalization");
     }--

   m_names.shrink_to_fit ();
diff --git a/gdb/dwarf2/cooked-index.h b/gdb/dwarf2/cooked-index.h
index 0d6f3e5aa0e..10207305109 100644
--- a/gdb/dwarf2/cooked-index.h
+++ b/gdb/dwarf2/cooked-index.h
@@ -78,15 +78,7 @@ struct cooked_index_entry : public allocate_on_obstack
   cooked_index_entry (sect_offset die_offset_, enum dwarf_tag tag_,
                      cooked_index_flag flags_, const char *name_,
                      const cooked_index_entry *parent_entry_,
-                     dwarf2_per_cu_data *per_cu_)
-    : name (name_),
-      tag (tag_),
-      flags (flags_),
-      die_offset (die_offset_),
-      parent_entry (parent_entry_),
-      per_cu (per_cu_)
-  {
-  }
+                     dwarf2_per_cu_data *per_cu_);

   /* Return true if this entry matches SEARCH_FLAGS.  */
   bool matches (block_search_flags search_flags) const
...

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/30672] [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30672

--- Comment #2 from Tom de Vries <vries at gcc dot gnu.org> ---
Created attachment 15004
  --> https://sourceware.org/bugzilla/attachment.cgi?id=15004&action=edit
tmp.patch

Alternative approach: make canonical std::atomic.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/30672] [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[tromey at sourceware dot org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30672

--- Comment #3 from Tom Tromey <tromey at sourceware dot org> ---
I suspect this is a false positive.
What is the value of "for_main" in that call to write_scope?

Using atomic here seems like it would cause performance problems.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/30672] [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30672

--- Comment #4 from Tom de Vries <vries at gcc dot gnu.org> ---
The problem here is as follows.

This bit of code:
...
  const char *local_name = for_main ? name : canonical;
...
is compiled by gcc at -O2 to load both name and canonical.

This speculative load is considered harmless by the compiler, because it's
considered side-effect free.

The speculation however introduces a data race, which tsan reports.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/30672] [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30672

--- Comment #5 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom Tromey from comment #3)
> I suspect this is a false positive.

Agreed.

> What is the value of "for_main" in that call to write_scope?
> 

I think it's false, but due to optimization gcc speculatively loads
"canonical".

> Using atomic here seems like it would cause performance problems.

I tested that a bit, saw no impact, but yeah that could be the case.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/30672] [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30672

--- Comment #6 from Tom de Vries <vries at gcc dot gnu.org> ---
The culprint in gcc seems to be -fhoist-adjacent-loads.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/30672] [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30672

--- Comment #7 from Tom de Vries <vries at gcc dot gnu.org> ---
(In reply to Tom de Vries from comment #6)
> The culprint in gcc seems to be -fhoist-adjacent-loads.

I've filed https://gcc.gnu.org/bugzilla/show_bug.cgi?id=110799 .

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/30672] [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30672

Tom de Vries <vries at gcc dot gnu.org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
         Resolution|---                         |MOVED
             Status|NEW                         |RESOLVED

--- Comment #8 from Tom de Vries <vries at gcc dot gnu.org> ---
I'm continuing my excercise with -fsanitize=thread -O2
-fno-hoist-adjacent-loads.

For now, closing as resolved-moved.

-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug symtab/30672] [gdb/symtab] data race in cooked_index_shard::do_finalize() / cooked_index_entry::write_scope

[vries at gcc dot gnu.org]

https://sourceware.org/bugzilla/show_bug.cgi?id=30672

--- Comment #9 from Tom de Vries <vries at gcc dot gnu.org> ---
*** Bug 30671 has been marked as a duplicate of this bug. ***

-- 
You are receiving this mail because:
You are on the CC list for the bug.