Re: [debug-early] emit locals early patchset

[Aldy Hernandez <aldyh at redhat dot com>]

On 10/28/14 10:28, Jason Merrill wrote:

My apologies for the long delay.  I was on PTO.

> On 10/27/2014 08:00 PM, Aldy Hernandez wrote:
> > 2. Changes to gen_variable_die() to handle multiple passes (early/late
> > dwarf generation).
> >
> > A lot of this is complicated by the fact that old_die's are cached and
> > keyed by `tree', but an abstract instance and an inline instance share
> > trees, while dwarf2out_abstract_function() sets DECL_ABSTRACT_P behind
> > the scenes.
> >
> > The current support (and my changes) maintain this shared and delicate
> > design.  I wonder whether we could simplify a lot of this code by
> > unsharing these trees, but this may be beyond the scope of this work.
>
> Copying all the trees in a function just for debug generation?  No, that
> sounds undesirable.
>
> > 3. I've removed deferred_locations.  With multiple dwarf passes we can
> > do without them.
>
> Yay!
>
> > Kind words greatly appreciated.  Basically I'm looking for feedback
> > and positive reinforcement that this is all eventually useful
>
> This all looks very good, just a few nitpicks:

Yay!

> > -     instance tree [that has DW_AT_inline] should not contain any
> > +     instance tree [has DW_AT_inline] should not contain any
>
> This doesn't seem like an improvement.

Reverted.

> > +      /* Find and reuse a previously generated DW_TAG_subrange_type if
> > +     available.  */
>
> Let's expand this comment a bit to clarify how this works for
> multi-dimensional arrays.

Done.

> > -     abstract instance (origin != NULL), in which case we need a new
> > +     inline instance (origin != NULL), in which case we need a new DIE
>
> I think "concrete instance" is what you want here.

Done.

> > +          /* Even if we have locations, we need to recurse through
> > +         the locals to make sure they also have locations.  */
>
> Why?  What is adding a location to the function without doing the same
> for the locals?

Apparently, nothing.  I put a gcc_unreachable() there, and in all my 
tests, it never got triggered, so I've removed it.  Thanks.

> > +      current_function_has_inlines = 0;
> > +
> > +      /* The first time through decls_for_scope we will generate the
> > +     DIEs for the locals.  The second time, we fill in the
> > +     location info.  */
> > +      decls_for_scope (outer_scope, subr_die, 0);
> > +
> >        /* Emit a DW_TAG_variable DIE for a named return value.  */
> >        if (DECL_NAME (DECL_RESULT (decl)))
> >      gen_decl_die (DECL_RESULT (decl), NULL, subr_die);
> >
> > -      current_function_has_inlines = 0;
> > -      decls_for_scope (outer_scope, subr_die, 0);
>
> Why does this need to be reordered?

This may have been fall back from a previous version.  I have reverted 
the change.

> > +  /* If the compiler emitted a definition for the DECL declaration
> > +     and we already emitted a DIE for it, don't emit a second
> > +     DIE for it again. Allow re-declarations of DECLs that are
> > +     inside functions, though.  */
> > +  else if (old_die && !declaration && !local_scope_p (context_die))
> > +    return;
>
> What DECLs in functions need re-declaration?

This was already there.  It is pre-existing code that got moved down 
after the new caching code.

> > -  if (decl && (DECL_ABSTRACT_P (decl) || declaration || old_die ==
> > NULL))
> > +  if (decl && (DECL_ABSTRACT_P (decl) || declaration || old_die == NULL
> > +           /* If we make it to a specialization, we have already
> > +          handled the declaration by virtue of early dwarf.
> > +          If so, make a new assocation if available, so late
> > +          dwarf can find it.  */
> > +           || (specialization_p && old_die && old_die->dumped_early)))
> >      equate_decl_number_to_die (decl, var_die);
>
> Instead of old_die->dumped_early, I think it would make more sense to
> check early_dwarf_dumping; the reason we need to call
> equate_decl_number_to_die is because we're early-dumping the definition
> and we will need to find it again later.

I've rewritten the above as:

	       || (specialization_p && early_dwarf_dumping)))

> > +  else if (BLOCK_ABSTRACT_ORIGIN (stmt))
> >      {
> > +      /* If this is an inlined instance, create a new lexical die for
> > +     anything below to attach DW_AT_abstract_origin to.  */
> > +      stmt_die = new_die (DW_TAG_lexical_block, context_die, stmt);
> > +    }
>
> What if we early dumped this block?

What do you mean?  Would you like me to calls decls_for_scope earlier 
for abstract instances, or generate the DW_TAG_lexical_block die earlier 
for abstract instances, or what?

> > +      /* Variabled-lengthed types may be incomplete even if
> > +     TREE_ASM_WRITTEN.
>
> "variable-length", I think.

Fixed in the changelog and otherwise in the patch.

I have committed the attached patch.  We can iterate on the 
DW_TAG_lexical_block and DECL re-declaration issues in subsequent followups.

As usual, feel free to scream 
(https://www.youtube.com/watch?v=HLI4EuDckgM) if in violent disagreement.

Aldy

Attachment: curr
Description: Text document