Re: [pushed] Change inline frame breakpoint skipping logic (fix gdb.gdb/selftest.exp)

Joel Brobecker
2018-06-26T22:02:49Z

Hi Pedro,

> > Program received signal SIGSEGV, Segmentation fault.
> > 0x00000000006f42bb in stopped_by_user_bp_inline_frame (
> > stop_chain=<optimized out>, frame_block=<optimized out>)
> > at /homes/brobecke/act/gdb/gdb-head/gdb/inline-frame.c:305
> > 305 && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol))
> > (gdb) p loc->symbol
> > $1 = (const symbol *) 0x0
>
> Whoops. I remember actually thinking about loc->symbol potentially
> being null, but somehow convinced myself that SYMBOL_BLOCK_VALUE
> would return null in that case... :-P
>
> >
> > I don't know yet whether that's a valid assumption or something
> > occurred earlier in the process. Any thoughts on this before I start
> > looking deeper?
> >
> > I'm using a version of GCC 7.3.1 on x86_64-linux if anyone wants to
> > reproduce.
>
> If we just add a "loc->symbol != NULL" check, then we end up
> presenting the stop at the caller of the inline function, where
> the inline function was inlined, which is not what we want, since that's
> not where the user set the breakpoint.
>
> Recording the symbol in the location (it is copied from the sal
> that linespec.c creates into the location by
> add_location_to_breakpoint), like in the patch below, makes gdb present
> the stop at the "break here" line successfully.
>
> I tried to come up with a more complicated testcase that included
> more nested blocks inside the inlined function, to see if we would
> need to record the inner inlined block in the sal instead of the
> function's symbol (does that actually make sense for inlined functions?),
> but all I came up with worked with this patch as is. So maybe we
> can defer thinking about that until we find a testcase?

Just a quick message that the patch makes sense to me, and that
I was just able to run it through AdaCore's testsuite with succes.
Or, I should qualify that - there is one tiny change that I haven't
had time to analyze, but from the surface, it is exactly what you
explained about why you need the second hunk.

I haven't had a chance to run it through the official testsuite,
however, as I have to go ... I am so laaaaate!

I can do that tomorrow, or if you prefer to just finish the patch
up and push it, it'd be perfect. I think the patch is good.

Thanks again!

>
> >From ac746927c3a8078098729dc3256010b2c1e617f8 Mon Sep 17 00:00:00 2001
> From: Pedro Alves <palves@redhat.com>
> Date: Tue, 26 Jun 2018 19:14:41 +0100
> Subject: [PATCH] inline
>
> ---
> gdb/inline-frame.c | 1 +
> gdb/linespec.c | 1 +
> 2 files changed, 2 insertions(+)
>
> diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
> index 896b0004e4a..3d07f8d0970 100644
> --- a/gdb/inline-frame.c
> +++ b/gdb/inline-frame.c
> @@ -302,6 +302,7 @@ stopped_by_user_bp_inline_frame (const block *frame_block, bpstat stop_chain)
>
> if ((t == bp_loc_software_breakpoint
> || t == bp_loc_hardware_breakpoint)
> + && loc->symbol != nullptr
> && frame_block == SYMBOL_BLOCK_VALUE (loc->symbol))
> return true;
> }
> diff --git a/gdb/linespec.c b/gdb/linespec.c
> index ae0200b8133..93e66c389f7 100644
> --- a/gdb/linespec.c
> +++ b/gdb/linespec.c
> @@ -2196,6 +2196,7 @@ create_sals_line_offset (struct linespec_state *self,
>
> if (self->funfirstline)
> skip_prologue_sal (&intermediate_results[i]);
> + intermediate_results[i].symbol = sym;
> add_sal_to_sals (self, &values, &intermediate_results[i],
> sym ? SYMBOL_NATURAL_NAME (sym) : NULL, 0);
> }
> --
> 2.14.4
>
> Thanks,
> Pedro Alves

--
Joel