From patchwork Fri Dec 5 23:08:31 2014 Content-Type: text/plain; charset="utf-8" MIME-Version: 1.0 Content-Transfer-Encoding: 7bit X-Patchwork-Submitter: Aldy Hernandez X-Patchwork-Id: 418320 Return-Path: X-Original-To: incoming@patchwork.ozlabs.org Delivered-To: patchwork-incoming@bilbo.ozlabs.org Received: from sourceware.org (server1.sourceware.org [209.132.180.131]) (using TLSv1.2 with cipher ECDHE-RSA-AES256-GCM-SHA384 (256/256 bits)) (No client certificate requested) by ozlabs.org (Postfix) with ESMTPS id EED1D140119 for ; Sat, 6 Dec 2014 10:08:45 +1100 (AEDT) DomainKey-Signature: a=rsa-sha1; c=nofws; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; q=dns; s=default; b=UGyelZTA6zAYk+12J aK05Bih+km4Uf2JryN3vJ3oxcLnI28S9Hj9T3PwH6Z+vf3wW0MTwDAIQJNTE81HF JM9eo93bcKT/sHjwCuyz7/La2Zw6qUX+MgJYvobjxVXZ47hfz0mm3MkjtQz02Nye uoPmVU877RRgd0ZpphTQbqZbBU= DKIM-Signature: v=1; a=rsa-sha1; c=relaxed; d=gcc.gnu.org; h=list-id :list-unsubscribe:list-archive:list-post:list-help:sender :message-id:date:from:mime-version:to:cc:subject:references :in-reply-to:content-type; s=default; bh=xPa+hikhCStD8I0nmoom/OX VXMM=; b=lKyvm0PJyg3wJbX7wqu2gzNa9e/WBvZF76/Z/EAZ9MNkJNCbzRTBooW BD1Q+oKXegQaLD2D9E2BYMjrpra+IKOjrWY8z9qcESo8RXY3WXylCF53YonHx91O OGH9jo6nNw1Om5QFzPG+RhR3gL9XJT54PZ30Ulfs25MKZ1xiPK/g= Received: (qmail 6716 invoked by alias); 5 Dec 2014 23:08:39 -0000 Mailing-List: contact gcc-patches-help@gcc.gnu.org; run by ezmlm Precedence: bulk List-Id: List-Unsubscribe: List-Archive: List-Post: List-Help: Sender: gcc-patches-owner@gcc.gnu.org Delivered-To: mailing list gcc-patches@gcc.gnu.org Received: (qmail 6704 invoked by uid 89); 5 Dec 2014 23:08:38 -0000 Authentication-Results: sourceware.org; auth=none X-Virus-Found: No X-Spam-SWARE-Status: No, score=-1.6 required=5.0 tests=AWL, BAYES_00, SPF_HELO_PASS, SPF_PASS, T_RP_MATCHES_RCVD autolearn=ham version=3.3.2 X-HELO: mx1.redhat.com Received: from mx1.redhat.com (HELO mx1.redhat.com) (209.132.183.28) by sourceware.org (qpsmtpd/0.93/v0.84-503-g423c35a) with (AES256-GCM-SHA384 encrypted) ESMTPS; Fri, 05 Dec 2014 23:08:36 +0000 Received: from int-mx10.intmail.prod.int.phx2.redhat.com (int-mx10.intmail.prod.int.phx2.redhat.com [10.5.11.23]) by mx1.redhat.com (8.14.4/8.14.4) with ESMTP id sB5N8Y8Z011744 (version=TLSv1/SSLv3 cipher=DHE-RSA-AES256-GCM-SHA384 bits=256 verify=FAIL); Fri, 5 Dec 2014 18:08:34 -0500 Received: from reynosa.quesejoda.com (vpn-57-225.rdu2.redhat.com [10.10.57.225]) by int-mx10.intmail.prod.int.phx2.redhat.com (8.14.4/8.14.4) with ESMTP id sB5N8WTT025816; Fri, 5 Dec 2014 18:08:33 -0500 Message-ID: <54823AEF.7090104@redhat.com> Date: Fri, 05 Dec 2014 15:08:31 -0800 From: Aldy Hernandez User-Agent: Mozilla/5.0 (X11; Linux x86_64; rv:31.0) Gecko/20100101 Thunderbird/31.1.1 MIME-Version: 1.0 To: Jason Merrill , Richard Biener CC: gcc-patches Subject: Re: [debug-early] emit locals early patchset References: <544EDCA8.6070001@redhat.com> <544FD24C.1080203@redhat.com> In-Reply-To: <544FD24C.1080203@redhat.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 commit 7d0ab897d086ac8648928b1236dc88697c96d037 Author: Aldy Hernandez Date: Fri Dec 5 14:54:41 2014 -0800 * dwarf2out.c (check_die_inline): Revert previous comment change. (add_subscript_info): Comment better. (gen_subprogram_die): Do not try to generate local locations if we already have locations as a whole. Remove recurse_through_locals label. Revert reordering of decls_for_scope call. (gen_variable_die): Check early_dwarf_dumping when testing for specialization_p instead of checking old_die->dumped_early. diff --git a/gcc/dwarf2out.c b/gcc/dwarf2out.c index 9cb4ace..3528083 100644 --- a/gcc/dwarf2out.c +++ b/gcc/dwarf2out.c @@ -5558,7 +5558,7 @@ static void check_die_inline (dw_die_ref die) { /* A debugging information entry that is a member of an abstract - instance tree [has DW_AT_inline] should not contain any + instance tree [that has DW_AT_inline] should not contain any attributes which describe aspects of the subroutine which vary between distinct inlined expansions or distinct out-of-line expansions. */ @@ -16637,7 +16637,16 @@ add_subscript_info (dw_die_ref type_die, tree type, bool collapse_p) here. */ /* Find and reuse a previously generated DW_TAG_subrange_type if - available. */ + available. + + For multi-dimensional arrays, as we iterate through the + various dimensions in the enclosing for loop above, we also + iterate through the DIE children and pick at each + DW_TAG_subrange_type previously generated (if available). + Each child DW_TAG_subrange_type DIE describes the range of + the current dimension. At this point we should have as many + DW_TAG_subrange_type's as we have dimensions in the + array. */ dw_die_ref subrange_die = NULL; if (child) while (1) @@ -17327,7 +17336,7 @@ gen_array_type_die (tree type, dw_die_ref context_die) bool collapse_nested_arrays = !is_ada (); - /* For variable-lengthed arrays that have been previously generated, + /* For variable-length arrays that have been previously generated, just fill in the possibly missing subscript info. */ if (TREE_ASM_WRITTEN (type) && TREE_CODE (type) == ARRAY_TYPE @@ -17818,8 +17827,8 @@ gen_formal_parameter_die (tree node, tree origin, bool emit_name_p, } /* If we have a previously generated DIE, use it, unless this is an - inline instance (origin != NULL), in which case we need a new DIE - with a corresponding DW_AT_abstract_origin. */ + concrete instance (origin != NULL), in which case we need a new + DIE with a corresponding DW_AT_abstract_origin. */ bool reusing_die; if (parm_die && origin == NULL) reusing_die = true; @@ -18418,14 +18427,7 @@ gen_subprogram_die (tree decl, dw_die_ref context_die) something we have already output. */ if (get_AT (old_die, DW_AT_low_pc) || get_AT (old_die, DW_AT_ranges)) - { - /* Even if we have locations, we need to recurse through - the locals to make sure they also have locations. */ - if (dumped_early) - goto recurse_through_locals; - - return; - } + return; /* If we have no location information, this must be a partially generated DIE from early dwarf generation. @@ -18835,7 +18837,6 @@ gen_subprogram_die (tree decl, dw_die_ref context_die) /* Add the calling convention attribute if requested. */ add_calling_convention_attribute (subr_die, decl); - recurse_through_locals: /* Output Dwarf info for all of the stuff within the body of the function (if it has one - it may be just a declaration). @@ -18859,6 +18860,10 @@ gen_subprogram_die (tree decl, dw_die_ref context_die) int call_site_note_count = 0; int tail_call_site_note_count = 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; /* The first time through decls_for_scope we will generate the @@ -18866,10 +18871,6 @@ gen_subprogram_die (tree decl, dw_die_ref context_die) 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); - if (call_arg_locations && !dwarf_strict) { struct call_arg_loc_node *ca_loc; @@ -19312,7 +19313,7 @@ gen_variable_die (tree decl, tree origin, dw_die_ref context_die) 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))) + || (specialization_p && early_dwarf_dumping))) equate_decl_number_to_die (decl, var_die); gen_variable_die_location: