Re: [PATCH v5] Fix range end handling of inlined subroutines

[Bernd Edlinger <bernd.edlinger@hotmail.de>]

On 12/30/20 7:48 PM, Simon Marchi wrote:
>> From 3c345ab5f7c497151d21945cba8668e91db00e7b Mon Sep 17 00:00:00 2001
>> From: Bernd Edlinger <bernd.edlinger@hotmail.de>
>> Date: Tue, 3 Nov 2020 18:41:43 +0100
>> Subject: [PATCH] Fix range end handling of inlined subroutines
>>
>> This introduces a new line table flag is_weak.
>> The line entries at the end of a subroutine range,
>> use this to indicate that they may possibly
>> be part of the previous subroutine.
>>
>> When there is a sequence of line entries at the
>> same address where an inline range ends, and the
>> last item has is_stmt = 0, we force all previous
>> items to have is_weak = 1.
>>
>> This is about the best we can do at the moment,
>> unless location view information are added to the
>> block ranges debug info structure, and location
>> views are implemented in gdb in general.
> 
> Ok, I'm not sure I can reasonably review this.
> 
> First, it would make the patch easier and more inviting to review if
> the commit message went a bit more in depth.  If I know nothing about
> the problem, it's some time-consuming patchwork to go read all the
> pieces of informations in the bug reports, maybe the test cases, to
> try to just understand what is the problem this patch is tackling.
> 
> When fixing anything remotely complicated, the commit message should be
> of the general form:
> 
> - When we do ..., this happens
> - This is wrong, we would expect ...
> - This is caused by ...
> - To avoid this, I propose ...
> 
> After reading the commit message, I should at least be convinced that
> there is indeed a problem to be fixed and that this patch, in this form
> or another, is desirable.
> 

Okay, I will try to improve the commit message.

> Anyhow, I understand somewhat the problem now (at least the one described
> in bug 25987).  But the patch is changing bits of twisted code here and
> there without much explanation, and I can't make a mental model of what
> each bit does.
> 

Yes, the whole patch accumulated some more or less related bits over time.

Currently debugging of optimized code is much less convenient than it
should actually be.  The situation got actually worse with gcc-8 (+/- 1)
although the debug information actually was supposed to be improved
by brand new features as -gstatement-frontiers and -gvariable-location-views.

Initially I became aware that the n command often steps into the last
line of an inline function, and that the skip function is often not able
to skip an inline function, because the call stack is not correct,
i.e. misses the inline frame.

This is always due to a line breakpoint at the end of an inline
function, or the end of a subrange of an inline.  When this happens
we always have a !is-stmt line table entry from the calling program.

Having breakpoints at these places looked odd initially, but now I think
that it can be a point where result values from the inline function
can be inspected, but that works only when the debugger knows the
executing inline function from the program block structure.

This patch detects the "weak" line table entries which are likely
part of the previous inline block, and if we have such a location,
it assumes the location belongs to the previous block, which could
theoretically wrong, but that happens virtually never, as far as
I can tell.  While currently the assumption is that this line
belongs to the calling function, which causes the confusing call stacks,
which do not match with the displayed visual location.

Additionally it may happen that the infrun machinery steps from one
inline range to another inline range of the same inline function.
That can look like jumping back and forth from the calling program
to the inline function, while really the inline function just jumps
from a hot to a cold section of the code, i.e. error handling.

Additionally it may happen that one inline sub-range is empty.
But filtering that information away is not the right solution,
since although there is no actual code from the inline, it is
still possible that variables from an inline function can be
inspected here.

Additionally it may happen that an inline function is completely
empty, but as the test cases demonstrate, there is a value from
the empty inline which can be inspected here.

Additionally it may happen that the inline entry point is not the
lowest address of an inline function, but currently all non-zero
inline subranges are sorted by PC and the lowest PC is used when
the user asks for a breakpiont on the function, I found that by
ananlyzing differences on which breakpoint PCs are set with
and without the patch.  It turns out, that the entry point is often
but not always the lowest PC.

Conceptually my patch uses a heuristic to work around a deficiency
in the dwarf-4 and dwarf-5 dwarf2_rnglists structure.
That is in my opinion there should be a location view number
for each inline begin and end range, as well as the
DW_TAG_inlined_subroutine low_pc and high_pc, similar
to the entry_point_view_number DW_AT_GNU_entry_view.

But adding that information to dwarf-5 (or 6 ?) is out of reach
for me.


> Is it possible to split the patch into smaller pieces?  For example, a first
> patch could just the DWARF reader, and perhaps other pieces, with the goal
> of having no functional changes (just augment GDB's internal line table),
> with an explanation of what's added.  With this kind of code, the smaller
> the increment the better.
> 

Yes, that should be possible.  I will try to split the patch as you suggested.


Thanks
Bernd.

> Simon
>