[PATCH] Remove misleading debug line entries

[Bernd Edlinger]

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.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: 0001-Remove-misleading-debug-line-entries.patch
Type: text/x-patch
Size: 4853 bytes
Desc: not available
URL: <https://gcc.gnu.org/pipermail/gcc-patches/attachments/20201204/42aeb13c/attachment-0001.bin>