On Fri, 4 Dec 2020, Bernd Edlinger wrote:
> On 12/3/20 9:30 AM, Richard Biener wrote:
> > On Wed, 2 Dec 2020, Bernd Edlinger wrote:
> >
> >> On 12/2/20 8:50 AM, Richard Biener wrote:
> >>> On Tue, 1 Dec 2020, Bernd Edlinger wrote:
> >>>
> >>>> Hi!
> >>>>
> >>>>
> >>>> This removes gimple_debug stmts without block info after a
> >>>> NULL INLINE_ENTRY.
> >>>>
> >>>> 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-11-20 Bernd Edlinger <bernd.edlin...@hotmail.de>
> >>>>
> >>>> * cfgexpand.c (expand_gimple_basic_block): Remove debug_begin_stmts
> >>>> following a removed debug_inline_entry.
> >>>>
> >>>>
> >>>> Bootstrapped and reg-tested on x86_64-pc-linux-gnu.
> >>>> Is it OK for trunk?
> >>>
> >>> So are those visited by clear_unused_block_pointer? If so wouldn't
> >>> it be more appropriate to remove those there, when we elide the
> >>> inlined block scope?
> >>>
> >>
> >> That's what I thought initially, but that is only true for 99% of the
> >> inline statements. However 1% of the inline_entries without block info,
> >> and debug_begin_stmts without block info, that have line numbers from
> >> an inline header, do actually originate here:
> >>
> >> copy_debug_stmt (gdebug *stmt, copy_body_data *id)
> >> {
> >> tree t, *n;
> >> struct walk_stmt_info wi;
> >>
> >> if (tree block = gimple_block (stmt))
> >> {
> >> n = id->decl_map->get (block);
> >> gimple_set_block (stmt, n ? *n : id->block);
> >> }
> >>
> >> because id->block is NULL, and decl_map does not have
> >> an entry.
> >>
> >> So I tracked it down why that happens.
> >>
> >> I think remap_gimple_stmt should just drop those nonbind markers
> >> on the floor when the call statement has no block information.
> >>
> >> Once that is fixed, the special handling of inline entries without
> >> block info can as well be moved from remap_gimple_stmt to
> >> clear_unused_block_pointer.
> >>
> >> What do you think of this (not yet fully tested) patch?
> >>
> >> Is it OK when bootstrap and reg-testing passes?
> >
> > diff --git a/gcc/tree-inline.c b/gcc/tree-inline.c
> > index d9814bd..e87c653 100644
> > --- a/gcc/tree-inline.c
> > +++ b/gcc/tree-inline.c
> > @@ -1819,7 +1819,8 @@ 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->block)
> > return stmts;
> >
> > Isn't this overly pessimistic in throwing away all debug markers
> > of an inline rather than just those which are associated with
> > the outermost scope (that's mapped to NULL if !id->block)? Can
> > we instead remap the block here (move it from copy_debug_stmt)
> > and elide the copy only when it maps to NULL?
> >
>
> Yes, indeed, I missed the fact that this is also called up from
> tree_function_versioning. id->block is always NULL in that case.
> But since this should be a 1:1 copy, missing block info should not
> get worse as it already is. Fortunately it is possible to distinguish
> that from the actual inlining by looking at id->call_stmt.
>
>
> >
> > gdebug *copy = as_a <gdebug *> (gimple_copy (stmt));
> > diff --git a/gcc/tree-ssa-live.c b/gcc/tree-ssa-live.c
> > index 21a9ee4..ca119c6 100644
> > --- a/gcc/tree-ssa-live.c
> > +++ b/gcc/tree-ssa-live.c
> > @@ -623,13 +623,25 @@ 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))
> >
> > why only for inlined BLOCKs? Did you want to restrict it further
> > to inlined_function_outer_scope_p?
> >
>
> Yes.
> I had assumed that check would be sufficient, but as you said,
> I have to walk the block structure, until I find a
> inlined_function_outer_scope_p.
>
> I don't know if there is a chance that any of the debug lines will
> get a block info assigned in the end, if id->block == NULL, but I think
> it does not hurt to remove the debug statements in copy_debug_stmt.
>
> > I guess I don't understand the debug situation fully - I guess it is
> > about jumping to locations in inlines where the call stack does
> > not show we are in the actual inlined function? But IIRC at least
> > unused BLOCK removal never elides the actual
> > inlined_function_outer_scope_p which would leave the inlining case
> > you spotted. But there we should zap all markers that belong to
> > the inlined function but not those which belong to another inline
> > instance? So we want to walk BLOCK_SUPERCONTEXT until we hit
> > an inlined_function_outer_scope_p where we don't want to zap
> > or the inlined functions outermost scope where we do want to zap
> > (if the call stmt has a NULL block)?
> >
>
> Yes, what I want to avoid is line numbers that do not correspond
> to the block structure, but there is also a big GDB patch I want
> to get applied, before debugging in optimized code will work
> really a lot better than it used to be.
>
> I have now probably an acceptable patch, which addresses another
> common cause why inlined functions end up without block info.
>
> That is a missing location info in a call statement in
> ipa_param_body_adjustments::modify_call_stmt.
>
> What remains now, is just https://gcc.gnu.org/bugzilla/show_bug.cgi?id=94474
> yes it hit me a second time :-)
>
> These thunks do not have line numbers and no block info at all,
> yet the original function is completely folded and again inlined
> into the thunk, and gets all debug info removed when it is inlined.
>
> I wonder if that can't be fixed by avoiding creating thunks of very short
> functions as in the PR, and once they are creating bail out in
> expand_call_inline when the DECL_IGNORED_P is set in the
> current_function_decl?
>
>
> So, attached you'll find is my new patch.
>
> Bootstrap and reg-tested on x86_64-pc-linux-gnu.
> Is it OK for trunk?
The ipa-param-manipulation.c hunk is OK, please commit separately.
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
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?
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);
