Re: [PATCH] Support DWARF tags for C++11 variadic templates

[Bruno Larsen <blarsen@redhat.com>]

On 04/02/2023 17:23, Ed Catmur wrote:
> This patch adds support for DW_TAG_GNU_formal_parameter_pack and
> DW_TAG_GNU_template_parameter_pack, added to DWARF in March 2009 for
> C++11 variadic templates[1].
>
> They are not currently emitted thanks to a typo[2] but the fix is
> trivial[3] and has been repeatedly submitted[4] to gcc; I'm not sure
> what else I can do to get it accepted; regardless, anyone building their
> own compiler can still make use of this.
>
> This implementation synthesizes type and parameter names as T#n, p#n
> e.g. Args#1, args#1. This is a pretty simple approach but it seems to
> work OK and is compatible with the old style type and parameter names
> emitted by old versions of gcc and when it's writing stabs+ format,
> meaning that any debugger scripts will continue to work.
>
> 1. http://wiki.dwarfstd.org/index.php?title=C%2B%2B0x:_Variadic_templates
> 2. https://gcc.gnu.org/bugzilla/show_bug.cgi?id=70536
> 3. https://github.com/ecatmur/gcc/pull/5
> 4. https://gcc.gnu.org/pipermail/gcc-patches/2023-January/609659.html
> PR. https://sourceware.org/bugzilla/show_bug.cgi?id=17272
> ---

Hello Ed,

Thanks for working on this!

It would be nice if you could provide a testcase so we can validate the 
solution even if we don't have an up-to-date compiler. There is a 
convenient way to generate custom dwarf, using the "dwarf assembler" in 
the GDB testsuite, but I don't think it supports 
DW_TAG_GNU_formal_parameter_pack, so it might be easier to make a custom 
assembly with it.

The patch also needs rebasing for proper regression testing, and I have 
some style comments inlined.

>   gdb/dwarf2/read.c | 71 ++++++++++++++++++++++++++++++++++++++++++++---
>   1 file changed, 67 insertions(+), 4 deletions(-)
>
> diff --git a/gdb/dwarf2/read.c b/gdb/dwarf2/read.c
> index ee4e7c1530..7591be5764 100644
> --- a/gdb/dwarf2/read.c
> +++ b/gdb/dwarf2/read.c
> @@ -12103,7 +12103,8 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>     for (child_die = die->child; child_die; child_die = child_die->sibling)
>       {
>         if (child_die->tag == DW_TAG_template_type_param
> -	  || child_die->tag == DW_TAG_template_value_param)
> +	  || child_die->tag == DW_TAG_template_value_param
> +	  || child_die->tag == DW_TAG_GNU_template_parameter_pack)
>   	{
>   	  templ_func = new (&objfile->objfile_obstack) template_symbol;
>   	  templ_func->subclass = SYMBOL_TEMPLATE;
> @@ -12152,6 +12153,23 @@ read_func_scope (struct die_info *die, struct dwarf2_cu *cu)
>   	      if (arg != NULL)
>   		template_args.push_back (arg);
>   	    }
> +	  else if (child_die->tag == DW_TAG_GNU_template_parameter_pack)
> +	    {
> +	      struct die_info *pack_die;
> +	      for (pack_die = child_die->child; pack_die; pack_die = pack_die->sibling)
When testing a pointer for null, you should always be explicit, using 
nullptr. Also, this like needs to be wrapped
> +		{
> +		  struct symbol *arg = new_symbol (pack_die, NULL, cu);
> +
> +		  if (arg != NULL)
Same here, should be (arg != nullptr)
> +		    template_args.push_back (arg);
> +		}
> +	    }
> +	  else if (child_die->tag == DW_TAG_GNU_formal_parameter_pack)
> +	    {
> +	      struct die_info *pack_die;
> +	      for (pack_die = child_die->child; pack_die; pack_die = pack_die->sibling)
ditto
> +		process_die (pack_die, cu);
> +	    }
>   	  else
>   	    process_die (child_die, cu);
>   	  child_die = child_die->sibling;
> @@ -16639,6 +16657,11 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
>   	{
>   	  if (child_die->tag == DW_TAG_formal_parameter)
>   	    nparams++;
> +	  else if (child_die->tag == DW_TAG_GNU_formal_parameter_pack)
> +	    {
> +	      child_die = child_die->child;
> +	      continue;
> +	    }

Shouldn't this block also set ftype->has_varargs to true?

Also, this could fail if there could be DW_TAG_formal_parameters after 
the GNU version, which is mentioend at the end of the dwarf wiki you 
linked. I think there should be a depth variable that knows if we should 
go back up to the parent die once we reach the final sibling.

And there is a FIXME comment right before this loop that mentions that 
GDB can't handle vararg functions that can probably be updated with this 
patch.

>   	  else if (child_die->tag == DW_TAG_unspecified_parameters)
>   	    ftype->set_has_varargs (true);
>   
> @@ -16714,6 +16737,11 @@ read_subroutine_type (struct die_info *die, struct dwarf2_cu *cu)
>   	      ftype->field (iparams).set_type (arg_type);
>   	      iparams++;
>   	    }
> +	  else if (child_die->tag == DW_TAG_GNU_formal_parameter_pack)
> +	    {
> +	      child_die = child_die->child;
> +	      continue;
> +	    }
Same here for being able to return to parent DIE.
>   	  child_die = child_die->sibling;
>   	}
>       }
> @@ -20847,13 +20875,16 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>   	sym->set_type (type);
>         else
>   	sym->set_type (die_type (die, cu));
> -      attr = dwarf2_attr (die,
> +      struct die_info *line_file_die = die;
> +      if (die->tag == DW_TAG_formal_parameter && die->parent && die->parent->tag == DW_TAG_GNU_formal_parameter_pack)
explicit nullptr check again and wrapping to:

if (die->tag == DW_TAG_formal_parameter
     && die->parent == nullpre
     && die->parent->tag == DW_TAG_GNU_formal_parameter_pack)

> +	line_file_die = die->parent;
> +      attr = dwarf2_attr (line_file_die,
>   			  inlined_func ? DW_AT_call_line : DW_AT_decl_line,
>   			  cu);
>         if (attr != nullptr)
>   	sym->set_line (attr->constant_value (0));
>   
> -      attr = dwarf2_attr (die,
> +      attr = dwarf2_attr (line_file_die,
>   			  inlined_func ? DW_AT_call_file : DW_AT_decl_file,
>   			  cu);
>         if (attr != nullptr && attr->is_nonnegative ())
> @@ -21073,6 +21104,8 @@ new_symbol (struct die_info *die, struct type *type, struct dwarf2_cu *cu,
>   	    list_to_add = cu->list_in_scope;
>   	  }
>   	  break;
> +	case DW_TAG_GNU_formal_parameter_pack:
> +	  break;
>   	case DW_TAG_unspecified_parameters:
>   	  /* From varargs functions; gdb doesn't seem to have any
>   	     interest in this information, so just ignore it for now.
> @@ -22081,8 +22114,11 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
>     if (attr_name == nullptr
>         && die->tag != DW_TAG_namespace
>         && die->tag != DW_TAG_class_type
> +      && die->tag != DW_TAG_formal_parameter
>         && die->tag != DW_TAG_interface_type
>         && die->tag != DW_TAG_structure_type
> +      && die->tag != DW_TAG_template_type_param
> +      && die->tag != DW_TAG_template_value_param
>         && die->tag != DW_TAG_namelist
>         && die->tag != DW_TAG_union_type
>         && die->tag != DW_TAG_template_type_param
> @@ -22111,9 +22147,36 @@ dwarf2_name (struct die_info *die, struct dwarf2_cu *cu)
>   	return attr_name;
>         return CP_ANONYMOUS_NAMESPACE_STR;
>   
> -    /* DWARF does not actually require template tags to have a name.  */
> +    case DW_TAG_formal_parameter:
>       case DW_TAG_template_type_param:
>       case DW_TAG_template_value_param:
> +      if (!attr
> +	  && die->parent
attr == nullptr && diw->parent != nullptr
> +	  && (die->parent->tag == DW_TAG_GNU_formal_parameter_pack
> +	      || die->parent->tag == DW_TAG_GNU_template_parameter_pack))
> +	{
> +	  const char *parent_name;
> +	  int ordinal = 0;
> +	  struct die_info *child_die;
> +	  size_t size;
> +	  char *name;
> +	  parent_name = dwarf2_name(die->parent, cu);
> +	  if (!parent_name)
> +	    return NULL;
When GDB finds a template parameter that doesn't have a name, it instead 
returns <unnamed N> to help users understand what's going on (see: 
https://sourceware.org/pipermail/gdb-patches/2022-August/191528.html). 
If this behavior is changed for GNU tags, it will look like a bug to 
users, it would be better to continue calling unnamed_template_tag_name 
in this exit.

-- 
Cheers,
Bruno

> +	  for (child_die = die->parent->child; child_die != die; child_die = child_die->sibling)
> +	    ++ordinal;
> +	  size = snprintf(NULL, 0, "%s#%d", parent_name, ordinal) + 1;
> +	  name = ((char *) obstack_alloc (&cu->per_objfile->per_bfd->obstack, size));
> +	  snprintf(name, size, "%s#%d", parent_name, ordinal);
> +	  return name;
> +	}
> +      if (die->tag == DW_TAG_formal_parameter)
> +	{
> +	  if (!attr || attr_name == NULL)
> +	    return NULL;
> +	  break;
> +	}
> +      /* DWARF does not actually require template tags to have a name.  */
>         if (attr_name == nullptr)
>   	return unnamed_template_tag_name (die, cu);
>         /* FALLTHROUGH.  */