Re: [PATCH v2] Don't elide all inlined frames

[Pedro Alves <palves at redhat dot com>]

Hi Keith,

Somehow I thought I had already replied to this, but apparently
not.  Here it goes.  This is close, but I'm still not clear why the
skip_inline_frames loop is how it currently is.  See more below.

On 02/28/2018 08:33 PM, Keith Seitz wrote:

>  
>  void
> -skip_inline_frames (ptid_t ptid)
> +skip_inline_frames (ptid_t ptid, bpstat stop_chain)
>  {
>    CORE_ADDR this_pc;
>    const struct block *frame_block, *cur_block;
> @@ -327,6 +326,36 @@ skip_inline_frames (ptid_t ptid)
>  	      if (BLOCK_START (cur_block) == this_pc
>  		  || block_starting_point_at (this_pc, cur_block))
>  		{
> +		  bool skip_this_frame = true;
> +
> +		  /* Loop over the stop chain and determine if execution
> +		     stopped in an inlined frame because of a user breakpoint.
> +		     If so do not skip the inlined frame.  */
> +		  for (bpstat s = stop_chain; s != NULL; s = s->next)
> +		    {
> +		      struct breakpoint *bpt = s->breakpoint_at;
> +
> +		      if (bpt != NULL && user_breakpoint_p (bpt))
> +			{
> +			  for (bp_location *loc = s->bp_location_at;
> +			       loc != NULL; loc = loc->next)
> +			    {
> +			      enum bp_loc_type t = loc->loc_type;
> +
> +			      if (loc->address == this_pc
> +				  && (t == bp_loc_software_breakpoint
> +				      || t == bp_loc_hardware_breakpoint))
> +				{
> +				  skip_this_frame = false;
> +				  break;
> +				}
> +			    }
> +			}
> +		    }
> +
> +		  if (!skip_this_frame)
> +		    break;
> +

It seems my comments from last time still apply here:

 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
 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
 [follow] bpstat->bp_location_at->owner though, see comments in breakpoint.h.
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

Had you tried that and it didn't work for some reason?

Also, this part of the comment:

 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

 > +
 > +		  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);

 ?
 ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

The part about breakint out of the outer loop no longer
applies as is, but, AFAICT, the current code is still letting the
outer stop_chain loop continue iterating even 'if (!skip_this_frame)'?


> -gdb_test "continue" \
> -         "Breakpoint $decimal, c\\.c_doit2 \\(\\).*" \
> -         "Hitting fourth call of read_small"
> +for {set i 0} {$i < 4} {incr i} {
> +    gdb_test "continue" \
> +	"Breakpoint $decimal, b\\.read_small \\(\\).*" \
> +	"Stopped in read_small ($i)"

Let's avoid the trailing " (...)" in test messages/names:

 https://sourceware.org/gdb/wiki/GDBTestcaseCookbook#Do_not_use_.22tail_parentheses.22_on_test_messages

How about for e.g.,:

 "stopped in read_small, $i"

or you can use with_test_prefix, foreach_with_prefix, etc.

> --- a/gdb/testsuite/gdb.dwarf2/implptr.exp
> +++ b/gdb/testsuite/gdb.dwarf2/implptr.exp
> @@ -66,9 +66,13 @@ proc implptr_test_baz {} {
>      gdb_test "break implptr.c:$line" "Breakpoint 3.*" \
>  	"set baz breakpoint for implptr"
>      gdb_continue_to_breakpoint "continue to baz breakpoint for implptr"
> +
> +    # We are breaking in an inlined function.  GDB used to stop in the
> +    # calling frame, but it now stops "in" the inlined function.
> +    gdb_test "up" "#1  foo .*"

I'd suggest avoiding talking about how GDB used to work, because IME that
tends to stay behind for years and years and become more noise
than signal.  I'd suggest something like:

    # We are breaking in an inlined function.  GDB should have stopped
    # "in" the inlined function, not the calling frame.
    gdb_test "up" "#1  foo .*"

>      gdb_test {p p[0].y} " = 92" "sanity check element 0"
>      gdb_test {p p[1].y} " = 46" "sanity check element 1"
> -    gdb_test "step" "\r\nadd \\(.*" "enter the inlined function"
> +    gdb_test "down" "#0  add .*"
>      gdb_test "p a->y" " = 92" "check element 0 for the offset"
>      gdb_test "p b->y" " = 46" "check element 1 for the offset"
>      gdb_continue_to_breakpoint "ignore the second baz breakpoint"
> diff --git a/gdb/testsuite/gdb.opt/inline-break.c b/gdb/testsuite/gdb.opt/inline-break.c


> +set ws {[\r\n\t ]+}
> +set backtrace [list "(in|at)? main"]
> +for {set i 3} {$i > 0} {incr i -1} {
> +
> +    foreach inline {"not_inline" "inline"} {
> +
> +	# Check that we stop at the correct location and print out
> +	# the (possibly) inlined frames.
> +	set num [gdb_get_line_number "/* ${inline}_func$i  */"]
> +	set pattern ".*/$srcfile:$num${ws}.*$num${ws}int y = $decimal;"

Avoid the / before $srcfile for remote host testing.

Thanks,
Pedro Alves