Re: [PATCH 3/3] gdb: Better frame tracking for inline frames

Andrew Burgess
2019-12-28T23:23:16Z

* Bernd Edlinger <bernd.edlinger@hotmail.de> [2019-12-28 10:02:40 +0000]:

> On 12/26/19 11:33 PM, Andrew Burgess wrote:
> > * Christian Biesinger <cbiesinger@google.com> [2019-12-26 07:24:39 +0100]:
> >
> >> On Mon, Dec 23, 2019, 02:51 Andrew Burgess <andrew.burgess@embecosm.com>
> >> wrote:
> >>
> >> > This commit improves GDB's handling of inline functions when there are
> >> > more than one inline function in a stack, so for example if we have a
> >> > stack like:
> >> >
> >> > main -> aaa -> bbb -> ccc -> ddd
> >> >
> >> > And aaa, bbb, and ccc are all inline within main GDB should (when
> >> > given sufficient debug information) be able to step from main through
> >> > aaa, bbb, and ccc. Unfortunately, this currently doesn't work, here's
> >> > an example session:
> >> >
> >> > (gdb) start
> >> > Temporary breakpoint 1 at 0x4003b0: file test.c, line 38.
> >> > Starting program: /project/gdb/tests/inline/test
> >> >
> >> > Temporary breakpoint 1, main () at test.c:38
> >> > 38 global_var = 0;
> >> > (gdb) step
> >> > 39 return aaa () + 1;
> >> > (gdb) step
> >> > aaa () at test.c:39
> >> > 39 return aaa () + 1;
> >> > (gdb) step
> >> > bbb () at test.c:39
> >> > 39 return aaa () + 1;
> >> > (gdb) step
> >> > ccc () at test.c:39
> >> > 39 return aaa () + 1;
> >> > (gdb) step
> >> > ddd () at test.c:32
> >> > 32 return global_var;
> >> > (gdb) bt
> >> > #0 ddd () at test.c:32
> >> > #1 0x00000000004003c1 in ccc () at test.c:39
> >> > #2 bbb () at test.c:26
> >> > #3 aaa () at test.c:14
> >> > #4 main () at test.c:39
> >> >
> >>
> >> How come only #1 is printing the address?
> >
> > Well.....
> >
> > For inline frames the sal returned by find_frame_sal has a symtab and
> > line set, but doesn't have the pc or end fields set.
> >
> > If we then look at frame_show_address it seems specifically designed
> > to return false for precisely the case we're discussing.
> >
> > I agree with you that it seems odd that we only get the address for #1
> > in this case. I considered this patch:
> >
> > ## START ##
> >
> > diff --git a/gdb/stack.c b/gdb/stack.c
> > index 22820524871..ce85a1183f0 100644
> > --- a/gdb/stack.c
> > +++ b/gdb/stack.c
> > @@ -327,7 +327,7 @@ frame_show_address (struct frame_info *frame,
> > gdb_assert (inline_skipped_frames (inferior_thread ()) > 0);
> > else
> > gdb_assert (get_frame_type (get_next_frame (frame)) == INLINE_FRAME);
> > - return false;
> > + return frame_relative_level (frame) > 0;
> > }
> >
> > return get_frame_pc (frame) != sal.pc;
> >
> > ## END ##
> >
> > The addresses are printed more now, there's a few test failures that
> > would need to be checked first.
> >
>
> Hmm....
>
> In the case of inline functions the call stack would behave odd
> with this patch.
>
> Since the inline frames all share the same PC, the value is redundant,
> still different source line location is presented for each.
> Also when stepping in the inner frame, all inlined frames would
> also change the PC, so you get the impression that the inlined function
> is now called from a slightly different place.
>
> It might be more useful to add an info to the call stack,
> that a frame was inlined instead?
>
> #0 iii () at dw2-inline-many-frames.c:145
> #1 inlined in hhh () at dw2-inline-many-frames.c:115
> #2 inlined in ggg () at dw2-inline-many-frames.c:77
> #3 inlined in fff () at dw2-inline-many-frames.c:92
> #4 0x0000000000401226 in eee () at dw2-inline-many-frames.c:155
> #5 0x0000000000401209 in ddd () at dw2-inline-many-frames.c:135
> #6 inlined in ccc () at dw2-inline-many-frames.c:84
> #7 inlined in bbb () at dw2-inline-many-frames.c:108
> #8 inlined in aaa () at dw2-inline-many-frames.c:60
> #9 inlined in main () at dw2-inline-many-frames.c:125

I really want to like this idea, as I agree showing the address feels
less useful, and somehow marking the inline nature of things feels
like a good thing, but I'm put off by this:

#9 inlined in main () ...

The 'inlined in main' is telling us about frame #8, right? And this
feels weird too.

Not sure I have any better suggestions though.

Thanks,
Andrew

>
>
>
> Bernd.
>
> > Ideally I would like to keep changing this behaviour separate from
> > this patch series, but I do think this might be something we should
> > consider changing.
> >
> > Thanks,
> > Andrew
> >
>