| Message ID | AM6PR03MB5170693F7CA0CAF100640148E4F40@AM6PR03MB5170.eurprd03.prod.outlook.com |
|---|---|
| State | New |
| Headers | show |
| Series | Remove misleading debug line entries | expand |
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.edlinger@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? Thanks, Richard. > > Thanks > Bernd. >
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.edlinger@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? Thanks Bernd. > Thanks, > Richard. > >> >> Thanks >> Bernd. >> >
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.edlinger@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? 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? 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)? + { + gsi_remove (&gsi, true); + if (gsi_end_p (gsi)) + break; > > Thanks > Bernd. > > > Thanks, > > Richard. > > > >> > >> Thanks > >> Bernd. > >> > > >
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.edlinger@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? Thanks Bernd.
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.edlinger@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.edlinger@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.edlinger@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);
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.edlinger@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.edlinger@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? 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); >
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.edlinger@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.edlinger@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); > > >
On 12/8/20 11:35 AM, Richard Biener wrote: > > + { > + /* 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)) > For gimple_debug_inline_entry_p inlined_function_outer_scope_p (b) is always true, and the code always proceeds to gsi_remove the gimple_debug_inline_entry_p. But for gimple_debug_begin_stmt_p all paths are actually taken. > Iff we ever elide such block then this will still never be > false since we've otherwise removed the BLOCK from the block I can assure you TREE_USED (b) can be false here. > 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. > Indeed, that is a good question. I tried to find out if there are constraints that are preserved by remove_unused_scope_block_p, that can be used to prove that the BLOCK_SUPERCONTEXT walking loop does not fine the "wrong" inline instance. diff --git a/gcc/tree-ssa-live.c b/gcc/tree-ssa-live.c index 9ea24a1..997ccee 100644 --- a/gcc/tree-ssa-live.c +++ b/gcc/tree-ssa-live.c @@ -525,6 +525,10 @@ remove_unused_scope_block_p (tree scope, bool in_ctor_dtor_block) *t = BLOCK_SUBBLOCKS (*t); while (BLOCK_CHAIN (*t)) { + gcc_assert (supercontext == BLOCK_SUPERCONTEXT (BLOCK_SUPERCONTEXT (*t))); + if (!TREE_USED (*t)) + gcc_assert (!inlined_function_outer_scope_p (BLOCK_SUPERCONTEXT (*t))); + gcc_assert (!TREE_USED (BLOCK_SUPERCONTEXT (*t))); BLOCK_SUPERCONTEXT (*t) = supercontext; t = &BLOCK_CHAIN (*t); } So I think I can prove that the above assertions hold, at least when this block is not taken: else if (!flag_auto_profile && debug_info_level == DINFO_LEVEL_NONE && !optinfo_wants_inlining_info_p ()) { /* Even for -g0 don't prune outer scopes from artificial functions, otherwise diagnostics using tree_nonartificial_location will not be emitted properly. */ if (inlined_function_outer_scope_p (scope)) { tree ao = BLOCK_ORIGIN (scope); if (ao && TREE_CODE (ao) == FUNCTION_DECL && DECL_DECLARED_INLINE_P (ao) && lookup_attribute ("artificial", DECL_ATTRIBUTES (ao))) unused = false; } } in that case it should be irrelevant, since we wont have debug_begin_stmt_p if debug_info_level == DINFO_LEVEL_NONE. So the above assertions mean, that *if* !TREE_USED (b) which is the precondition for the while loop, then we know that BLOCK_SUPERCONTEXT (b) *was* not a inlined_function_outer_scope_p, and it was replaced by BLOCK_SUPERCONTEXT (BLOCKL_SUPERCONTEXT (b)) so the loop skips one step, but the result is still the same inline block. However what concerns me, is that the assertion if (TREE_USED (*t)) gcc_assert (!inlined_function_outer_scope_p (BLOCK_SUPERCONTEXT (*t)) does not hold. I think that means, that it can happen, that a USED block is moved out of the inline block. And while I have no test case for it, that sheds a big question on the correctness of the debug info when that happens. But I think that is a different issue if I at all. > 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 did not look at before, and just expected it to behave reasonably, that is just elide *unused* lexical scopes, and *empty* subroutines, but that it seems to move *used* lexical scopes to the grandfather scope, while *removing* the now *emptied* original inline block is odd. Or maybe I missed something... > 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 > I would rather not move *used* blocks out of an inline scope, whether or not that appears to be unused. But maybe as a follow up, maybe I find even a test case, where the debug info is broken, I have debugged a lot optimized code, and never encountered something like that, but that proves not much. > 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. > Yes, it does indeed happen. I have a gdb patch for that already posted for review: https://sourceware.org/pipermail/gdb-patches/2020-November/173614.html It is important to only break at the entry_point_pc, not when crossing a sub-block boundary of the same subroutine. Another deficiency that is not completely solvable is line breakpoints at the exit of a subroutine, the dwarf format specifies the block ranges just as byte ranges, while the line table can hold different lines at the same PC, from different scopes, there have different view numbers. So I would be happy to have the complete call stack, maybe some frames too much, instead of some frames missing in the middle... Thanks Bernd.
On 12/8/20 7:57 PM, Bernd Edlinger wrote: > On 12/8/20 11:35 AM, Richard Biener wrote: >> >> + { >> + /* 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? We can remove inlined function outer scope when they have no subblocks any more, or only unused subblocks, and there is an exception from the rule when no debug info is generated, that is due to this: > else if (!flag_auto_profile && debug_info_level == DINFO_LEVEL_NONE > && !optinfo_wants_inlining_info_p ()) > { > /* Even for -g0 don't prune outer scopes from artificial > functions, otherwise diagnostics using tree_nonartificial_location > will not be emitted properly. */ > if (inlined_function_outer_scope_p (scope)) > { > tree ao = BLOCK_ORIGIN (scope); > if (ao > && TREE_CODE (ao) == FUNCTION_DECL > && DECL_DECLARED_INLINE_P (ao) > && lookup_attribute ("artificial", DECL_ATTRIBUTES (ao))) > unused = false; > } > } > I instrumented the remove_unused_scope_block_p now as follows, to better understand what happens here: diff --git a/gcc/tree-ssa-live.c b/gcc/tree-ssa-live.c index 9ea24a1..3dd859c 100644 --- a/gcc/tree-ssa-live.c +++ b/gcc/tree-ssa-live.c @@ -525,9 +525,15 @@ remove_unused_scope_block_p (tree scope, bool in_ctor_dtor_block) *t = BLOCK_SUBBLOCKS (*t); while (BLOCK_CHAIN (*t)) { + gcc_assert (TREE_USED (*t)); + if (debug_info_level != DINFO_LEVEL_NONE) + gcc_assert (!inlined_function_outer_scope_p (BLOCK_SUPERCONTEXT (*t))); BLOCK_SUPERCONTEXT (*t) = supercontext; t = &BLOCK_CHAIN (*t); } + gcc_assert (TREE_USED (*t)); + if (debug_info_level != DINFO_LEVEL_NONE) + gcc_assert (!inlined_function_outer_scope_p (BLOCK_SUPERCONTEXT (*t))); BLOCK_CHAIN (*t) = next; BLOCK_SUPERCONTEXT (*t) = supercontext; t = &BLOCK_CHAIN (*t); This survives a bootstrap, but I consider that just as an experiment... This means that the BLOCK_SUPERCONTEXT pointers never skip an inlined_function_outer_scope_p, *except* when no debug info is generated, but then it is fine, as there are either no debug_nonbind_marker_p, or it would not matter, if an outer scope is missed. After the above loop runs, the BLOCK_SUBBLOCKS->BLOCK_CHAIN have only Blocks with TREE_USED Blocks with !TREE_USED are removed from the SUBBLOCKS->CHAIN list, but have still a valid BLOCK_SUPERCONTEXT. However BLOCK_CHAIN and BLOCK_SUBBLOCKS are not used any more, and could theoretically misused for something, but fortunately that is not necessary. I think that result suggests that the proposed patch does the right thing, already as-is. Do you agree? Thanks Bernd.
On Wed, 9 Dec 2020, Bernd Edlinger wrote: > On 12/8/20 7:57 PM, Bernd Edlinger wrote: > > On 12/8/20 11:35 AM, Richard Biener wrote: > >> > >> + { > >> + /* 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? > > We can remove inlined function outer scope when they have no subblocks > any more, or only unused subblocks, and there is an exception from the > rule when no debug info is generated, that is due to this: > > > else if (!flag_auto_profile && debug_info_level == DINFO_LEVEL_NONE > > && !optinfo_wants_inlining_info_p ()) > > { > > /* Even for -g0 don't prune outer scopes from artificial > > functions, otherwise diagnostics using tree_nonartificial_location > > will not be emitted properly. */ > > if (inlined_function_outer_scope_p (scope)) > > { > > tree ao = BLOCK_ORIGIN (scope); > > if (ao > > && TREE_CODE (ao) == FUNCTION_DECL > > && DECL_DECLARED_INLINE_P (ao) > > && lookup_attribute ("artificial", DECL_ATTRIBUTES (ao))) > > unused = false; > > } > > } > > > > I instrumented the remove_unused_scope_block_p now as follows, > to better understand what happens here: > > diff --git a/gcc/tree-ssa-live.c b/gcc/tree-ssa-live.c > index 9ea24a1..3dd859c 100644 > --- a/gcc/tree-ssa-live.c > +++ b/gcc/tree-ssa-live.c > @@ -525,9 +525,15 @@ remove_unused_scope_block_p (tree scope, bool in_ctor_dtor_block) > *t = BLOCK_SUBBLOCKS (*t); > while (BLOCK_CHAIN (*t)) > { > + gcc_assert (TREE_USED (*t)); > + if (debug_info_level != DINFO_LEVEL_NONE) > + gcc_assert (!inlined_function_outer_scope_p (BLOCK_SUPERCONTEXT (*t))); > BLOCK_SUPERCONTEXT (*t) = supercontext; > t = &BLOCK_CHAIN (*t); > } > + gcc_assert (TREE_USED (*t)); > + if (debug_info_level != DINFO_LEVEL_NONE) > + gcc_assert (!inlined_function_outer_scope_p (BLOCK_SUPERCONTEXT (*t))); > BLOCK_CHAIN (*t) = next; > BLOCK_SUPERCONTEXT (*t) = supercontext; > t = &BLOCK_CHAIN (*t); > > This survives a bootstrap, but I consider that just as an experiment... > > This means that the BLOCK_SUPERCONTEXT pointers never skip > an inlined_function_outer_scope_p, *except* when no debug info is > generated, but then it is fine, as there are either no debug_nonbind_marker_p, > or it would not matter, if an outer scope is missed. > > After the above loop runs, the BLOCK_SUBBLOCKS->BLOCK_CHAIN have only > Blocks with TREE_USED > Blocks with !TREE_USED are removed from the SUBBLOCKS->CHAIN list, but > have still a valid BLOCK_SUPERCONTEXT. However BLOCK_CHAIN and BLOCK_SUBBLOCKS > are not used any more, and could theoretically misused for something, but > fortunately that is not necessary. > > I think that result suggests that the proposed patch does the right thing, > already as-is. > > > Do you agree? Yes, I think that in the end with all the constraints under which we elide blocks the patch does the correct thing. What makes me uneasy is how un-obvious that is ;) That said, the comment should probably say Elide debug marker stmts that have an associated BLOCK from an inline instance removed with also the outermost scope BLOCK of said inline instance removed. If the outermost scope BLOCK of said inline instance is preserved use that in place of the removed BLOCK. That keeps the marker associated to the correct inline instance (or no inline instance in case it was not from an inline instance). That at least makes the intent clear while the implementation might still not be 100% obviously correct. That's why I suggested to track "inline instance outermost scope BLOCK of BLOCK" in the function that messes with the BLOCK tree. But I guess the comment is enough to reverse-engineer that. So OK with a comment along this line (if I got it correct, that is ;)) Thanks, Richard.
From 464867ca9b4cc6270fb6d41dc5346dc55395efb0 Mon Sep 17 00:00:00 2001 From: Bernd Edlinger <bernd.edlinger@hotmail.de> Date: Fri, 13 Nov 2020 16:26:28 +0100 Subject: [PATCH] Remove misleading debug line entries 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.edlinger@hotmail.de> * cfgexpand.c (expand_gimple_basic_block): Remove debug_begin_stmts following a removed debug_inline_entry. --- gcc/cfgexpand.c | 8 +++++++- 1 file changed, 7 insertions(+), 1 deletion(-) diff --git a/gcc/cfgexpand.c b/gcc/cfgexpand.c index 1df6f4b..6bd38ac 100644 --- a/gcc/cfgexpand.c +++ b/gcc/cfgexpand.c @@ -5785,6 +5785,7 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls) tree value = NULL_TREE; rtx val = NULL_RTX; machine_mode mode; + bool skip_inline_loc = false; if (!gimple_debug_nonbind_marker_p (stmt)) { @@ -5837,7 +5838,10 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls) if (block) val = GEN_RTX_DEBUG_MARKER_INLINE_ENTRY_PAT (); else - goto delink_debug_stmt; + { + skip_inline_loc = true; + goto delink_debug_stmt; + } } else gcc_unreachable (); @@ -5877,6 +5881,8 @@ expand_gimple_basic_block (basic_block bb, bool disable_tail_calls) stmt = gsi_stmt (nsi); if (!is_gimple_debug (stmt)) break; + if (skip_inline_loc && !gimple_block (stmt)) + goto delink_debug_stmt; } set_curr_insn_location (sloc); -- 1.9.1