RE: [PATCH 4/6] Dwarf: Fortran, support DW_TAG_entry_point.

["Wiederhake, Tim" <tim dot wiederhake at intel dot com>]

See below.

> -----Original Message-----
> From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> owner@sourceware.org] On Behalf Of Wiederhake, Tim
> Sent: Friday, August 4, 2017 1:04 PM
> To: Yao Qi <qiyaoltc@gmail.com>
> Cc: gdb-patches@sourceware.org
> Subject: RE: [PATCH 4/6] Dwarf: Fortran, support DW_TAG_entry_point.
> 
> Hi Yao!
> 
> > -----Original Message-----
> > From: gdb-patches-owner@sourceware.org [mailto:gdb-patches-
> > owner@sourceware.org] On Behalf Of Yao Qi
> > Sent: Wednesday, August 2, 2017 3:15 PM
> > To: Wiederhake, Tim <tim.wiederhake@intel.com>
> > Cc: gdb-patches@sourceware.org; Bernhard Heckel
> > <bernhard.heckel@intel.com>
> > Subject: Re: [PATCH 4/6] Dwarf: Fortran, support DW_TAG_entry_point.
> >
> > Tim Wiederhake <tim.wiederhake@intel.com> writes:
> >
> > > Fortran provides additional entry-points to an subprogram.  Those
> entry-
> > points
> > > may have only a subset of parameters of the original subprogram as
> well.
> > >
> > > Add support for parsing DW_TAG_entry_point's for Fortran.
> >
> > Looks gfortran doesn't generate DW_TAG_entry_point now.  Could you
> > confirm that this patch fix PR 8043 and PR 9279?
> 
> Tested and confirmed.
> 
> > >
> > > xxxx-yy-zz  Bernhard Heckel  <bernhard.heckel@intel.com>
> > >             Tim Wiederhake  <tim.wiederhake@intel.com>
> > >
> > > gdb/ChangeLog:
> >
> > 	PR fortran/8043
> > 	PR fortran/9279
> > > 	* gdb/dwarf2read.c (add_partial_symbol): Handle DW_TAG_entry_point.
> > > 	(add_partial_entry_point): New function.
> > > 	(add_partial_subprogram): Search for entry_points.
> > > 	(process_die): Handle DW_TAG_entry_point.
> > > 	(dwarf2_get_pc_bounds): Update low pc from DWARF.
> > > 	(load_partial_dies): Save DW_TAG_entry_point's.
> > > 	(load_partial_dies): Save DW_TAG_entry_point to hash table.
> > > 	(load_partial_dies): Look into child's of DW_TAG_sub_program
> > > 	for fortran.
> > > 	(new_symbol_full): Process DW_TAG_entry_point.
> > > 	(read_type_die_1): Handle DW_TAG_entry_point.
> > >
> > > gdb/testsuite/ChangeLog:
> > > 	* gdb.fortran/entry_point.f90: New file.
> > > 	* gdb.fortran/entry_point.exp: New file.
> > >
> > >
> > > ---
> > >  gdb/dwarf2read.c                          | 100
> > +++++++++++++++++++++++++++++-
> > >  gdb/testsuite/gdb.fortran/entry_point.exp |  70 +++++++++++++++++++++
> > >  gdb/testsuite/gdb.fortran/entry_point.f90 |  48 ++++++++++++++
> > >  3 files changed, 217 insertions(+), 1 deletion(-)
> > >  create mode 100644 gdb/testsuite/gdb.fortran/entry_point.exp
> > >  create mode 100644 gdb/testsuite/gdb.fortran/entry_point.f90
> > >
> > > diff --git a/gdb/dwarf2read.c b/gdb/dwarf2read.c
> > > index 6d38d70..c3c01a7 100644
> > > --- a/gdb/dwarf2read.c
> > > +++ b/gdb/dwarf2read.c
> > > @@ -1556,6 +1556,10 @@ static void add_partial_module (struct
> > partial_die_info *pdi, CORE_ADDR *lowpc,
> > >  static void add_partial_enumeration (struct partial_die_info
> *enum_pdi,
> > >  				     struct dwarf2_cu *cu);
> > >
> > > +static void add_partial_entry_point (struct partial_die_info *pdi,
> > > +				     CORE_ADDR *lowpc, CORE_ADDR *highpc,
> > > +				     int need_pc, struct dwarf2_cu *cu);
> > > +
> > >  static void add_partial_subprogram (struct partial_die_info *pdi,
> > >  				    CORE_ADDR *lowpc, CORE_ADDR *highpc,
> > >  				    int need_pc, struct dwarf2_cu *cu);
> > > @@ -7132,6 +7136,32 @@ add_partial_symbol (struct partial_die_info
> *pdi,
> > struct dwarf2_cu *cu)
> > >
> > >    switch (pdi->tag)
> > >      {
> > > +    case DW_TAG_entry_point:
> > > +      {
> > > +	/* Don't know any other language than fortran which is
> > > +	   using DW_TAG_entry_point.  */
> > > +	if (cu->language == language_fortran)
> > > +	  {
> > > +	    addr = gdbarch_adjust_dwarf2_addr (gdbarch, pdi->lowpc +
> > baseaddr);
> > > +	    /* DW_TAG_entry_point provides an additional entry_point to an
> > > +	       existing sub_program.  Therefore, we inherit the "external"
> > > +	       attribute from the sub_program to which the entry_point
> > > +	       belongs to.  */
> > > +	    if (pdi->die_parent->is_external)
> > > +	      add_psymbol_to_list (actual_name, strlen (actual_name),
> > > +				   built_actual_name != NULL,
> > > +				   VAR_DOMAIN, LOC_BLOCK,
> > > +				   &objfile->global_psymbols,
> > > +				   addr, cu->language, objfile);
> > > +	    else
> > > +	      add_psymbol_to_list (actual_name, strlen (actual_name),
> > > +				   built_actual_name != NULL,
> > > +				   VAR_DOMAIN, LOC_BLOCK,
> > > +				   &objfile->static_psymbols,
> > > +				   addr, cu->language, objfile);
> >
> > Nit, can you do
> >
> >      add_psymbol_to_list (actual_name, strlen (actual_name),
> >                           built_actual_name != NULL,
> >                           VAR_DOMAIN, LOC_BLOCK,
> >                           pdi->die_parent->is_external ?
> >                           &objfile->global_psymbols : &objfile-
> > >static_psymbols,
> >                           addr, cu->language, objfile);
> >
> > or
> >
> >     auto psymbols = pdi->die_parent->is_external ?
> >                           &objfile->global_psymbols : &objfile-
> > >static_psymbols;
> >
> >      add_psymbol_to_list (actual_name, strlen (actual_name),
> >                           built_actual_name != NULL,
> >                           VAR_DOMAIN, LOC_BLOCK,
> >                           psymbols,
> >                           addr, cu->language, objfile);
> 
> Changed locally.
> 
> > > +	  }
> > > +	break;
> > > +      }
> > >      case DW_TAG_subprogram:
> > >        addr = gdbarch_adjust_dwarf2_addr (gdbarch, pdi->lowpc +
> > baseaddr);
> > >        if (pdi->is_external || cu->language == language_ada)
> > > @@ -7332,6 +7362,18 @@ add_partial_module (struct partial_die_info
> *pdi,
> > CORE_ADDR *lowpc,
> > >      scan_partial_symbols (pdi->die_child, lowpc, highpc, set_addrmap,
> > cu);
> > >  }
> > >
> > > +static void
> > > +add_partial_entry_point (struct partial_die_info *pdi,
> > > +			 CORE_ADDR *p_lowpc, CORE_ADDR *p_highpc,
> > > +			 int set_addrmap, struct dwarf2_cu *cu)
> > > +{
> > > +  if (pdi->name == NULL)
> > > +    complaint (&symfile_complaints,
> > > +	       _("DW_TAG_entry_point have to have a name"));
> > > +  else
> > > +    add_partial_symbol (pdi, cu);
> > > +}
> > > +
> > >  /* Read a partial die corresponding to a subprogram and create a
> > partial
> > >     symbol for that subprogram.  When the CU language allows it, this
> > >     routine also defines a partial symbol for each nested subprogram
> > > @@ -7402,6 +7444,16 @@ add_partial_subprogram (struct partial_die_info
> > *pdi,
> > >  	  pdi = pdi->die_sibling;
> > >  	}
> > >      }
> > > +  else if (cu->language == language_fortran)
> > > +    {
> > > +      pdi = pdi->die_child;
> > > +      while (pdi != NULL)
> > > +	{
> > > +	  if (pdi->tag == DW_TAG_entry_point)
> > > +	    add_partial_entry_point (pdi, lowpc, highpc, set_addrmap, cu);
> >
> > In Fortran, is it possible to define "nested functions" or "contained
> > routines" within an entry point of a subprogram?  like,
> >
> > subroutine bar
> >
> >   entry foo
> >
> >     contains
> >        subroutine foo_sub
> >              entry foo_sub_entry
> >        end subroutine foo_sub
> >   entry bar
> >
> > end subroutine
> 
> Will test and report.

  $ gfortran -g -o test test.f90 
  test.f90:5:15:
  
      entry foo_sub_entry
                 1
  Error: ENTRY statement at (1) cannot appear in a contained procedure

Regards,
Tim

> > can gdb correctly handle these nested functions and nested entry points?
> > My point is that it is better that add_partial_subprogram handles both
> > DW_TAG_subprogram and DW_TAG_entry_point first, and then, recursively
> > call itself to handle DIE children.
> >
> > add_partial_subprogram ()
> > {
> >   if (pdi->tag == DW_TAG_subprogram)
> >      {}
> >   else if (pdi->tag == DW_TAG_entry_point)
> >      {}
> >
> >   if (! pdi->has_children)
> >     return;
> >
> >   if (cu->language == language_ada || cu->language == language_fortran)
> >     {
> >       pdi = pdi->die_child;
> >       while (pdi != NULL)
> > 	{
> > 	  fixup_partial_die (pdi, cu);
> > 	  if (pdi->tag == DW_TAG_subprogram
> > 	      || pdi->tag == DW_TAG_lexical_block
> >               || pdi->tag == DW_TAG_entry_point)
> > 	    add_partial_subprogram (pdi, lowpc, highpc, set_addrmap, cu);
> > 	  pdi = pdi->die_sibling;
> > 	}
> >     }
> > }
> >
> > > +	  pdi = pdi->die_sibling;
> > > +	}
> > > +    }
> > >  }
> > >
> > >  /* Read a partial die corresponding to an enumeration type.  */
> > > @@ -8508,6 +8560,7 @@ process_die (struct die_info *die, struct
> > dwarf2_cu *cu)
> > >      case DW_TAG_type_unit:
> > >        read_type_unit_scope (die, cu);
> > >        break;
> > > +    case DW_TAG_entry_point:
> > >      case DW_TAG_subprogram:
> > >      case DW_TAG_inlined_subroutine:
> > >        read_func_scope (die, cu);
> > > @@ -12499,6 +12552,27 @@ dwarf2_get_pc_bounds (struct die_info *die,
> > CORE_ADDR *lowpc,
> > >    CORE_ADDR high = 0;
> > >    enum pc_bounds_kind ret;
> > >
> > > +  if (die->tag == DW_TAG_entry_point)
> > > +    {
> > > +      /* Entry_point is embedded in an subprogram.  Therefore, we can
> > use the
> > > +	 highpc from its enveloping subprogram and get the lowpc from DWARF.
> > */
> > > +      ret = dwarf2_get_pc_bounds (die->parent, lowpc, highpc, cu,
> pst);
> > > +      if (ret == PC_BOUNDS_NOT_PRESENT || ret == PC_BOUNDS_INVALID)
> > > +	return ret;
> > > +
> > > +      attr = dwarf2_attr (die, DW_AT_low_pc, cu);
> > > +      if (!attr)
> > > +	{
> > > +	  complaint (&symfile_complaints,
> > > +		     _("DW_TAG_entry_point is missing DW_AT_low_pc"));
> > > +	  return PC_BOUNDS_INVALID;
> > > +	}
> > > +      low = attr_value_as_address (attr);
> > > +      *lowpc = low;
> > > +
> > > +      return PC_BOUNDS_HIGH_LOW;
> >
> > Why return PC_BOUNDS_HIGH_LOW, which means both DW_AT_low_pc and
> > DW_AT_high_pc are found.  However, DW_TAG_entry_point doesn't have
> > DW_AT_high_pc.  The question is why do we call dwarf2_get_pc_bounds for
> > DW_TAG_entry_point.  Is it because we call read_func_scope for
> > DW_TAG_entry_point?
> 
> I may be misunderstanding you here. Yes, DW_TAG_entry_point doesn't
> have DW_AT_high_pc but we know that value implicitly from the surrounding
> subprogram, as explained in the comment above.
> 
> > > +    }
> > > +
> > >    attr_high = dwarf2_attr (die, DW_AT_high_pc, cu);
> > >    if (attr_high)
> > >      {
> > > @@ -16029,6 +16103,7 @@ load_partial_dies (const struct
> die_reader_specs
> > *reader,
> > >  	  && abbrev->tag != DW_TAG_constant
> > >  	  && abbrev->tag != DW_TAG_enumerator
> > >  	  && abbrev->tag != DW_TAG_subprogram
> > > +	  && abbrev->tag != DW_TAG_entry_point
> > >  	  && abbrev->tag != DW_TAG_lexical_block
> > >  	  && abbrev->tag != DW_TAG_variable
> > >  	  && abbrev->tag != DW_TAG_namespace
> > > @@ -16155,6 +16230,7 @@ load_partial_dies (const struct
> die_reader_specs
> > *reader,
> > >        if (load_all
> > >  	  || abbrev->tag == DW_TAG_constant
> > >  	  || abbrev->tag == DW_TAG_subprogram
> > > +	  || abbrev->tag == DW_TAG_entry_point
> >
> > Could you update the comments above this block?
> 
> 
> Sorry, which comments specifically?
> The comment directly above the last block states:
>   DW_AT_abstract_origin refers to functions (and many things under the
> function DIE [...])"
> 
> > >  	  || abbrev->tag == DW_TAG_variable
> > >  	  || abbrev->tag == DW_TAG_namespace
> > >  	  || part_die->is_declaration)
> > > @@ -16197,7 +16273,9 @@ load_partial_dies (const struct
> die_reader_specs
> > *reader,
> > >  		      || last_die->tag == DW_TAG_union_type))
> > >  	      || (cu->language == language_ada
> > >  		  && (last_die->tag == DW_TAG_subprogram
> > > -		      || last_die->tag == DW_TAG_lexical_block))))
> > > +		      || last_die->tag == DW_TAG_lexical_block))
> > > +	      || (cu->language == language_fortran
> > > +		  && last_die->tag == DW_TAG_subprogram)))
> >
> > Likewise, update comments above.  Do we need to check both
> > DW_TAG_subprogram and DW_TAG_entry_point?
> 
> Changed locally.
> 
> > >  	{
> > >  	  nesting_level++;
> > >  	  parent_die = last_die;
> > > @@ -19058,6 +19136,25 @@ new_symbol_full (struct die_info *die, struct
> > type *type, struct dwarf2_cu *cu,
> > >  	  SYMBOL_ACLASS_INDEX (sym) = LOC_LABEL;
> > >  	  add_symbol_to_list (sym, cu->list_in_scope);
> > >  	  break;
> > > +	case DW_TAG_entry_point:
> > > +	  /* Don't know any other language than fortran which is
> > > +	     using DW_TAG_entry_point.  */
> > > +	  if (cu->language == language_fortran)
> > > +	    {
> > > +	      /* SYMBOL_BLOCK_VALUE (sym) will be filled in later by
> > > +	      	 finish_block.  */
> > > +	      SYMBOL_ACLASS_INDEX (sym) = LOC_BLOCK;
> > > +	      /* DW_TAG_entry_point provides an additional entry_point to an
> > > +		 existing sub_program.  Therefore, we inherit the "external"
> > > +		 attribute from the sub_program to which the entry_point
> > > +		 belongs to.  */
> > > +	      attr2 = dwarf2_attr (die->parent, DW_AT_external, cu);
> > > +	      if (attr2 && (DW_UNSND (attr2) != 0))
> >
> > if (attr2 != NULL && DW_UNSND (attr2) != 0)
> 
> Changed locally.
> 
> > > +		list_to_add = &global_symbols;
> > > +	      else
> > > +		list_to_add = cu->list_in_scope;
> > > +	    }
> > > +	  break;
> > >  	case DW_TAG_subprogram:
> >
> >
> > Can we merge to case block for DW_TAG_subprogram and DW_TAG_subprogram?
> > They are quite similar.
> 
> Merged locally.
> >
> > --
> > Yao (齐尧)
> 
> Regards,
> Tim
> Intel Deutschland GmbH
> Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
> Tel: +49 89 99 8853-0, www.intel.de
> Managing Directors: Christin Eisenschmid, Christian Lamprechter
> Chairperson of the Supervisory Board: Nicole Lau
> Registered Office: Munich
> Commercial Register: Amtsgericht Muenchen HRB 186928
Intel Deutschland GmbH
Registered Address: Am Campeon 10-12, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Christin Eisenschmid, Christian Lamprechter
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928