Require current language before symbol lookups

  py-symbol.exp fails with various target boards, including fission and
gold-gdb-index.

The problem here is that, in this test, the current language is still
unset (i.e., lazy) when the symbol lookup is done.  It is eventually
set deep in the lookup -- but this then requires a reentrant symbol
lookup, which fails.  (DWARF symbol lookup is not reentrant.)

This patch changes the top-level symbol-lookup functions to require
that the current language be available.  This fixes the bug.

Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32490
---
 gdb/symtab.c | 21 +++++++++++++++++++++
 1 file changed, 21 insertions(+)
  

On 12/28/24 22:03, Tom Tromey wrote:
> py-symbol.exp fails with various target boards, including fission and
> gold-gdb-index.
> 
> The problem here is that, in this test, the current language is still
> unset (i.e., lazy) when the symbol lookup is done.  It is eventually
> set deep in the lookup -- but this then requires a reentrant symbol
> lookup, which fails.  (DWARF symbol lookup is not reentrant.)
> 
> This patch changes the top-level symbol-lookup functions to require
> that the current language be available.  This fixes the bug.
> 

Hi Tom,

thanks for working on this.

I've applied the patch and verified that it detects failure and passes 
after rebuilding with the fix with make-check-all.sh, which covers the 
three boards with FAILs reported in the PR.

After reading the patch, I understand how it fixes the problem described 
above, but I wonder if we can get a better fix by approaching it 
differently.

AFAIU there are two related problems:
- DWARF symbol lookup is not reentrant, but reentrancy is not detected
- calling a top-level symbol lookup function without having the current
   language set causes a reentrant symbol lookup.

The proposed patch fixes the second problem, but not the first.

[ And I care about the first problem because, well, I tried to analyse 
this PR but didn't manage, and if gdb would have detected the reentrancy 
I would have had an easier starting point. ]

So, I tried the following approach:
- first fix the first problem (see patch below, without the
   "get_current_language ()" line), changing the failure mode
   of the second problem to an assertion failure, and
- then fix the second problem by adding the "get_current_language ()"
   line in a single location.

I started out with the "enter_symbol_lookup tmp" lines in the same 
locations as the "require_language_to_be_set ()" lines in the proposed 
patch, and had to shuffle things around a bit to handle top-level 
functions calling other top-level functions.

[ If this approach is feasible, perhaps also
"gdb_assert (in_symbol_lookup)" could be added in some places to detect 
incomplete reentrancy detection, but I'm not sure at this point where. ]

Anyway, I've tested this on x86_64-linux, as well as py-symbol.exp with 
make-check-all.sh.

WDYT?

Thanks,
- Tom

diff --git a/gdb/symtab.c b/gdb/symtab.c
index ba421267b9a..a268336d154 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -124,6 +124,21 @@ struct main_info

  static const registry<program_space>::key<main_info> main_progspace_key;

+static bool in_symbol_lookup;
+
+struct enter_symbol_lookup
+{
+  enter_symbol_lookup () {
+    gdb_assert (!in_symbol_lookup);
+    get_current_language ();
+    in_symbol_lookup = true;
+  }
+  ~enter_symbol_lookup () {
+    gdb_assert (in_symbol_lookup);
+    in_symbol_lookup = false;
+  }
+};
+
  /* The default symbol cache size.
     There is no extra cpu cost for large N (except when flushing the cache,
     which is rare).  The value here is just a first attempt.  A better 
default
@@ -2259,6 +2274,8 @@ lookup_symbol_in_block (const char *name, 
symbol_name_match_type
match_type,
  			const struct block *block,
  			const domain_search_flags domain)
  {
+  enter_symbol_lookup tmp;
+
    struct symbol *sym;

    if (symbol_lookup_debug)
@@ -2296,6 +2313,8 @@ lookup_global_symbol_from_objfile (struct objfile 
*main_objfile,

  {
    gdb_assert (block_index == GLOBAL_BLOCK || block_index == STATIC_BLOCK);

+  enter_symbol_lookup tmp;
+
    for (objfile *objfile : main_objfile->separate_debug_objfiles ())
      {
        struct block_symbol result
@@ -2600,6 +2619,7 @@ lookup_global_or_static_symbol (const char *name,
  				struct objfile *objfile,
  				const domain_search_flags domain)
  {
+  enter_symbol_lookup tmp;
    struct symbol_cache *cache = get_symbol_cache (current_program_space);
    struct block_symbol result;
    struct block_symbol_cache *bsc;
  

Tom> I started out with the "enter_symbol_lookup tmp" lines in the same
Tom> locations as the "require_language_to_be_set ()" lines in the proposed
Tom> patch, and had to shuffle things around a bit to handle top-level
Tom> functions calling other top-level functions.

Tom> [ If this approach is feasible, perhaps also
Tom> "gdb_assert (in_symbol_lookup)" could be added in some places to
Tom> detect incomplete reentrancy detection, but I'm not sure at this point
Tom> where. ]

Tom> Anyway, I've tested this on x86_64-linux, as well as py-symbol.exp
Tom> with make-check-all.sh.

Tom> WDYT?

I'm ok with this.  It needs comments and reformatting.

There's not really an intrinsic reason that the DWARF reader should be
non-reentrant.  I wouldn't mind lifting this restriction.  However that
requires the "look up via psymtabs" series, aka
https://sourceware.org/bugzilla/show_bug.cgi?id=16998

Tom
  

On 1/8/25 00:48, Tom Tromey wrote:
> Tom> I started out with the "enter_symbol_lookup tmp" lines in the same
> Tom> locations as the "require_language_to_be_set ()" lines in the proposed
> Tom> patch, and had to shuffle things around a bit to handle top-level
> Tom> functions calling other top-level functions.
> 
> Tom> [ If this approach is feasible, perhaps also
> Tom> "gdb_assert (in_symbol_lookup)" could be added in some places to
> Tom> detect incomplete reentrancy detection, but I'm not sure at this point
> Tom> where. ]
> 
> Tom> Anyway, I've tested this on x86_64-linux, as well as py-symbol.exp
> Tom> with make-check-all.sh.
> 
> Tom> WDYT?
> 
> I'm ok with this.  It needs comments and reformatting.
> 

Done, and submitted as v2 ( 
https://sourceware.org/pipermail/gdb-patches/2025-January/214546.html ).

> There's not really an intrinsic reason that the DWARF reader should be
> non-reentrant.  I wouldn't mind lifting this restriction.  However that
> requires the "look up via psymtabs" series, aka
> https://sourceware.org/bugzilla/show_bug.cgi?id=16998

I've mentioned in the comments that the limitation is not intrinsic.

Thanks,
- Tom