[Bug exp/30271] New: Addresses of static thread_local fields are badly calculated sometimes

[Bug exp/30271] New: Addresses of static thread_local fields are badly calculated sometimes

[michal.chojnowski at scylladb dot com]

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

            Bug ID: 30271
           Summary: Addresses of static thread_local fields are badly
                    calculated sometimes
           Product: gdb
           Version: HEAD
            Status: UNCONFIRMED
          Severity: normal
          Priority: P2
         Component: exp
          Assignee: unassigned at sourceware dot org
          Reporter: michal.chojnowski at scylladb dot com
  Target Milestone: ---

Created attachment 14779
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14779&action=edit
A patch adding thread_local field handling to value_static_field (possibly in a
wrong manner).

Bottom line:

value_static_field() isn't aware of thread_local fields. It just uses the
address returned by lookup_minimum_symbol() as-is, and for thread_local
variables that's wrong, because in their case the returned value is a TLS
section offset, not the address.

Context:

After we upgraded gdb from 12.1 to 13.1, we noticed that some of our gdb
scripts stopped working because gdb started to evaluate `&a::b` (where `a::b`
is an important `static thread_local` C++ variable) to the TLS offset of `a::b`
instead of its runtime address.
We bisected the problem to commit 3d20b8d99a54382e6e1a6c433e71e0775c6856c6
(`Enable the new DWARF indexer`). When observing the execution of GDB before
and after this patch, we learned that before it the variable would be
successfully looked up via `lookup_symbol_via_quick_fns()`, but after it
`lookup_symbol_via_quick_fns()` doesn't succeed in looking the variable up
(could this be a problem of its own?) and GDB progresses to
`evaluate_expression` -> `value_aggregate_elt` -> `value_static_field` ->
`lookup_minimal_symbol`, and this secondary code path gives the wrong result
because it just can't handle thread_local variables.

I have attached a patch (which I based on some code found in findvars.c) which
fixes our problem. (But I'm clueless about GDB internals, so I can't say
anything about the quality of this patch other than that it solves our
particular case.)

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[tromey at sourceware dot org]

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

Tom Tromey <tromey at sourceware dot org> changed:

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

--- Comment #1 from Tom Tromey <tromey at sourceware dot org> ---
Created attachment 14781
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14781&action=edit
test case

I tried writing a test case to investigate this.
I've attached the test I came up with, based on your
description.  Unfortunately it works for me.
Could you modify the test so that it shows your bug?

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[tromey at sourceware dot org]

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

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |29366


Referenced Bugs:

https://sourceware.org/bugzilla/show_bug.cgi?id=29366
[Bug 29366] [meta] New DWARF indexer meta bug
-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[tromey at sourceware dot org]

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

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|UNCONFIRMED                 |WAITING
     Ever confirmed|0                           |1
   Last reconfirmed|                            |2023-03-25

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[michal.chojnowski at scylladb dot com]

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

--- Comment #2 from Michal Chojnowski <michal.chojnowski at scylladb dot com> ---
Created attachment 14782
  --> https://sourceware.org/bugzilla/attachment.cgi?id=14782&action=edit
A minimal reproducer for the bug

Fails only when the test is compiled with clang.

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[michal.chojnowski at scylladb dot com]

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

--- Comment #3 from Michal Chojnowski <michal.chojnowski at scylladb dot com> ---
I managed to boil it down to a minimal example. I've added the test case as an
attachment.

As it turns out, the problem happens because
`cooked_index_functions::expand_symtabs_matching` fails to expand the symtabs
which contain the definitions of the static fields, because cooked index
entries corresponding to static fields don't have parent_entry set. (For this
part of the bug, thread_local is not relevant — this happens for any static
field, but only thread_local messes up the fallback code path which happens
after the lookup fails).

This only happens when the test is compiled with clang, though. (I tested that
with clang++ 15.0.7). It doesn't seem to happen with gcc. (I tested that with
g++ 12.2.1).

If the relevant symtab is expanded beforehand, e.g. by another lookup, then the
lookup of the static field won't fail. This makes the bug quite hard to hit. In
many cases, the relevant symtab is expanded by the lookup of the class which
occurs immediately before the lookup of the field. That's why two files are
needed in the test — the class lookup expands one symtab, and so the lookup of
one field doesn't fail, but it doesn't expand the other symtab and the lookup
of the other field fails.

Note that this problem with symtabs is orthogonal to the problem with
`value_static_field` which I described in earlier comments. If this turns out
to be a clang bug, `value_static_field` may still deserve a fix.

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[tromey at sourceware dot org]

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

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Blocks|                            |26909


Referenced Bugs:

https://sourceware.org/bugzilla/show_bug.cgi?id=26909
[Bug 26909] [meta] Debug Clang-built code
-- 
You are receiving this mail because:
You are on the CC list for the bug.

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[tromey at sourceware dot org]

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

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
             Status|WAITING                     |NEW

--- Comment #4 from Tom Tromey <tromey at sourceware dot org> ---
Kind of weird DWARF here.  The only location for this
is given by:

 <1><82>: Abbrev Number: 2 (DW_TAG_variable)
    <83>   DW_AT_specification: <0x9f>
    <87>   DW_AT_location    : 10 byte block: e 0 0 0 0 0 0 0 0 e0     
(DW_OP_const8u: 0 0; DW_OP_GNU_push_tls_address or DW_OP_HP_unknown)
    <92>   DW_AT_linkage_name: (indirect string, offset: 0x156):
_ZN9container8tlsvar_0E

It has a forward reference to its defining scope...

gdb currently deals with *backward* references like this by tracking
the DWARF scopes for DIE ranges.  This is needed because gdb's fast
scanner is "forward only", and DWARF provides no way to move up the DIE
tree.

Maybe we can also defer the processing of forward-referencing specifications.

Gosh I hate DWARF.  This stuff is all so needless -- it makes reader
implementation very difficult and provides zero benefit whatsoever.

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[tromey at sourceware dot org]

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

--- Comment #5 from Tom Tromey <tromey at sourceware dot org> ---
Actually it's kind of weird that the existing defer mechanism
doesn't work here.

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[tromey at sourceware dot org]

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

--- Comment #6 from Tom Tromey <tromey at sourceware dot org> ---
I found out that deferred_entry::spec_offset is set but not used... ugh.
Changing the defer code to use this makes the test work.
So I guess maybe the deferral stuff was all working by accident somehow?
What a mess.

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[tromey at sourceware dot org]

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

--- Comment #7 from Tom Tromey <tromey at sourceware dot org> ---
Thank you, btw, for updating the test case.  That was very helpful.

This patch works but I think I want to rewrite the test to use the
DWARF assembler, so it's compiler-independent.

diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index 8f35b973f3e..609fc44190d 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -16577,9 +16577,8 @@ cooked_indexer::make_index (cutu_reader *reader)

   for (const auto &entry : m_deferred_entries)
     {
-      CORE_ADDR key = form_addr (entry.die_offset, m_per_cu->is_dwz);
-      void *obj = m_die_range_map.find (key);
-      cooked_index_entry *parent = static_cast <cooked_index_entry *> (obj);
+      void *obj = m_die_range_map.find (entry.spec_offset);
+      cooked_index_entry *parent = static_cast<cooked_index_entry *> (obj);
       m_index_storage->add (entry.die_offset, entry.tag, entry.flags,
                            entry.name, parent, m_per_cu);
     }

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[tromey at sourceware dot org]

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

Tom Tromey <tromey at sourceware dot org> changed:

           What    |Removed                     |Added
----------------------------------------------------------------------------
   Target Milestone|---                         |13.2

--- Comment #8 from Tom Tromey <tromey at sourceware dot org> ---
I think this is a decent candidate for 13.2

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[cvs-commit at gcc dot gnu.org]

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

--- Comment #9 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The master branch has been updated by Tom Tromey <tromey@sourceware.org>:

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

commit b10f2cd3f3c3b25c71e50a342fb46f9eb9eba792
Author: Tom Tromey <tom@tromey.com>
Date:   Fri Apr 28 13:04:15 2023 -0600

    Correctly handle forward DIE references in scanner

    The cooked index scanner has special code to handle forward DIE
    references.  However, a bug report lead to the discovery that this
    code does not work -- the "deferred_entry::spec_offset" field is
    written to but never used, i.e., the lookup is done using the wrong
    key.

    This patch fixes the bug and adds a regression test.

    The test in the bug itself used a thread_local variable, which
    provoked a failure at runtime.  This test instead uses "maint print
    objfiles" and then inspects to ensure that the entry in question has a
    parent.  This lets us avoid a clang dependency in the test.

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

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[michal.chojnowski at scylladb dot com]

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

--- Comment #10 from Michal Chojnowski <michal.chojnowski at scylladb dot com> ---
Thanks for the fix!

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[cvs-commit at gcc dot gnu.org]

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

--- Comment #11 from cvs-commit at gcc dot gnu.org <cvs-commit at gcc dot gnu.org> ---
The gdb-13-branch branch has been updated by Tom Tromey
<tromey@sourceware.org>:

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

commit 49664141e1446e5f403a716bf8e50375842934b1
Author: Tom Tromey <tom@tromey.com>
Date:   Fri Apr 28 13:04:15 2023 -0600

    Correctly handle forward DIE references in scanner

    The cooked index scanner has special code to handle forward DIE
    references.  However, a bug report lead to the discovery that this
    code does not work -- the "deferred_entry::spec_offset" field is
    written to but never used, i.e., the lookup is done using the wrong
    key.

    This patch fixes the bug and adds a regression test.

    The test in the bug itself used a thread_local variable, which
    provoked a failure at runtime.  This test instead uses "maint print
    objfiles" and then inspects to ensure that the entry in question has a
    parent.  This lets us avoid a clang dependency in the test.

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

    (cherry picked from commit b10f2cd3f3c3b25c71e50a342fb46f9eb9eba792)

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

[Bug exp/30271] Addresses of static thread_local fields are badly calculated sometimes

[tromey at sourceware dot org]

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

Tom Tromey <tromey at sourceware dot org> changed:

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

--- Comment #12 from Tom Tromey <tromey at sourceware dot org> ---
Fixed.

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