This is the mail archive of the gcc-patches@gcc.gnu.org mailing list for the GCC project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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



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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]