[2/2] Report stop locations in inlined functions.

  On 07/11/2017 03:36 AM, Keith Seitz wrote:
> This is a patch for a very related inline function problem.  Using the
> test case from breakpoints/17534,
> 
> 3	static inline void NVIC_EnableIRQ(int IRQn)
> 4	{
> 5	  volatile int y;
> 6	  y = IRQn;
> 7	}
> 8
> 9	__attribute__( ( always_inline ) ) static inline void __WFI(void)
> 10	{
> 11	    __asm volatile ("nop");
> 12	}
> 13
> 14	int main(void) {
> 15
> 16	    x= 42;
> 17
> 18	    if (x)
> 19	      NVIC_EnableIRQ(16);
> 20	    else
> 21	      NVIC_EnableIRQ(18);
> (gdb) b NVIC_EnableIRQ
> Breakpoint 1 at 0x4003e4: NVIC_EnableIRQ. (2 locations)
> (gdb) r
> Starting program: 17534
> 
> Breakpoint 1, main () at 17534.c:19
> 19	      NVIC_EnableIRQ(16);
> 
> This happens because skip_inline_frames automatically skips every inlined
> frame.  Based on a suggestion by Jan, this patch introduces a new function,
> breakpoint_for_stop, which attempts to ascertain which breakpoint, if any,
> caused a particular stop in the inferior.  That breakpoint is then passed
> to skip_inline_frames so that it can decide if a particular inlined frame
> should be skipped.
> 
> I've had to separate the bpstat chain building from bpstat_stop_status --
> py-finish-breakpoint.exp did not like me calling bpstat_stop_status multiple
> times.  So I've added the ability to allocate the chain separately and
> optionally pass it to bpstat_stop_status, which remains otherwise unchanged.
> 
> With this patch, GDB now correctly reports that the inferior has stopped
> inside the inlined function:
> 
> (gdb) r
> Starting program: 17534
> 
> Breakpoint 1, NVIC_EnableIRQ (IRQn=16) at 17534.c:6
> 6	  y = IRQn;
> 
> I don't quite like this, though.  This solution involves calling
> decode_line_full, and that is really expensive, so I would be grateful if
> maintaienrs could offer advice on how to better tackle this.

I'm still trying to grok these patches fully, but, shouldn't comparing
the breakpoint's bp_location's addresses work the same?  I.e., with this,
gdb.opt/inline-break.exp still passes cleanly here:

From a7b8eb698d2853fc16bca1b933be5c517dd1d446 Mon Sep 17 00:00:00 2001
From: Pedro Alves <palves@redhat.com>
Date: Tue, 18 Jul 2017 18:09:50 +0100
Subject: [PATCH] no decode

---
 gdb/breakpoint.c   |  2 +-
 gdb/inline-frame.c | 45 +++++++++++----------------------------------
 2 files changed, 12 insertions(+), 35 deletions(-)
  

On 07/18/2017 06:16 PM, Pedro Alves wrote:
> On 07/11/2017 03:36 AM, Keith Seitz wrote:

>> I don't quite like this, though.  This solution involves calling
>> decode_line_full, and that is really expensive, so I would be grateful if
>> maintaienrs could offer advice on how to better tackle this.
> 
> I'm still trying to grok these patches fully, but, shouldn't comparing
> the breakpoint's bp_location's addresses work the same?  I.e., with this,
> gdb.opt/inline-break.exp still passes cleanly here:
> 

Hmm, and with this direction, we may not even need the
breakpoint_for_stop function.  The location(s) that caused
the stop is/are in the bpstat chain:

  stop_chain->bp_location_at->address

etc.  Using those also implicitly makes sure that you're only
consulting locations that were inserted/enabled, as other
not-enabled/inserted locations won't appear in the bpstat chain.

Maybe we need to move this bit in infrun.c:

  /* See if there is a breakpoint/watchpoint/catchpoint/etc. that
     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_chain);

a bit above, before the skip_inline_frames call, and then
you don't even need to pass down the bpstat to skip_inline_frames,
as you can then access it from the thread directly?

Thanks,
Pedro Alves
  

On 07/18/2017 10:16 AM, Pedro Alves wrote:
>>
>> I don't quite like this, though.  This solution involves calling
>> decode_line_full, and that is really expensive, so I would be grateful if
>> maintaienrs could offer advice on how to better tackle this.
> 
> I'm still trying to grok these patches fully, but, shouldn't comparing
> the breakpoint's bp_location's addresses work the same?  I.e., with this,
> gdb.opt/inline-break.exp still passes cleanly here:

I've played with your suggestion, and I *think* I am getting close. :-)

> diff --git a/gdb/inline-frame.c b/gdb/inline-frame.c
> index 006ae0d..9120554 100644
> --- a/gdb/inline-frame.c
> +++ b/gdb/inline-frame.c
> @@ -349,23 +330,19 @@ skip_inline_frames (ptid_t ptid, struct breakpoint *bpt)
>  	      if (BLOCK_START (cur_block) == this_pc
>  		  || block_starting_point_at (this_pc, cur_block))
>  		{
> -		  int lsal_i;
> -		  struct linespec_sals *lsal;
>  		  bool skip_this_frame = true;
>  
> -		  for (lsal_i = 0;
> -		       VEC_iterate (linespec_sals, canonical.sals,
> -				    lsal_i, lsal); lsal_i++)
> +		  if (bpt != NULL
> +		      && breakpoint_address_is_meaningful (bpt))
>  		    {
> -		      struct symtabs_and_lines &sals = lsal->sals;
> -
> -		      for (int sals_i = 0; sals_i < sals.nelts; sals_i++)
> -			{
> -			  struct symtab_and_line &sal = sals.sals[sals_i];
> -
> -			  if (sal.pc == this_pc)
> +		      for (bp_location *loc = bpt->loc;
> +			   loc != NULL;
> +			   loc = loc->next)
> +			if (this_pc == loc->address)
> +			  {
>  			    skip_this_frame = false;
> -			}
> +			    break;
> +			  }
>  		    }
>  
>  		  if (skip_this_frame)
> 

The next version of this patch does this, and it works. One small addition I had to make was to /not/ skip inline frames for non-user breakpoints. step-resume breakpoints, IIRC, were otherwise broken.

Keith