diff mbox

[1/2] Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs

Message ID 20191022152307.37dea0d1@f29-4.lan
State New
Headers show

Commit Message

Kevin Buettner Oct. 22, 2019, 10:23 p.m. UTC
On Fri, 18 Oct 2019 11:07:31 -0400
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> > I looked over your patch; it looks reasonable to me.  If we go that
> > route, I like the idea of introducing a dwarf2_cu_processing_context
> > struct.  (But see below for some later misgivings that I have/had.)  
> 
> Ok, I can try to make something cleaner, but I don't know when it
> would be ready, and I wouldn't want that to block the GDB 9.1 branch
> creation (or release) for that.  Would you like to still push your
> patch (or a perhaps updated version of it) so that we have the fix
> in GDB 9.1?

[...]

> > There may be other concerns too; I'm certain that I didn't look at all
> > of the ways that CU is used in dwarf2_physname and its callees.  
> 
> I don't think it's humanly possible to manually check all the
> possible branches this code can take.  I say, let's do a quick pass
> to check for the obvious (like what you found above), but otherwise
> I'm fine with this patch, it already makes things better than they
> are now.

My testing shows that the patch below still fixes the problem while
also avoiding the poential problems of passing a CU to
compute_delayed_physnames() which is different from the CU of the
methods for which want to compute physnames.

I think that this patch is safer than the one I originally proposed
and is, therefore, a better short term solution.

What do you think?

- - -

Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs

This is a fix for BZ 25065.

GDB segfaults when running either gdb.cp/subtypes.exp or
gdb.cp/local.exp in conjunction with using the -flto compiler/linker
flag.

A much simpler program, which was used to help create the test for
this fix, is:

-- doit.cc --
int main()
{
  class Foo {
  public:
    int doit ()
    {
      return 0;
    }
  };

  Foo foo;

  return foo.doit ();
}
-- end doit.cc --

gcc -o doit -flto -g doit.cc
gdb -q doit
Reading symbols from doit...
(gdb) ptype main::Foo
type = class Foo {
Segmentation fault (core dumped)

The segfault occurs due to a NULL physname in
c_type_print_base_struct_union in c-typeprint.c.  Specifically,
calling is_constructor_name() eventually causes the SIGSEGV is this
code in c-typeprint.c:

	      const char *physname = TYPE_FN_FIELD_PHYSNAME (f, j);
	      int is_full_physname_constructor =
		TYPE_FN_FIELD_CONSTRUCTOR (f, j)
		|| is_constructor_name (physname)
		|| is_destructor_name (physname)
		|| method_name[0] == '~';

However, looking at compute_delayed_physnames(), we see that
the TYPE_FN_FIELD_PHYSNAME field should never be NULL.  This
field will be set to "" for NULL physnames:

      physname = dwarf2_physname (mi.name, mi.die, cu);
      TYPE_FN_FIELD_PHYSNAME (fn_flp->fn_fields, mi.index)
	= physname ? physname : "";

For this particular case, it turns out that compute_delayed_physnames
wasn't being called, which left TYPE_FN_FIELD_PHYSNAME set to the NULL
value that it started with when that data structure was allocated.

The place to fix it, I think, is towards the end of
inherit_abstract_dies().

My first attempt at fix caused the origin CU's method_list (which is
simply the list of methods whose physnames still need to be computed)
to be added to the CU which is doing the inheriting.  One drawback
with this approach is that compute_delayed_physnames is (eventually)
called with a CU that's different than the CU in which the methods
were found.  It's not clear whether this will cause problems or not.

A safer approach, which is what I ultimately settled on, is to call
compute_delayed_physnames() from inherit_abstract_dies().  One
potential drawback is that all needed types might not be known at that
point.  However, in my testing, I haven't seen a problem along these
lines.

gdb/ChangeLog:

	* dwarf2read.c (inherit_abstract_dies): Ensure that delayed
	physnames are computed for inherited DIEs.
    
    Change-Id: I6c6ffe96b301a9daab9f653956b89e3a33fa9445
---
 gdb/dwarf2read.c | 3 +++
 1 file changed, 3 insertions(+)

Comments

Kevin Buettner Nov. 4, 2019, 8:41 p.m. UTC | #1
Ping.

On Tue, 22 Oct 2019 15:23:07 -0700
Kevin Buettner <kevinb@redhat.com> wrote:

> On Fri, 18 Oct 2019 11:07:31 -0400
> Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
> > > I looked over your patch; it looks reasonable to me.  If we go that
> > > route, I like the idea of introducing a dwarf2_cu_processing_context
> > > struct.  (But see below for some later misgivings that I have/had.)    
> > 
> > Ok, I can try to make something cleaner, but I don't know when it
> > would be ready, and I wouldn't want that to block the GDB 9.1 branch
> > creation (or release) for that.  Would you like to still push your
> > patch (or a perhaps updated version of it) so that we have the fix
> > in GDB 9.1?  
> 
> [...]
> 
> > > There may be other concerns too; I'm certain that I didn't look at all
> > > of the ways that CU is used in dwarf2_physname and its callees.    
> > 
> > I don't think it's humanly possible to manually check all the
> > possible branches this code can take.  I say, let's do a quick pass
> > to check for the obvious (like what you found above), but otherwise
> > I'm fine with this patch, it already makes things better than they
> > are now.  
> 
> My testing shows that the patch below still fixes the problem while
> also avoiding the poential problems of passing a CU to
> compute_delayed_physnames() which is different from the CU of the
> methods for which want to compute physnames.
> 
> I think that this patch is safer than the one I originally proposed
> and is, therefore, a better short term solution.
> 
> What do you think?
> 
> - - -
> 
> Fix BZ 25065 - Ensure that physnames are computed for inherited DIEs
> 
> This is a fix for BZ 25065.
> 
> GDB segfaults when running either gdb.cp/subtypes.exp or
> gdb.cp/local.exp in conjunction with using the -flto compiler/linker
> flag.
> 
> A much simpler program, which was used to help create the test for
> this fix, is:
> 
> -- doit.cc --
> int main()
> {
>   class Foo {
>   public:
>     int doit ()
>     {
>       return 0;
>     }
>   };
> 
>   Foo foo;
> 
>   return foo.doit ();
> }
> -- end doit.cc --
> 
> gcc -o doit -flto -g doit.cc
> gdb -q doit
> Reading symbols from doit...
> (gdb) ptype main::Foo
> type = class Foo {
> Segmentation fault (core dumped)
> 
> The segfault occurs due to a NULL physname in
> c_type_print_base_struct_union in c-typeprint.c.  Specifically,
> calling is_constructor_name() eventually causes the SIGSEGV is this
> code in c-typeprint.c:
> 
> 	      const char *physname = TYPE_FN_FIELD_PHYSNAME (f, j);
> 	      int is_full_physname_constructor =
> 		TYPE_FN_FIELD_CONSTRUCTOR (f, j)
> 		|| is_constructor_name (physname)
> 		|| is_destructor_name (physname)
> 		|| method_name[0] == '~';
> 
> However, looking at compute_delayed_physnames(), we see that
> the TYPE_FN_FIELD_PHYSNAME field should never be NULL.  This
> field will be set to "" for NULL physnames:
> 
>       physname = dwarf2_physname (mi.name, mi.die, cu);
>       TYPE_FN_FIELD_PHYSNAME (fn_flp->fn_fields, mi.index)
> 	= physname ? physname : "";
> 
> For this particular case, it turns out that compute_delayed_physnames
> wasn't being called, which left TYPE_FN_FIELD_PHYSNAME set to the NULL
> value that it started with when that data structure was allocated.
> 
> The place to fix it, I think, is towards the end of
> inherit_abstract_dies().
> 
> My first attempt at fix caused the origin CU's method_list (which is
> simply the list of methods whose physnames still need to be computed)
> to be added to the CU which is doing the inheriting.  One drawback
> with this approach is that compute_delayed_physnames is (eventually)
> called with a CU that's different than the CU in which the methods
> were found.  It's not clear whether this will cause problems or not.
> 
> A safer approach, which is what I ultimately settled on, is to call
> compute_delayed_physnames() from inherit_abstract_dies().  One
> potential drawback is that all needed types might not be known at that
> point.  However, in my testing, I haven't seen a problem along these
> lines.
> 
> gdb/ChangeLog:
> 
> 	* dwarf2read.c (inherit_abstract_dies): Ensure that delayed
> 	physnames are computed for inherited DIEs.
>     
>     Change-Id: I6c6ffe96b301a9daab9f653956b89e3a33fa9445
> ---
>  gdb/dwarf2read.c | 3 +++
>  1 file changed, 3 insertions(+)
> 
> diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> index ee9df34ed2..976153640a 100644
> --- a/gdb/dwarf2read.c
> +++ b/gdb/dwarf2read.c
> @@ -13666,6 +13666,9 @@ inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu)
>        origin_child_die = sibling_die (origin_child_die);
>      }
>    origin_cu->list_in_scope = origin_previous_list_in_scope;
> +
> +  if (cu != origin_cu)
> +    compute_delayed_physnames (origin_cu);
>  }
>  
>  static void
>
Simon Marchi Nov. 4, 2019, 8:49 p.m. UTC | #2
On 2019-10-22 6:23 p.m., Kevin Buettner wrote:
> On Fri, 18 Oct 2019 11:07:31 -0400
> Simon Marchi <simon.marchi@polymtl.ca> wrote:
> 
>>> I looked over your patch; it looks reasonable to me.  If we go that
>>> route, I like the idea of introducing a dwarf2_cu_processing_context
>>> struct.  (But see below for some later misgivings that I have/had.)  
>>
>> Ok, I can try to make something cleaner, but I don't know when it
>> would be ready, and I wouldn't want that to block the GDB 9.1 branch
>> creation (or release) for that.  Would you like to still push your
>> patch (or a perhaps updated version of it) so that we have the fix
>> in GDB 9.1?
> 
> [...]
> 
>>> There may be other concerns too; I'm certain that I didn't look at all
>>> of the ways that CU is used in dwarf2_physname and its callees.  
>>
>> I don't think it's humanly possible to manually check all the
>> possible branches this code can take.  I say, let's do a quick pass
>> to check for the obvious (like what you found above), but otherwise
>> I'm fine with this patch, it already makes things better than they
>> are now.
> 
> My testing shows that the patch below still fixes the problem while
> also avoiding the poential problems of passing a CU to
> compute_delayed_physnames() which is different from the CU of the
> methods for which want to compute physnames.
> 
> I think that this patch is safer than the one I originally proposed
> and is, therefore, a better short term solution.
> 
> What do you think?

Yep, this looks good and relatively safe to me.

Thanks!

Simon
Kevin Buettner Nov. 27, 2019, 8:17 p.m. UTC | #3
On Mon, 4 Nov 2019 15:49:21 -0500
Simon Marchi <simon.marchi@polymtl.ca> wrote:

> > I think that this patch is safer than the one I originally proposed
> > and is, therefore, a better short term solution.
> > 
> > What do you think?  
> 
> Yep, this looks good and relatively safe to me.
> 
> Thanks!

I've (finally) pushed this patch along with the test case.

Thanks again for the review.

Kevin
diff mbox

Patch

diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
index ee9df34ed2..976153640a 100644
--- a/gdb/dwarf2read.c
+++ b/gdb/dwarf2read.c
@@ -13666,6 +13666,9 @@  inherit_abstract_dies (struct die_info *die, struct dwarf2_cu *cu)
       origin_child_die = sibling_die (origin_child_die);
     }
   origin_cu->list_in_scope = origin_previous_list_in_scope;
+
+  if (cu != origin_cu)
+    compute_delayed_physnames (origin_cu);
 }
 
 static void