[PATCH] C++/DWARF : Add 'using' support - take 2

[Devang Patel]

On Jan 13, 2004, at 1:06 PM, Jason Merrill wrote:
>
>>   gen_type_die_for_member (tree type, tree member, dw_die_ref 
>> context_die)
>>   {
>> +   dw_die_ref tmp_die;
>> *** 10549,10555 ****
>> ! 	gen_subprogram_die (member, lookup_type_die (type));
>> --- 10557,10563 ----
>> ! 	tmp_die = gen_subprogram_die (member, lookup_type_die (type));
>
> These hunks seem useless; you aren't using tmp_die anywhere.  You 
> really
> ought to review your own patches for this sort of thing before you ask
> someone else to review them.

I intentionally added it to note that now gen_subprogram_die returns 
new die.
If you want I will remove it.

>> ! static void
>> --- 10609,10620 ----
>> ! static dw_die_ref
>>   gen_subprogram_die (tree decl, dw_die_ref context_die)
>> +
>> *** 10657,10663 ****
>> ! 	    return;
>> --- 10683,10689 ----
>> ! 	    return NULL;
>> --- 10901,10907 ----
>> +   return subr_die;
>
> I'd prefer to do without all the changes to make gen_subprogram_die 
> return
> a value.  I don't see a good reason to break the convention of 
> "generate,
> then look up" for just this one variety of DIE.

We do not equate abstract dies with decl number so we can not do look up
for it. As per your suggestion, force_decl_die() now forces die for
abstract inline function. Without return value I do not have any way
to refer this forced die.

>
>> *************** gen_subprogram_die (tree decl, dw_die_re
>> *** 10620,10625 ****
>> --- 10628,10651 ----
>>        from the member list for the class.  If so, DECLARATION takes 
>> priority;
>>        we'll get back to the abstract instance when done with the 
>> class.  */
>>
>> +   if (old_die
>> +       && !get_AT_flag (old_die, DW_AT_declaration)
>> +       && !get_AT (old_die, DW_AT_inline))
>> +     {
>> +       unsigned file_index = lookup_filename (DECL_SOURCE_FILE 
>> (decl));
>> +       unsigned line_number = DECL_SOURCE_LINE (decl);
>> +
>> +       if (get_AT_unsigned (old_die, DW_AT_decl_file) == file_index
>> + 	  && get_AT_unsigned (old_die, DW_AT_decl_line) == line_number)
>> + 	{
>> + 	  /* This die is half backed die for this function, created earlier
>> + 	     by force_decl_die(). Complete it now. Use this die as subr_die
>> + 	     and reset old_die and continue.  */
>> + 	  subr_die = old_die;
>> + 	  old_die = NULL;
>> + 	}
>> +     }
>
> You should handle this in the "else if (old_die)" block below, which 
> also
> deals with reusing a die.

It handles reusing decl die as definition die. We need to complete this
die which is the else section after "else if (old_die)".

>
>> *************** static void
>> *** 10882,10893 ****
>>   gen_variable_die (tree decl, dw_die_ref context_die)
>>   {
>>     tree origin = decl_ultimate_origin (decl);
>> !   dw_die_ref var_die = new_die (DW_TAG_variable, context_die, decl);
>>
>>     dw_die_ref old_die = lookup_decl_die (decl);
>>     int declaration = (DECL_EXTERNAL (decl)
>>   		     || class_or_namespace_scope_p (context_die));
>>
>>     if (origin != NULL)
>>       add_abstract_origin_attribute (var_die, origin);
>>
>> --- 10910,10929 ----
>>   gen_variable_die (tree decl, dw_die_ref context_die)
>>   {
>>     tree origin = decl_ultimate_origin (decl);
>> !   dw_die_ref var_die;
>>
>>     dw_die_ref old_die = lookup_decl_die (decl);
>>     int declaration = (DECL_EXTERNAL (decl)
>>   		     || class_or_namespace_scope_p (context_die));
>>
>> +   if (old_die
>> +       && get_AT_flag (old_die, DW_AT_declaration) == 1
>> +       && old_die->die_definition)
>> +     /* We have seen this variable. It may have been forced out 
>> earlier
>> +        during force_decl_de().  */
>> +     return;
>> +
>> +   var_die = new_die (DW_TAG_variable, context_die, decl);
>>     if (origin != NULL)
>>       add_abstract_origin_attribute (var_die, origin);
>>
>
> I don't see how checking die_definition makes any sense here.  What
> situation is this code trying to handle?

If variable for forced out earlier than later on add_AT_specification()
aborts because die_definition is non-zero.

>> ! force_decl_die (tree decl)
>> ! {
>> ...
>> ! 	  else if (DECL_INLINE (decl))
>> ! 	    /* Force die to represent this function as abstract
>> ! 	       function for our reference.  */
>> ! 	      dwarf2out_abstract_function (decl);
>
> I realize that I suggested this in my earlier review, but I think we
> shouldn't handle inline functions differently here.  If all we care 
> about
> is having a DIE to refer to, we don't need to emit the whole abstract
> function.

so instead what do you suggest here?

>  (btw, checking DECL_INLINE is the wrong way to test this)

why?

>> ! 	case FIELD_DECL:
>> ! 	case VAR_DECL:
>> ! 	  gen_decl_die (decl, context_die);
>> ! 	  break;
>> ! 	default:
>> ! 	  dwarf2out_decl (decl);
>> ! 	  break;
>
> What other decls are likely to come up?  Why is dwarf2out_decl the 
> right
> thing for them?

I do not remember. May be it is here because force_decl_die() is a
replacement for original dwarf2out_decl() call I had. I'll test again
by adding abort() for other decls.

>> ! 	}
>> !
>> !       /* See if we can find the die for this deci now.
>> ! 	 If not then abort.  */
>> !       if (!decl_die)
>
> This test can go away if we lose the gen_subprogram_die return value.
>
>>   gen_decl_die (tree decl, dw_die_ref context_die)
>>   {
>> +   dw_die_ref tmp_die;
>> *** 11979,11985 ****
>> !       gen_subprogram_die (decl, context_die);
>> --- 12083,12089 ----
>> !       tmp_die = gen_subprogram_die (decl, context_die);
>
> Again, no use setting a variable you never check.
>
>> *************** do_class_using_decl (tree decl)
>> *** 2839,2844 ****
>> --- 2844,2865 ----
>>     type = dependent_type_p (scope) ? NULL_TREE : void_type_node;
>>     value = build_lang_decl (USING_DECL, name, type);
>>     DECL_INITIAL (value) = scope;
>> +
>> +   /* FIXME: Handle TEMPLATE_TYPE_PARM, TYPENAME_TYPE debug info.  */
>> +   if (scope
>> +       && TREE_CODE (scope) != TEMPLATE_TYPE_PARM
>> +       && TREE_CODE (scope) != TYPENAME_TYPE)
>> +     {
>> +       tree r;
>> +
>> +       r = lookup_qualified_name (scope, name, false, false);
>> +       if (r && TREE_CODE (r) != ERROR_MARK)
>> + 	cp_emit_debug_info_for_using (r, scope);
>> +       else
>> + 	{
>> + 	  /* FIXME: Find way to look up templates.  */
>> + 	}
>> +     }
>>     return value;
>>   }
>
> You should never encounter a TEMPLATE_TYPE_PARM or TYPENAME_TYPE when
> emitting debug information; that would suggest that you're trying to 
> emit
> debug info for templates themselves.  You should only be emitting debug
> info for template instantiation.  Perhaps you need to disable some of 
> your
> code 'if (processing_template_decl)'.

OK.

>
>> *************** do_using_directive (tree namespace)
>> *** 3300,3305 ****
>> --- 3330,3342 ----
>>     else
>>       /* direct usage */
>>       add_using_namespace (current_namespace, namespace, 0);
>> +
>> +   /* Emit debugging info.  */
>> +   if (current_namespace != global_namespace)
>> +     context = current_namespace;
>
> The if seems redundant with the code at the top of
> cp_emit_debug_info_for_using.

Yes, I can directly use current_namespace here.

>  Also, this is wrong for function local
> using-directives; they need to be in current_scope(), not
> current_namespace.

I use current_scope() in do_local_using_decl().

>> + /* Emit debugging information for using declarations and directives.
>> +    If input tree is overloaded fn then emit debug info for all
>> +    candidates.  */
>> +
>> + static void
>> + cp_emit_debug_info_for_using (tree t, tree context)
>> + {
>> +   /* Do not supply context to imported_module_or_decl, if
>> +      it is a global namespace.  */
>> +   if (context != NULL_TREE
>> +       && TREE_CODE (context) == NAMESPACE_DECL
>> +       && current_namespace == global_namespace)
>> +     context = NULL_TREE;
>
> I don't follow this logic.  Why not just
>
>   if (context == global_namespace)

You're right here :-)

Thank you,
--
Devang