[17/20] Change die_info methods to check the attribute's form

Message ID 20200328192208.11324-18-tom@tromey.com
State New
Headers show
Series
  • Make DWARF attribute references safe
Related show

Commit Message

Tom Tromey March 28, 2020, 7:22 p.m.
This changes two die_info methods to check the form of the attribute
before using it.

2020-03-28  Tom Tromey  <tom@tromey.com>

	* dwarf2/die.h (struct die_info) <addr_base, ranges_base>: Check
	the attribute's form.
---
 gdb/ChangeLog    |  5 +++++
 gdb/dwarf2/die.h | 14 ++++++++------
 2 files changed, 13 insertions(+), 6 deletions(-)

-- 
2.17.2

Comments

Simon Marchi March 30, 2020, 4:02 p.m. | #1
On 2020-03-28 3:22 p.m., Tom Tromey wrote:
> This changes two die_info methods to check the form of the attribute

> before using it.

> 

> 2020-03-28  Tom Tromey  <tom@tromey.com>

> 

> 	* dwarf2/die.h (struct die_info) <addr_base, ranges_base>: Check

> 	the attribute's form.

> ---

>  gdb/ChangeLog    |  5 +++++

>  gdb/dwarf2/die.h | 14 ++++++++------

>  2 files changed, 13 insertions(+), 6 deletions(-)

> 

> diff --git a/gdb/dwarf2/die.h b/gdb/dwarf2/die.h

> index 5522ebdf311..37f83a45a50 100644

> --- a/gdb/dwarf2/die.h

> +++ b/gdb/dwarf2/die.h

> @@ -39,11 +39,12 @@ struct die_info

>    gdb::optional<ULONGEST> addr_base ()

>    {

>      for (unsigned i = 0; i < num_attrs; ++i)

> -      if (attrs[i].name == DW_AT_addr_base

> -	  || attrs[i].name == DW_AT_GNU_addr_base)

> +      if ((attrs[i].name == DW_AT_addr_base

> +	   || attrs[i].name == DW_AT_GNU_addr_base)

> +	  && attrs[i].form_is_unsigned ())

>  	{

>  	  /* If both exist, just use the first one.  */

> -	  return DW_UNSND (&attrs[i]);

> +	  return attrs[i].get_unsigned ();

>  	}

>      return gdb::optional<ULONGEST> ();

>    }


A bit like in the previous patch, if there is a DW_AT_addr_base, but it's
not of the right form, I think we should emit a warning saying that we ignore
it, instead of just ignoring it silently.

That's probably out of the scope of this patch though, but I'm just noting it.

Simon
Tom Tromey March 30, 2020, 7:04 p.m. | #2
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


>> +      if ((attrs[i].name == DW_AT_addr_base

>> +	   || attrs[i].name == DW_AT_GNU_addr_base)

>> +	  && attrs[i].form_is_unsigned ())

>> {

>> /* If both exist, just use the first one.  */

>> -	  return DW_UNSND (&attrs[i]);

>> +	  return attrs[i].get_unsigned ();

>> }

>> return gdb::optional<ULONGEST> ();

>> }


Simon> A bit like in the previous patch, if there is a DW_AT_addr_base, but it's
Simon> not of the right form, I think we should emit a warning saying that we ignore
Simon> it, instead of just ignoring it silently.

I tend to think gdb complaints are just time-wasters TBH.

Normally no one examines them.  They aren't visible to users, and if
they were they wouldn't make sense or be actionable anyway.

I enable complaints in my gdbinit but they've turned out just to be
noise.  In fact, last time I fixed a bug that was noted by a complaint,
it turned out I didn't realize that gdb was complaining until well after
the fact.

I'm all for checking the DWARF output of compilers, but I think it's
better as a separate tool; and should be done in a context where someone
actually wants to fix the compiler bugs.

I guess that's why I left out complaints in some spots.

Tom
Simon Marchi March 30, 2020, 8:18 p.m. | #3
On 2020-03-30 3:04 p.m., Tom Tromey wrote:
> I tend to think gdb complaints are just time-wasters TBH.

> 

> Normally no one examines them.  They aren't visible to users, and if

> they were they wouldn't make sense or be actionable anyway.

> 

> I enable complaints in my gdbinit but they've turned out just to be

> noise.  In fact, last time I fixed a bug that was noted by a complaint,

> it turned out I didn't realize that gdb was complaining until well after

> the fact.


Hmm I didn't even know you had to enable them.  When I debug gdb with gdb, I
see some lines like this:

During symbol reading: Member function "~_Sp_counted_base" (offset 0xbc3e90) is virtual but the vtable offset is not specified
During symbol reading: cannot get low and high bounds for subprogram DIE at 0xbd80c0
During symbol reading: Multiple children of DIE 0xbf6816 refer to DIE 0xbf6804 as their abstract origin

I thought those were complaints.

> I'm all for checking the DWARF output of compilers, but I think it's

> better as a separate tool; and should be done in a context where someone

> actually wants to fix the compiler bugs.

> 

> I guess that's why I left out complaints in some spots.


I agree that there's nothing the users can't do much about those issues, so
we shouldn't flood them with meaningless (for them) warnings.  But I'm also
worried that silently ignoring suspicious situations just leads to problems
staying around for longer.

Though if nobody fixes them, they are not really useful.  I'd like to take
the time to take a look at each complaint of GDB and address it (either fix
GDB or open a bug with the compiler), but the reality is that I don't have
time for that.

I think the patches are fine how they are in this regard, this is an issue
orthogonal to the goal of your patchset anyway.

Simon
Tom Tromey March 30, 2020, 8:26 p.m. | #4
>>>>> "Simon" == Simon Marchi <simark@simark.ca> writes:


Simon> Hmm I didn't even know you had to enable them.  When I debug gdb
Simon> with gdb, I see some lines like this:

Simon> During symbol reading: Member function "~_Sp_counted_base" (offset 0xbc3e90) is virtual but the vtable offset is not specified
Simon> During symbol reading: cannot get low and high bounds for subprogram DIE at 0xbd80c0
Simon> During symbol reading: Multiple children of DIE 0xbf6816 refer to DIE 0xbf6804 as their abstract origin

Simon> I thought those were complaints.

Yeah.  That happens because gdb-gdb.gdb (lol) has

  set complaints 1

If you try with -nx you won't see these.

Tom

Patch

diff --git a/gdb/dwarf2/die.h b/gdb/dwarf2/die.h
index 5522ebdf311..37f83a45a50 100644
--- a/gdb/dwarf2/die.h
+++ b/gdb/dwarf2/die.h
@@ -39,11 +39,12 @@  struct die_info
   gdb::optional<ULONGEST> addr_base ()
   {
     for (unsigned i = 0; i < num_attrs; ++i)
-      if (attrs[i].name == DW_AT_addr_base
-	  || attrs[i].name == DW_AT_GNU_addr_base)
+      if ((attrs[i].name == DW_AT_addr_base
+	   || attrs[i].name == DW_AT_GNU_addr_base)
+	  && attrs[i].form_is_unsigned ())
 	{
 	  /* If both exist, just use the first one.  */
-	  return DW_UNSND (&attrs[i]);
+	  return attrs[i].get_unsigned ();
 	}
     return gdb::optional<ULONGEST> ();
   }
@@ -54,11 +55,12 @@  struct die_info
   ULONGEST ranges_base ()
   {
     for (unsigned i = 0; i < num_attrs; ++i)
-      if (attrs[i].name == DW_AT_rnglists_base
-	  || attrs[i].name == DW_AT_GNU_ranges_base)
+      if ((attrs[i].name == DW_AT_rnglists_base
+	   || attrs[i].name == DW_AT_GNU_ranges_base)
+	  && attrs[i].form_is_unsigned ())
 	{
 	  /* If both exist, just use the first one.  */
-	  return DW_UNSND (&attrs[i]);
+	  return attrs[i].get_unsigned ();
 	}
     return 0;
   }