Re: [RFA] Emit DWARF for template parameters (PR debug/30161)

[Dodji Seketeli <dodji at redhat dot com>]

Le 14/08/2009 16:22, Jason Merrill a écrit :
> On 08/13/2009 03:12 PM, Dodji Seketeli wrote:
>> +/* Return true if T is a primary function
>> +   template instantiation.  */
>> +
>> +bool
>> +decl_template_instantiation_p (const_tree t)
> 
> The name should have "primary" in it if that's a condition.

I renamed it into primary_template_instantiation_p and made it work for
function and class templates. I also moved the language hooks generic_p out
of the types members so that
it is accessed now by langhooks.generic_p and made it use this function.

> 
>> +    case DW_AT_GNU_name:
>> +      return "DW_AT_GNU_name";
> 
> This name isn't specific enough.  Perhaps DW_AT_GNU_template_name or
> _value_name or _name_value or _string_value or something.

I renamed it into DW_AT_GNU_template_name.

> 
>> +      || (!lang_hooks.types.generic_p (t)
>> +          && !lang_hooks.decls.generic_p (t)))
>> +    return;
>> +
>> +  if (TYPE_P (t))
>> +    die = lookup_type_die (t);
>> +  else if (DECL_P (t))
>> +    die = lookup_decl_die (t);
>> +
>> +  gcc_assert (die);
>> +
>> +  parms = lang_hooks.types.get_innermost_generic_parms (t);
> 
> Why have two different generic_p hooks, but only one get_parms hook?
> Probably better to have just one hook on both cases.  Or even do away
> with both generic_p hooks and just have the get_parms hook return null
> for non-generics.

Agreed. I did the later.

> 
>> +  for (i = 0; i < parms_num; i++)
>> +    {
>> +      tree args = lang_hooks.types.get_innermost_generic_args (t);
> 
> Why re-fetch the args each time we go through the loop?
> 

Oops. I moved the args fetching out of the loop.

>> +      if (lang_hooks.types.generic_type_argument_pack_p (arg)
>> +          || lang_hooks.types.generic_nontype_argument_pack_p (arg))
>> +        {
>> +          int i = 0, len = 0;
>> +          tree pack_elems =
>> +        lang_hooks.types.get_argument_pack_elems (arg);
> 
> Again, why two hooks to test but only one to fetch?  And again, I don't
> think we even need a separate test hook at all.  Just try to get the
> pack elements, and return null if it isn't a pack (as the hook
> implementation already does).

Yes. I just did that.

> 
>> +  else if (lang_hooks.decls.generic_type_parameter_decl_p (parm))
> 
> Why not just test TREE_CODE (parm) == TYPE_DECL?

Done.

> 
>> +  if (TREE_CODE (parm) == PARM_DECL || TREE_CODE (parm) == CONST_DECL)
>> +    /* PARM is a nontype generic parameter  */
> ...
>> +      if (TREE_CODE (parm) == PARM_DECL)
>> +    {
>> +      /* So PARM is a non-type generic parameter.
> 
> Why are these tests different?

Because I was confused. I changed that to if (TREE_CODE (parm) ==
PARM_DECL) in both places.

> 
>> -tree_add_const_value_attribute (dw_die_ref var_die, tree decl)
>> +tree_add_const_value_attribute (dw_die_ref var_die, tree t)
> 
> Currently this function takes as its second argument the decl whose
> constant value we want to add to the die (which is the die for that same
> decl).  You're changing it to either take that decl OR the constant
> value itself, which is going to cause trouble.  I think it would be
> better to change it to only take the constant value, and move the code
> for deciding which types of decls to bother with into a separate
> function like tree_add_const_value_attribute_for_decl.
> 
> Why do you need the other code changes?  Why can't you just use
> rtl_for_decl_init for template arguments, too?

Good question. When I first tried using rtl_for_decl_init, it wasn't
working. So I started playing around to understand how it should work for
things like pointers etc. Then I forgot to integrate those changes into
rtl_for_decl_init itself I guess. That'll teach me.

Now I looked at it more closely.

rtl_for_decl_init wasn't working on some expressions because it thought
those expressions where referencing unused decls. For instance, in the
template-params-3.C embedded in the patch, no rtl was generated for the
pointer to function 'bleh'.

I think the problem comes from reference_to_unused that was overly
conservative in which decl to consider as unused. I modified it so that it
doesn't consider a decl accessible from outside the current CU as being
unused. At least not when we don't have enough cgraph information to think
otherwise. I also reorganised the function to make sure we don't try to
access cgraph information before it is ready.

> 
>> +  /* Returns the name of the ith element of a generic parameter
>> pack.  */
>> +  tree (*make_ith_generic_pack_parm_name) (tree, int);
> 
> I don't think this needs to be a hook; there's no need for the front end
> internal name to match the name in the debug info.  In this case, i
> think it's better to duplicate the C++ front end function in dwarf2out.c.
> 

Done. I replaced the use of sprintf and strlen with snprintf and strnlen as
well.

>> -  /* True if the type is an instantiation of a generic type,
>> -     e.g. C++ template implicit specializations.  */
>> +  /* True if the parameter is an instantiation of a generic type,
>> +     e.g. C++ class template specializations.  */
> 
> The change to "parameter" is wrong.

True. I removed it.

>> +  /* Retuns the generic parameters of a generic declaration,
>> +     e.g. C++ template declaration.  */
>> +  tree (*get_innermost_generic_parms) (const_tree);
> 
> The argument isn't a template, it's an instantiation.
> 

Duh. Fixed.

>> +// We don't support debug info output for template template parameter
>> yet.
>> +// Look at the comment near the end of the test source code for more.
> 
> This comment seems out of date.  :)

Removed :)

Bootstrapped with enable-languages=all and tested on x86_64-unknown-linux-gnu.

Thanks.

-- 
Dodji Seketeli
Red Hat

Attachment: PR30161-patch-v12.txt
Description: Text document