[PATCH][gdb/ada] Handle artificial local symbols

[PATCH][gdb/ada] Handle artificial local symbols

[Tom de Vries]

Hi,

With current master and gcc 7.5.0/8.5.0, we have this timeout:
...
(gdb) print s^M
Multiple matches for s^M
[0] cancel^M
[1] s at src/gdb/testsuite/gdb.ada/interface/foo.adb:20^M
[2] s at src/gdb/testsuite/gdb.ada/interface/foo.adb:?^M
> FAIL: gdb.ada/interface.exp: print s (timeout)
...

[ The FAIL doesn't reproduce with gcc 9.3.1.  This difference in
behaviour bisects to gcc commit d70ba0c10de.

The FAIL with earlier gcc bisects to gdb commit ba8694b650b. ]

The FAIL is caused by gcc generating this debug info describing a named
artificial variable:
...
 <2><1204>: Abbrev Number: 31 (DW_TAG_variable)
    <1205>   DW_AT_name        : s.14
    <1209>   DW_AT_type        : <0x1213>
    <120d>   DW_AT_artificial  : 1
    <120d>   DW_AT_location    : 5 byte block: 91 e0 7d 23 18   \
      (DW_OP_fbreg: -288; DW_OP_plus_uconst: 24)
...

An easy way to fix this would be to simply not put named artificial variables
into the symbol table.  However, that causes regressions for Ada.  It relies
on being able to get the value from such variables, using a named reference.

Fix this instead by marking the symbol as artificial, and:
- ignoring such symbols in ada_resolve_variable, which fixes the FAIL
- ignoring such ada symbols in do_print_variable_and_value, which prevents
  them from showing up in "info locals"

Note that a fix for the latter was submitted here (
https://sourceware.org/pipermail/gdb-patches/2008-January/054994.html ), and
this patch borrows from it.

Tested on x86_64-linux.

Co-Authored-By: Joel Brobecker  <brobecker@adacore.com>

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

Any comments?

Thanks,
- Tom

[gdb/ada] Handle artificial local symbols

---
 gdb/ada-lang.c                      | 17 +++++++++++++++++
 gdb/dwarf2/read.c                   |  5 +++++
 gdb/language.h                      |  8 ++++++++
 gdb/stack.c                         |  2 ++
 gdb/symtab.h                        |  7 ++++++-
 gdb/testsuite/gdb.ada/interface.exp |  7 +++++++
 6 files changed, 45 insertions(+), 1 deletion(-)

diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
index 6680a4fd657..487d92be5c9 100644
--- a/gdb/ada-lang.c
+++ b/gdb/ada-lang.c
@@ -3539,6 +3539,17 @@ ada_resolve_variable (struct symbol *sym, const struct block *block,
 	 candidates.end ());
     }
 
+  /* Filter out artificial symbols.  */
+  candidates.erase
+    (std::remove_if
+     (candidates.begin (),
+      candidates.end (),
+      [] (block_symbol &bsym)
+      {
+       return bsym.symbol->artificial;
+      }),
+     candidates.end ());
+
   int i;
   if (candidates.empty ())
     error (_("No definition found for %s"), sym->print_name ());
@@ -13027,6 +13038,12 @@ class ada_language : public language_defn
     return language_defn::read_var_value (var, var_block, frame);
   }
 
+  /* See language.h.  */
+  virtual bool symbol_printing_suppressed (struct symbol *symbol) const override
+  {
+    return symbol->artificial;
+  }
+
   /* See language.h.  */
   void language_arch_info (struct gdbarch *gdbarch,
 			   struct language_arch_info *lai) const override
diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
index a8b44952dc0..9ff13abad40 100644
--- a/gdb/dwarf2/read.c
+++ b/gdb/dwarf2/read.c
@@ -21644,6 +21644,11 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
 	  sym->set_linkage_name (linkagename);
 	}
 
+      /* Handle DW_AT_artificial.  */
+      attr = dwarf2_attr (die, DW_AT_artificial, cu);
+      if (attr != nullptr)
+	sym->artificial = 1;
+
       /* Default assumptions.
 	 Use the passed type or decode it from the die.  */
       SYMBOL_DOMAIN (sym) = VAR_DOMAIN;
diff --git a/gdb/language.h b/gdb/language.h
index 21ed47b3580..40d22d20559 100644
--- a/gdb/language.h
+++ b/gdb/language.h
@@ -327,6 +327,14 @@ struct language_defn
     return {};
   }
 
+  /* Return true if SYMBOL represents an entity that is not
+     supposed to be seen by the user.  To be used to filter symbols
+     during printing.  */
+  virtual bool symbol_printing_suppressed (struct symbol *symbol) const
+  {
+    return false;
+  }
+
   /* The per-architecture (OS/ABI) language information.  */
 
   virtual void language_arch_info (struct gdbarch *,
diff --git a/gdb/stack.c b/gdb/stack.c
index 516e4d45696..7c4cf9491cc 100644
--- a/gdb/stack.c
+++ b/gdb/stack.c
@@ -2322,6 +2322,8 @@ do_print_variable_and_value (const char *print_name,
   if (p->treg.has_value ()
       && !treg_matches_sym_type_name (*p->treg, sym))
     return;
+  if (language_def (sym->language ())->symbol_printing_suppressed (sym))
+    return;
 
   frame = frame_find_by_id (p->frame_id);
   if (frame == NULL)
diff --git a/gdb/symtab.h b/gdb/symtab.h
index fd8dd62a406..fde083f22d5 100644
--- a/gdb/symtab.h
+++ b/gdb/symtab.h
@@ -1122,7 +1122,8 @@ struct symbol : public general_symbol_info, public allocate_on_obstack
       is_argument (0),
       is_inlined (0),
       maybe_copied (0),
-      subclass (SYMBOL_NONE)
+      subclass (SYMBOL_NONE),
+      artificial (0)
     {
       /* We can't use an initializer list for members of a base class, and
 	 general_symbol_info needs to stay a POD type.  */
@@ -1192,6 +1193,10 @@ struct symbol : public general_symbol_info, public allocate_on_obstack
 
   ENUM_BITFIELD (symbol_subclass_kind) subclass : 2;
 
+  /* Whether this symbol is artificial.  */
+
+  unsigned int artificial : 1;
+
   /* Line number of this symbol's definition, except for inlined
      functions.  For an inlined function (class LOC_BLOCK and
      SYMBOL_INLINED set) this is the line number of the function's call
diff --git a/gdb/testsuite/gdb.ada/interface.exp b/gdb/testsuite/gdb.ada/interface.exp
index 68b51917e8d..2dfcd8e8afd 100644
--- a/gdb/testsuite/gdb.ada/interface.exp
+++ b/gdb/testsuite/gdb.ada/interface.exp
@@ -33,3 +33,10 @@ gdb_test "print r" \
 
 gdb_test "print s" \
          "= \\(x => 1, y => 2, w => 3, h => 4\\)"
+
+set cmd "info locals"
+gdb_test $cmd \
+    [multi_line \
+	 $cmd \
+	 "r = \[^\r\n\]*" \
+	 "s = \[^\r\n\]*"]

[PING][PATCH][gdb/ada] Handle artificial local symbols

[Tom de Vries]

On 8/24/21 1:13 PM, Tom de Vries wrote:
> Hi,
> 
> With current master and gcc 7.5.0/8.5.0, we have this timeout:
> ...
> (gdb) print s^M
> Multiple matches for s^M
> [0] cancel^M
> [1] s at src/gdb/testsuite/gdb.ada/interface/foo.adb:20^M
> [2] s at src/gdb/testsuite/gdb.ada/interface/foo.adb:?^M
>> FAIL: gdb.ada/interface.exp: print s (timeout)
> ...
> 
> [ The FAIL doesn't reproduce with gcc 9.3.1.  This difference in
> behaviour bisects to gcc commit d70ba0c10de.
> 
> The FAIL with earlier gcc bisects to gdb commit ba8694b650b. ]
> 
> The FAIL is caused by gcc generating this debug info describing a named
> artificial variable:
> ...
>  <2><1204>: Abbrev Number: 31 (DW_TAG_variable)
>     <1205>   DW_AT_name        : s.14
>     <1209>   DW_AT_type        : <0x1213>
>     <120d>   DW_AT_artificial  : 1
>     <120d>   DW_AT_location    : 5 byte block: 91 e0 7d 23 18   \
>       (DW_OP_fbreg: -288; DW_OP_plus_uconst: 24)
> ...
> 
> An easy way to fix this would be to simply not put named artificial variables
> into the symbol table.  However, that causes regressions for Ada.  It relies
> on being able to get the value from such variables, using a named reference.
> 
> Fix this instead by marking the symbol as artificial, and:
> - ignoring such symbols in ada_resolve_variable, which fixes the FAIL
> - ignoring such ada symbols in do_print_variable_and_value, which prevents
>   them from showing up in "info locals"
> 
> Note that a fix for the latter was submitted here (
> https://sourceware.org/pipermail/gdb-patches/2008-January/054994.html ), and
> this patch borrows from it.
> 
> Tested on x86_64-linux.
> 
> Co-Authored-By: Joel Brobecker  <brobecker@adacore.com>
> 
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=28180
> 
> Any comments?
> 

Ping.

Thanks,
- Tom

> [gdb/ada] Handle artificial local symbols
> 
> ---
>  gdb/ada-lang.c                      | 17 +++++++++++++++++
>  gdb/dwarf2/read.c                   |  5 +++++
>  gdb/language.h                      |  8 ++++++++
>  gdb/stack.c                         |  2 ++
>  gdb/symtab.h                        |  7 ++++++-
>  gdb/testsuite/gdb.ada/interface.exp |  7 +++++++
>  6 files changed, 45 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/ada-lang.c b/gdb/ada-lang.c
> index 6680a4fd657..487d92be5c9 100644
> --- a/gdb/ada-lang.c
> +++ b/gdb/ada-lang.c
> @@ -3539,6 +3539,17 @@ ada_resolve_variable (struct symbol *sym, const struct block *block,
>  	 candidates.end ());
>      }
>  
> +  /* Filter out artificial symbols.  */
> +  candidates.erase
> +    (std::remove_if
> +     (candidates.begin (),
> +      candidates.end (),
> +      [] (block_symbol &bsym)
> +      {
> +       return bsym.symbol->artificial;
> +      }),
> +     candidates.end ());
> +
>    int i;
>    if (candidates.empty ())
>      error (_("No definition found for %s"), sym->print_name ());
> @@ -13027,6 +13038,12 @@ class ada_language : public language_defn
>      return language_defn::read_var_value (var, var_block, frame);
>    }
>  
> +  /* See language.h.  */
> +  virtual bool symbol_printing_suppressed (struct symbol *symbol) const override
> +  {
> +    return symbol->artificial;
> +  }
> +
>    /* See language.h.  */
>    void language_arch_info (struct gdbarch *gdbarch,
>  			   struct language_arch_info *lai) const override
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index a8b44952dc0..9ff13abad40 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -21644,6 +21644,11 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>  	  sym->set_linkage_name (linkagename);
>  	}
>  
> +      /* Handle DW_AT_artificial.  */
> +      attr = dwarf2_attr (die, DW_AT_artificial, cu);
> +      if (attr != nullptr)
> +	sym->artificial = 1;
> +
>        /* Default assumptions.
>  	 Use the passed type or decode it from the die.  */
>        SYMBOL_DOMAIN (sym) = VAR_DOMAIN;
> diff --git a/gdb/language.h b/gdb/language.h
> index 21ed47b3580..40d22d20559 100644
> --- a/gdb/language.h
> +++ b/gdb/language.h
> @@ -327,6 +327,14 @@ struct language_defn
>      return {};
>    }
>  
> +  /* Return true if SYMBOL represents an entity that is not
> +     supposed to be seen by the user.  To be used to filter symbols
> +     during printing.  */
> +  virtual bool symbol_printing_suppressed (struct symbol *symbol) const
> +  {
> +    return false;
> +  }
> +
>    /* The per-architecture (OS/ABI) language information.  */
>  
>    virtual void language_arch_info (struct gdbarch *,
> diff --git a/gdb/stack.c b/gdb/stack.c
> index 516e4d45696..7c4cf9491cc 100644
> --- a/gdb/stack.c
> +++ b/gdb/stack.c
> @@ -2322,6 +2322,8 @@ do_print_variable_and_value (const char *print_name,
>    if (p->treg.has_value ()
>        && !treg_matches_sym_type_name (*p->treg, sym))
>      return;
> +  if (language_def (sym->language ())->symbol_printing_suppressed (sym))
> +    return;
>  
>    frame = frame_find_by_id (p->frame_id);
>    if (frame == NULL)
> diff --git a/gdb/symtab.h b/gdb/symtab.h
> index fd8dd62a406..fde083f22d5 100644
> --- a/gdb/symtab.h
> +++ b/gdb/symtab.h
> @@ -1122,7 +1122,8 @@ struct symbol : public general_symbol_info, public allocate_on_obstack
>        is_argument (0),
>        is_inlined (0),
>        maybe_copied (0),
> -      subclass (SYMBOL_NONE)
> +      subclass (SYMBOL_NONE),
> +      artificial (0)
>      {
>        /* We can't use an initializer list for members of a base class, and
>  	 general_symbol_info needs to stay a POD type.  */
> @@ -1192,6 +1193,10 @@ struct symbol : public general_symbol_info, public allocate_on_obstack
>  
>    ENUM_BITFIELD (symbol_subclass_kind) subclass : 2;
>  
> +  /* Whether this symbol is artificial.  */
> +
> +  unsigned int artificial : 1;
> +
>    /* Line number of this symbol's definition, except for inlined
>       functions.  For an inlined function (class LOC_BLOCK and
>       SYMBOL_INLINED set) this is the line number of the function's call
> diff --git a/gdb/testsuite/gdb.ada/interface.exp b/gdb/testsuite/gdb.ada/interface.exp
> index 68b51917e8d..2dfcd8e8afd 100644
> --- a/gdb/testsuite/gdb.ada/interface.exp
> +++ b/gdb/testsuite/gdb.ada/interface.exp
> @@ -33,3 +33,10 @@ gdb_test "print r" \
>  
>  gdb_test "print s" \
>           "= \\(x => 1, y => 2, w => 3, h => 4\\)"
> +
> +set cmd "info locals"
> +gdb_test $cmd \
> +    [multi_line \
> +	 $cmd \
> +	 "r = \[^\r\n\]*" \
> +	 "s = \[^\r\n\]*"]
> 

Re: [PATCH][gdb/ada] Handle artificial local symbols

[Tom Tromey]

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

Tom> Fix this instead by marking the symbol as artificial, and:
Tom> - ignoring such symbols in ada_resolve_variable, which fixes the FAIL
Tom> - ignoring such ada symbols in do_print_variable_and_value, which prevents
Tom>   them from showing up in "info locals"

Tom> Note that a fix for the latter was submitted here (
Tom> https://sourceware.org/pipermail/gdb-patches/2008-January/054994.html ), and
Tom> this patch borrows from it.

Thanks for doing this.

Tom> +      /* Handle DW_AT_artificial.  */
Tom> +      attr = dwarf2_attr (die, DW_AT_artificial, cu);
Tom> +      if (attr != nullptr)
Tom> +	sym->artificial = 1;

This should also check attr->as_boolean.
Probably also use '= true', though

   sym->artificial = attr->as_boolean ()

would also be fine.

Tom> +  if (language_def (sym->language ())->symbol_printing_suppressed (sym))
Tom> +    return;

Seems fine though I also wonder if it would be better to just always
suppress artificial variables here.

Tom> +  /* Whether this symbol is artificial.  */
Tom> +
Tom> +  unsigned int artificial : 1;

I think making this bool would be better.

Tom

Re: [PATCH][gdb/ada] Handle artificial local symbols

[Tom de Vries]

On 9/17/21 9:42 PM, Tom Tromey wrote:
>>>>>> "Tom" == Tom de Vries via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Tom> Fix this instead by marking the symbol as artificial, and:
> Tom> - ignoring such symbols in ada_resolve_variable, which fixes the FAIL
> Tom> - ignoring such ada symbols in do_print_variable_and_value, which prevents
> Tom>   them from showing up in "info locals"
> 
> Tom> Note that a fix for the latter was submitted here (
> Tom> https://sourceware.org/pipermail/gdb-patches/2008-January/054994.html ), and
> Tom> this patch borrows from it.
> 
> Thanks for doing this.
> 
> Tom> +      /* Handle DW_AT_artificial.  */
> Tom> +      attr = dwarf2_attr (die, DW_AT_artificial, cu);
> Tom> +      if (attr != nullptr)
> Tom> +	sym->artificial = 1;
> 
> This should also check attr->as_boolean.
> Probably also use '= true', though
> 
>    sym->artificial = attr->as_boolean ()
> 
> would also be fine.
> 

Ack.

> Tom> +  if (language_def (sym->language ())->symbol_printing_suppressed (sym))
> Tom> +    return;
> 
> Seems fine though I also wonder if it would be better to just always
> suppress artificial variables here.
> 

There may be trouble with other languages though.  So for now I'm going
ahead with this approach.

> Tom> +  /* Whether this symbol is artificial.  */
> Tom> +
> Tom> +  unsigned int artificial : 1;
> 
> I think making this bool would be better.

Hmm, the struct is advertised as space-critical, and a 1-bit bitfield is
smaller than an 8-bit bool.  So I wonder why you prefer bool here.

Thanks,
- Tom

Re: [PATCH][gdb/ada] Handle artificial local symbols

[Tom Tromey]

>> I think making this bool would be better.

Tom> Hmm, the struct is advertised as space-critical, and a 1-bit bitfield is
Tom> smaller than an 8-bit bool.  So I wonder why you prefer bool here.

Sorry, I meant 'bool ... : 1', which should work ok.

Tom