This is the mail archive of the gdb-patches@sourceware.org mailing list for the GDB project.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]