On Mon, 7 Dec 2020, Bernd Edlinger wrote:
> On 12/7/20 11:50 AM, Richard Biener wrote:
> > The ipa-param-manipulation.c hunk is OK, please commit separately.
> >
>
> done.
>
> > The tree-inline.c and cfgexpand.c changes are OK as well, for the
> > tree-ssa-live.c change see below
> >
> >
> > From 85b0e37d0c0d3ecac4908ebbfd67edc612ef22b2 Mon Sep 17 00:00:00 2001
> > From: Bernd Edlinger <bernd.edlin...@hotmail.de>
> > Date: Wed, 2 Dec 2020 12:32:02 +0100
> > Subject: [PATCH] Remove misleading debug line entries
> >
> > This removes gimple_debug_begin_stmts without block info which remain
> > after a gimple block originating from an inline function is unused.
> >
> > The line numbers from these stmts are from the inline function,
> > but since the inline function is completely optimized away,
> > there will be no DW_TAG_inlined_subroutine so the debugger has
> > no callstack available at this point, and therefore those
> > line table entries are not helpful to the user.
> >
> > 2020-12-02 Bernd Edlinger <bernd.edlin...@hotmail.de>
> >
> > * cfgexpand.c (expand_gimple_basic_block): Remove special handling
> > of debug_inline_entries without block info.
> > * ipa-param-manipulation.c
> > (ipa_param_body_adjustments::modify_call_stmt): Set location info.
> > * tree-inline.c (remap_gimple_stmt): Drop debug_nonbind_markers when
> > the call statement has no block info.
> > (copy_debug_stmt): Remove debug_nonbind_markers when inlining
> > and the block info is mapped to NULL.
> > * tree-ssa-live.c (clear_unused_block_pointer): Remove
> > debug_nonbind_markers originating from inlined functions.
> > ---
> > gcc/cfgexpand.c | 9 +--------
> > gcc/ipa-param-manipulation.c | 2 ++
> > gcc/tree-inline.c | 14 ++++++++++----
> > gcc/tree-ssa-live.c | 22 ++++++++++++++++++++--
> > 4 files changed, 33 insertions(+), 14 deletions(-)
> >
> > diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c
> > index 7e0bdd58e85..df7b62080b6 100644
> > --- a/gcc/cfgexpand.c
> > +++ b/gcc/cfgexpand.c
> > @@ -5953,14 +5953,7 @@ expand_gimple_basic_block (basic_block bb, bool
> > disable_tail_calls)
> > else if (gimple_debug_begin_stmt_p (stmt))
> > val = GEN_RTX_DEBUG_MARKER_BEGIN_STMT_PAT ();
> > else if (gimple_debug_inline_entry_p (stmt))
> > - {
> > - tree block = gimple_block (stmt);
> > -
> > - if (block)
> > - val = GEN_RTX_DEBUG_MARKER_INLINE_ENTRY_PAT ();
> > - else
> > - goto delink_debug_stmt;
> > - }
> > + val = GEN_RTX_DEBUG_MARKER_INLINE_ENTRY_PAT ();
> > else
> > gcc_unreachable ();
> >
> > diff --git a/gcc/ipa-param-manipulation.c b/gcc/ipa-param-manipulation.c
> > index 2bbea21be2e..9ab4a10096d 100644
> > --- a/gcc/ipa-param-manipulation.c
> > +++ b/gcc/ipa-param-manipulation.c
> > @@ -1681,6 +1681,8 @@ ipa_param_body_adjustments::modify_call_stmt (gcall
> > **stmt_p)
> > }
> > }
> > gcall *new_stmt = gimple_build_call_vec (gimple_call_fn (stmt),
> > vargs);
> > + if (gimple_has_location (stmt))
> > + gimple_set_location (new_stmt, gimple_location (stmt));
> > gimple_call_set_chain (new_stmt, gimple_call_chain (stmt));
> > gimple_call_copy_flags (new_stmt, stmt);
> > if (tree lhs = gimple_call_lhs (stmt))
> > diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> > index d9814bd10d3..360b85f14dc 100644
> > --- a/gcc/tree-inline.c
> > +++ b/gcc/tree-inline.c
> > @@ -1819,12 +1819,11 @@ remap_gimple_stmt (gimple *stmt, copy_body_data *id)
> > /* If the inlined function has too many debug markers,
> > don't copy them. */
> > if (id->src_cfun->debug_marker_count
> > - > param_max_debug_marker_count)
> > + > param_max_debug_marker_count
> > + || id->reset_location)
> > return stmts;
> >
> > gdebug *copy = as_a <gdebug *> (gimple_copy (stmt));
> > - if (id->reset_location)
> > - gimple_set_location (copy, input_location);
> > id->debug_stmts.safe_push (copy);
> > gimple_seq_add_stmt (&stmts, copy);
> > return stmts;
> > @@ -3169,7 +3168,14 @@ copy_debug_stmt (gdebug *stmt, copy_body_data *id)
> > }
> >
> > if (gimple_debug_nonbind_marker_p (stmt))
> > - return;
> > + {
> > + if (id->call_stmt && !gimple_block (stmt))
> > + {
> > + gimple_stmt_iterator gsi = gsi_for_stmt (stmt);
> > + gsi_remove (&gsi, true);
> > + }
> > + return;
> > + }
> >
> > /* Remap all the operands in COPY. */
> > memset (&wi, 0, sizeof (wi));
> > diff --git a/gcc/tree-ssa-live.c b/gcc/tree-ssa-live.c
> > index 21a9ee43e6b..acba4a58626 100644
> > --- a/gcc/tree-ssa-live.c
> > +++ b/gcc/tree-ssa-live.c
> > @@ -623,13 +623,31 @@ clear_unused_block_pointer (void)
> > {
> > unsigned i;
> > tree b;
> > - gimple *stmt = gsi_stmt (gsi);
> > + gimple *stmt;
> >
> > + next:
> > + stmt = gsi_stmt (gsi);
> > if (!is_gimple_debug (stmt) && !gimple_clobber_p (stmt))
> > continue;
> > b = gimple_block (stmt);
> > if (b && !TREE_USED (b))
> > - gimple_set_block (stmt, NULL);
> > + {
> > + if (gimple_debug_nonbind_marker_p (stmt)
> > + && BLOCK_ABSTRACT_ORIGIN (b))
> >
> > so we have a non-bind marker that was inlined and its BLOCK is
> > elided.
> >
> > + {
> > + while (TREE_CODE (b) == BLOCK
> > + && !inlined_function_outer_scope_p (b))
> > + b = BLOCK_SUPERCONTEXT (b);
> >
> > we're looking for the inlined_function_outer_scope_p containing it
> > and if we found one we remove the stmt instead of setting its
> > block to zero.
> >
> > I'm still confused a bit here - a comment explaining what we do
> > here would be much appreciated. Note my earlier comment about
>
> I added a comment now, but I am not sure if it states just
> the obvious, or if it needs further improvement.
>
> ... and while I was there, I realized that
> I handle the debug_begin_stmt_p could be more conservative.
>
> That is, if only a lexical block in an inlined subroutine is
> elided - which is possible - there is no need to zap the
> line entirely, just assign the inlined function's outer scope,
> which is similar to what happens for debug_begin_stmt_p outside
> of inline functions, where NULL is assigned as block.
>
> > this hunk derailed into comments about how to handle the
> > tree-inline.c case which might have confused you (but I think
> > you found a nice solution there).
> >
> > That said, originally I wondered why not simply do the following,
> > that is - what is special about not inlined debug markers with
> > a NULL block?
> >
>
> actually the gimple_block of a gimple_debug_begin_stmt_p has no
> significance at all, it's just is a note, to which function the line number
> originally belongs to. BUT once we let it pass to the final stage of
> compilation, the line numbers from debug-begin-stmt and ordinary
> gimple_locations etc. from executable code line number do not have
> any block structure anymore. The block structure is an entirely
> different data structure, that consists of sets of byte-ranges
> where code for a certain lexical block or inline block is located.
> The line numbers from debug-begin-stmt are presented to the
> user when the program stops at the given byte address. But
> the when the block's debug_inline_entry_p is removed, for debug_inline_entry_p
> it is the same, if it gets the block_info set to NULL or if it is actually
> removed. But for debug_begin_stmt_p setting the block-info to NULL makes them
> ordinary line numbers from the global scope.
> If the debug_inline_entry_p is removed, not even an empty block range is left
> in the resulting dwarf debug info, and in fact the complete
> DW_TAG_inlined_subroutine
> debug entry is removed, and now the line entry from the removed subroutine
> block
> looks like it is from the outer scope now. Totally confusing when you step
> into this line.
>
>
> Attached is the new version of the patch, with the mentioned
> improvement and a short comment at the clear_unused_block_pointer
> function.
>
>
> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
+ {
+ /* Remove a nonbind marker when the outer scope of the
+ inline function is completely removed. */
+ if (gimple_debug_nonbind_marker_p (stmt)
+ && BLOCK_ABSTRACT_ORIGIN (b))
+ {
+ while (TREE_CODE (b) == BLOCK
+ && !inlined_function_outer_scope_p (b))
+ b = BLOCK_SUPERCONTEXT (b);
So given we never remove a inlined_function_outer_scope_p BLOCK from
the block tree can we assert that we find such a BLOCK? If we never
elide those BLOCKs how can it happen that we elide it in the end?
+ if (TREE_CODE (b) == BLOCK)
+ {
+ if (TREE_USED (b))
Iff we ever elide such block then this will still never be
false since we've otherwise removed the BLOCK from the block
tree already when we arrive here. Iff we did that, then the
above search for a inlined_function_outer_scope_p BLOCK
might find the "wrong" inline instance.
So I think we only eliminate the inline scopes if all
subblocks are unused but then if there's a used outer
inline instance your patch will assign that outer inline
instances BLOCK to debug stmts formerly belonging to the
elided inline instance, no? (we also mess with
BLOCK_SUPERCONTEXT during BLOCK removal so I'm not sure
the walking works as expected)
I guess we could, during the remove_unused_scope_block_p
DFS walk, record and pass down the "current"
inlined_function_outer_scope_p BLOCK and for BLOCKs
we elide record that somehow? (since we elide the block
we could abuse fragment_origin/chain for this)
Then in the patched loop do
if (b && !TREE_USED (b))
{
if (BLOCK_ABSTRACT_ORIGIN (b))
{
if (!TREE_USED (BLOCK_FRAGMENT_CHAIN (b))
{
/* inline instance removed */
gsi_remove ();
}
else
{
/* inline instance kept, use it */
gimple_set_block (stmt, BLOCK_FRAGMENT_CHAIN (b));
}
}
else
gimple_set_block (stmt, NULL);
}
? Not sure what happens if the inline instance ends up split into
a hot/cold part - then the "punning" of the BLOCK of the stmt to
the outermost scope could cause gdb to set bogus breakpoints?
Thanks,
Richard.
+ {
+ gimple_set_block (stmt, b);
+ continue;
+ }
+ gsi_remove (&gsi, true);
+ if (gsi_end_p (gsi))
+ break;
+ goto next;
>
> Thanks
> Bernd.
>
> > Richard.
> >
> > diff --git a/gcc/tree-ssa-live.c b/gcc/tree-ssa-live.c
> > index 21a9ee43e6b..e9633da26ba 100644
> > --- a/gcc/tree-ssa-live.c
> > +++ b/gcc/tree-ssa-live.c
> > @@ -623,13 +623,24 @@ clear_unused_block_pointer (void)
> > {
> > unsigned i;
> > tree b;
> > - gimple *stmt = gsi_stmt (gsi);
> > + gimple *stmt;
> >
> > +next:
> > + stmt = gsi_stmt (gsi);
> > if (!is_gimple_debug (stmt) && !gimple_clobber_p (stmt))
> > continue;
> > b = gimple_block (stmt);
> > if (b && !TREE_USED (b))
> > - gimple_set_block (stmt, NULL);
> > + {
> > + if (gimple_debug_nonbind_marker_p (stmt))
> > + {
> > + gsi_remove (&gsi, true);
> > + if (gsi_end_p (gsi))
> > + break;
> > + goto next;
> > + }
> > + gimple_set_block (stmt, NULL);
> > + }
> > for (i = 0; i < gimple_num_ops (stmt); i++)
> > walk_tree (gimple_op_ptr (stmt, i),
> > clear_unused_block_pointer_1,
> > NULL, NULL);
> >
>
--
Richard Biener <rguent...@suse.de>
SUSE Software Solutions Germany GmbH, Maxfeldstrasse 5, 90409 Nuernberg,
Germany; GF: Felix Imendörffer; HRB 36809 (AG Nuernberg)
