[committed][gdb/symtab] Make per_cu->unit_type atomic

[committed][gdb/symtab] Make per_cu->unit_type atomic

[Tom de Vries]

Hi,

With gdb with -fsanitize=thread and test-case gdb.ada/array_bounds.exp, I run
into a data race between:
...
  Read of size 1 at 0x7b2000025f0f by main thread:
    #0 packed<dwarf_unit_type, 1ul>::operator dwarf_unit_type() const packed.h:54
    #1 dwarf2_per_cu_data::set_unit_type(dwarf_unit_type) read.h:339
...
and:
...
  Previous write of size 1 at 0x7b2000025f0f by thread T3:
    #0 dwarf2_per_cu_data::set_unit_type(dwarf_unit_type) read.h:341
...

Fix this by making per_cu->unit_type atomic.

Tested on x86_64-linux.

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

Committed to trunk.

Thanks,
- Tom

[gdb/symtab] Make per_cu->unit_type atomic

---
 gdb/dwarf2/read.h | 23 ++++++++++++++---------
 1 file changed, 14 insertions(+), 9 deletions(-)

diff --git a/gdb/dwarf2/read.h b/gdb/dwarf2/read.h
index fcf0cd70530..acbe298b310 100644
--- a/gdb/dwarf2/read.h
+++ b/gdb/dwarf2/read.h
@@ -178,7 +178,7 @@ struct dwarf2_per_cu_data
 
 private:
   /* The unit type of this CU.  */
-  packed<dwarf_unit_type, 1> m_unit_type = (dwarf_unit_type) 0;
+  std::atomic<packed<dwarf_unit_type, 1>> m_unit_type {(dwarf_unit_type)0};
 
   /* The language of this CU.  */
   packed<language, LANGUAGE_BYTES> m_lang = language_unknown;
@@ -329,19 +329,24 @@ struct dwarf2_per_cu_data
 
   dwarf_unit_type unit_type (bool strict_p = true) const
   {
+    dwarf_unit_type ut = m_unit_type.load ();
     if (strict_p)
-      gdb_assert (m_unit_type != 0);
-    return m_unit_type;
+      gdb_assert (ut != 0);
+    return ut;
   }
 
   void set_unit_type (dwarf_unit_type unit_type)
   {
-    if (m_unit_type == 0)
-      /* Set if not set already.  */
-      m_unit_type = unit_type;
-    else
-      /* If already set, verify that it's the same value.  */
-      gdb_assert (m_unit_type == unit_type);
+    /* Set if not set already.  */
+    packed<dwarf_unit_type, 1> nope = (dwarf_unit_type)0;
+    if (m_unit_type.compare_exchange_strong (nope, unit_type))
+      return;
+
+    /* If already set, verify that it's the same value.  */
+    nope = unit_type;
+    if (m_unit_type.compare_exchange_strong (nope, unit_type))
+      return;
+    gdb_assert_not_reached ();
   }
 
   enum language lang (bool strict_p = true) const

Re: [committed][gdb/symtab] Make per_cu->unit_type atomic

[Tom Tromey]

>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:

Tom> -  packed<dwarf_unit_type, 1> m_unit_type = (dwarf_unit_type) 0;
Tom> +  std::atomic<packed<dwarf_unit_type, 1>> m_unit_type {(dwarf_unit_type)0};
 
I was thinking about this patch this morning and I wonder if packed<> is
even needed now that the member is atomic.

Tom

Re: [committed][gdb/symtab] Make per_cu->unit_type atomic

[Tom de Vries]

On 7/16/22 18:04, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> -  packed<dwarf_unit_type, 1> m_unit_type = (dwarf_unit_type) 0;
> Tom> +  std::atomic<packed<dwarf_unit_type, 1>> m_unit_type {(dwarf_unit_type)0};
>   
> I was thinking about this patch this morning and I wonder if packed<> is
> even needed now that the member is atomic.

With packed:
...
   std::atomic<packed<dwarf_unit_type, 1>> m_unit_type {(dwarf_unit_type)0};
...
we have:
...
         /* size: 120, cachelines: 2, members: 22 */
...
and without:
...
   std::atomic<dwarf_unit_type> m_unit_type {(dwarf_unit_type)0};
...
we have:
...
         /* size: 128, cachelines: 2, members: 22 */
...

So it's necessary to keep the struct small.

Thanks,
- Tom