Re: [PATCH 2/2] Report stop locations in inlined functions.

[Pedro Alves <palves@redhat.com>]

Hi Keith,

On 10/30/2017 09:18 PM, Keith Seitz wrote:
> On 10/27/2017 05:37 AM, Pedro Alves wrote:
>>
>> Can you send me these as full patches, or send me the
>> patch they apply on top of too?  I'll need to play with
>> it a bit to understand it all better.
> 
> Sure, here they are. The two are from my stgit sandbox so #2 applies on top
> of #1. [That allowed me to easily flip between the two and maintain tests.]

Finally managed to play a bit with this.

> 
> First up is the more "complex" solution, which does /not/ move the call
> to bpstat_stop_status, but instead uses (a new function) build_bpstat_chain.
> This is used to get at the current breakpoint (if any) to pass to
> skip_inline_frames.

Yes, let's use this one.  The "simpler" one is too ad hoc.

>  	{
> -	  skip_inline_frames (ecs->ptid);
> +	  struct breakpoint *bpt = NULL;
> +
> +	  stop_chain = build_bpstat_chain (aspace, stop_pc, &ecs->ws);
> +	  if (stop_chain != NULL)
> +	    bpt = stop_chain->breakpoint_at;
> +
> +	  skip_inline_frames (ecs->ptid, bpt);

I think you should pass down the stop_chain directly to
skip_inline_frames.  This is because a stop can be explained
by more than one breakpoint (that's why it's a chain), and
the user breakpoint may not be the head of the chain.

>  
>  	  /* Re-fetch current thread's frame in case that invalidated
>  	     the frame cache.  */
> @@ -5945,7 +5952,7 @@ handle_signal_stop (struct execution_control_state *ecs)
>       handles this event.  */
>    ecs->event_thread->control.stop_bpstat
>      = bpstat_stop_status (get_regcache_aspace (get_current_regcache ()),
> -			  stop_pc, ecs->ptid, &ecs->ws);
> +			  stop_pc, ecs->ptid, &ecs->ws, stop_chain);
>  
>    /* Following in case break condition called a
>       function.  */
> diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
> index b6154cdcc5..f06ecf61a6 100644
> --- a/gdb/inline-frame.c
> +++ b/gdb/inline-frame.c
> @@ -301,7 +301,7 @@ block_starting_point_at (CORE_ADDR pc, const struct block *block)
>     user steps into them.  */
>  
>  void
> -skip_inline_frames (ptid_t ptid)
> +skip_inline_frames (ptid_t ptid, struct breakpoint *bpt)
>  {
>    CORE_ADDR this_pc;
>    const struct block *frame_block, *cur_block;
> @@ -327,7 +327,25 @@ skip_inline_frames (ptid_t ptid)
>  	      if (BLOCK_START (cur_block) == this_pc
>  		  || block_starting_point_at (this_pc, cur_block))
>  		{
> -		  skip_count++;
> +		  bool skip_this_frame = true;
> +
> +		  if (bpt != NULL
> +		      && user_breakpoint_p (bpt)
> +		      && breakpoint_address_is_meaningful (bpt))
> +		    {
> +		      for (bp_location *loc = bpt->loc; loc != NULL;
> +			   loc = loc->next)
> +			{
> +			  if (loc->address == this_pc)
> +			    {
> +			      skip_this_frame = false;
> +			      break;
> +			    }
> +			}
> +		    }

So here you'd check each breakpoint in the bpstat chain, not
just the head.

Also, to look at the locations, you should look at
bpstat->bp_location_at, not at the locations of the breakpoint,
because some of the locations of the breakpoint may be
disabled/not-inserted, for example.  There's one bpstat entry for
each _location_ that actually caused a stop, so checking bp_location_at
directly saves one loop.  (Though you'll add one loop back to walk
the bpstat chain, so it's a wash).  Careful to not
bpstat->bp_location_at->owner though, see comments in breakpoint.h.

(I think you could look at bpstat->bp_location_at->loc_type,
match against bp_loc_software_breakpoint / bp_loc_hardware_breakpoint,
instead of exposing breakpoint_address_is_meaningful.)

> +
> +		  if (skip_this_frame)
> +		    skip_count++;
>  		  last_sym = BLOCK_FUNCTION (cur_block);

Couldn't this break out of the outer loop if !skip_this_frame ?

Like:

		  if (!skip_this_frame)
		    break;

		  skip_count++;
		  last_sym = BLOCK_FUNCTION (cur_block);

?

Thanks,
Pedro Alves