diff mbox series

[v2,gdb/symtab] Require current language before symbol lookups

Message ID 20250108113816.27724-1-tdevries@suse.de
State Superseded
Headers
Series [v2,gdb/symtab] Require current language before symbol lookups |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_gdb_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-arm success Test passed
linaro-tcwg-bot/tcwg_gdb_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_gdb_check--master-aarch64 success Test passed

Commit Message

Tom de Vries Jan. 8, 2025, 11:38 a.m. UTC 
  Test-case gdb.python/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.)

Fix this by:
- detecting symbol lookup reentrance using an assert, and
- requiring the current language to be set when entering symbol lookup.

Tested on x86_64-linux.

Co-Authored-By: Tom Tromey <tom@tromey.com>

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


base-commit: 19ca41656eef546957d707aa086ab196f0890c51

Comments

Simon Marchi Jan. 9, 2025, 2:55 a.m. UTC | #1 
On 2025-01-08 06:38, Tom de Vries wrote:
> Test-case gdb.python/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.)
> 
> Fix this by:
> - detecting symbol lookup reentrance using an assert, and
> - requiring the current language to be set when entering symbol lookup.
> 
> Tested on x86_64-linux.
> 
> Co-Authored-By: Tom Tromey <tom@tromey.com>
> 
> PR symtab/32490
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32490

It's unfortunate that this is needed, but I guess that fixing it
properly would require a huge refactoring to have the symbol lookup code
not rely on the current language (instead getting it from the context /
parameters).  Just one nit below.

> ---
>  gdb/symtab.c | 39 +++++++++++++++++++++++++++++++++++++++
>  1 file changed, 39 insertions(+)
> 
> diff --git a/gdb/symtab.c b/gdb/symtab.c
> index ba421267b9a..837c5aef6be 100644
> --- a/gdb/symtab.c
> +++ b/gdb/symtab.c
> @@ -124,6 +124,39 @@ struct main_info
>  
>  static const registry<program_space>::key<main_info> main_progspace_key;
>  
> +/* Symbol lookup is not reentrant (though this is not an intrinsic
> +   restriction).  Keep track of whether a symbol lookup is active, to be able
> +   to detect reentrancy.  */
> +static bool in_symbol_lookup;
> +
> +/* Struct to mark that a symbol lookup is active for the duration of its
> +   lifetime.  */
> +
> +struct enter_symbol_lookup
> +{
> +  enter_symbol_lookup ()
> +  {
> +    /* Ensure that the current language has been set.  Normally the
> +       language is set lazily.  However, when performing a symbol lookup,
> +       this could result in a recursive call into the lookup code in some
> +       cases.  Set it now to ensure that this does not happen.  */
> +    get_current_language ();
> +
> +    /* Detect symbol lookup reentrance.  */
> +    gdb_assert (!in_symbol_lookup);
> +
> +    in_symbol_lookup = true;
> +  }
> +
> +  ~enter_symbol_lookup ()
> +  {
> +    /* Sanity check.  */
> +    gdb_assert (in_symbol_lookup);
> +
> +    in_symbol_lookup = false;
> +  }

It's unlikely that a copy or assignment would happen, but still I think
it would be good to use DISABLE_COPY_AND_ASSIGN (i.e. the rule of five).

Simon
Tom de Vries Jan. 13, 2025, 9:51 a.m. UTC | #2 
On 1/9/25 03:55, Simon Marchi wrote:
> 
> 
> On 2025-01-08 06:38, Tom de Vries wrote:
>> Test-case gdb.python/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.)
>>
>> Fix this by:
>> - detecting symbol lookup reentrance using an assert, and
>> - requiring the current language to be set when entering symbol lookup.
>>
>> Tested on x86_64-linux.
>>
>> Co-Authored-By: Tom Tromey <tom@tromey.com>
>>
>> PR symtab/32490
>> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=32490
> 
> It's unfortunate that this is needed, but I guess that fixing it
> properly would require a huge refactoring to have the symbol lookup code
> not rely on the current language (instead getting it from the context /
> parameters).  Just one nit below.
> 
>> ---
>>   gdb/symtab.c | 39 +++++++++++++++++++++++++++++++++++++++
>>   1 file changed, 39 insertions(+)
>>
>> diff --git a/gdb/symtab.c b/gdb/symtab.c
>> index ba421267b9a..837c5aef6be 100644
>> --- a/gdb/symtab.c
>> +++ b/gdb/symtab.c
>> @@ -124,6 +124,39 @@ struct main_info
>>   
>>   static const registry<program_space>::key<main_info> main_progspace_key;
>>   
>> +/* Symbol lookup is not reentrant (though this is not an intrinsic
>> +   restriction).  Keep track of whether a symbol lookup is active, to be able
>> +   to detect reentrancy.  */
>> +static bool in_symbol_lookup;
>> +
>> +/* Struct to mark that a symbol lookup is active for the duration of its
>> +   lifetime.  */
>> +
>> +struct enter_symbol_lookup
>> +{
>> +  enter_symbol_lookup ()
>> +  {
>> +    /* Ensure that the current language has been set.  Normally the
>> +       language is set lazily.  However, when performing a symbol lookup,
>> +       this could result in a recursive call into the lookup code in some
>> +       cases.  Set it now to ensure that this does not happen.  */
>> +    get_current_language ();
>> +
>> +    /* Detect symbol lookup reentrance.  */
>> +    gdb_assert (!in_symbol_lookup);
>> +
>> +    in_symbol_lookup = true;
>> +  }
>> +
>> +  ~enter_symbol_lookup ()
>> +  {
>> +    /* Sanity check.  */
>> +    gdb_assert (in_symbol_lookup);
>> +
>> +    in_symbol_lookup = false;
>> +  }
> 
> It's unlikely that a copy or assignment would happen, but still I think
> it would be good to use DISABLE_COPY_AND_ASSIGN (i.e. the rule of five).

Hi Simon,

thanks for the review.

I've added the DISABLE_COPY_AND_ASSIGN in a v3 ( 
https://sourceware.org/pipermail/gdb-patches/2025-January/214684.html ).

Thanks,
- Tom
Tom Tromey Jan. 14, 2025, 4:06 p.m. UTC | #3 
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:

Simon> It's unfortunate that this is needed, but I guess that fixing it
Simon> properly would require a huge refactoring to have the symbol lookup code
Simon> not rely on the current language (instead getting it from the context /
Simon> parameters).  Just one nit below.

Yes, probably.  It's something we should look at.  Like, maybe the
current language could be captured when the outermost lookup_name_info
object is created.

Tom
diff mbox series

Patch

diff --git a/gdb/symtab.c b/gdb/symtab.c
index ba421267b9a..837c5aef6be 100644
--- a/gdb/symtab.c
+++ b/gdb/symtab.c
@@ -124,6 +124,39 @@  struct main_info
 
 static const registry<program_space>::key<main_info> main_progspace_key;
 
+/* Symbol lookup is not reentrant (though this is not an intrinsic
+   restriction).  Keep track of whether a symbol lookup is active, to be able
+   to detect reentrancy.  */
+static bool in_symbol_lookup;
+
+/* Struct to mark that a symbol lookup is active for the duration of its
+   lifetime.  */
+
+struct enter_symbol_lookup
+{
+  enter_symbol_lookup ()
+  {
+    /* Ensure that the current language has been set.  Normally the
+       language is set lazily.  However, when performing a symbol lookup,
+       this could result in a recursive call into the lookup code in some
+       cases.  Set it now to ensure that this does not happen.  */
+    get_current_language ();
+
+    /* Detect symbol lookup reentrance.  */
+    gdb_assert (!in_symbol_lookup);
+
+    in_symbol_lookup = true;
+  }
+
+  ~enter_symbol_lookup ()
+  {
+    /* Sanity check.  */
+    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 +2292,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)
@@ -2294,6 +2329,8 @@  lookup_global_symbol_from_objfile (struct objfile *main_objfile,
 				   const char *name,
 				   const domain_search_flags domain)
 {
+  enter_symbol_lookup tmp;
+
   gdb_assert (block_index == GLOBAL_BLOCK || block_index == STATIC_BLOCK);
 
   for (objfile *objfile : main_objfile->separate_debug_objfiles ())
@@ -2619,6 +2656,8 @@  lookup_global_or_static_symbol (const char *name,
       return result;
     }
 
+  enter_symbol_lookup tmp;
+
   /* Do a global search (of global blocks, heh).  */
   if (result.symbol == NULL)
     gdbarch_iterate_over_objfiles_in_search_order