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

[Kevin Buettner <kevinb@redhat.com>]

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
>