Re: [PATCH] gdb: introduce bp_loc_tracepoint

[Simon Marchi <simon.marchi@polymtl.ca>]

Ping.

On 11/25/22 13:56, Simon Marchi wrote:
> Since commit cb1e4e32c2d9 ("catch catch/throw/rethrow", breakpoint ->
> catchpoint), this simple tracing scenario does not work:
> 
>     $ gdb/gdb -nx -q --data-directory=gdb/data-directory ./test
>     Reading symbols from ./test...
>     (gdb) tar rem :1234
>     Remote debugging using :1234
>     Reading symbols from /lib64/ld-linux-x86-64.so.2...
>     (No debugging symbols found in /lib64/ld-linux-x86-64.so.2)
>     0x00007ffff7fe5730 in ?? () from /lib64/ld-linux-x86-64.so.2
>     (gdb) trace do_something
>     Tracepoint 1 at 0x555555555144: file test.c, line 5.
>     (gdb) tstart
>     (gdb) continue
>     Continuing.
>     Target returns error code '01'.
> 
> The root cause is that the bp_location::nserted flag does not transfer
> anymore from an old bp_location to the new matching one.  When a shared
> library gets loaded, GDB computes new breakpoint locations for each
> breakpoint in update_breakpoint_locations.  The new locations are in the
> breakpoint::loc chain, while the old locations are still in the
> bp_locations global vector.  Later, update_global_location_list is
> called.  It tries to map old locations to new locations, and if
> necessary transfer some properties, like the inserted flag.
> 
> Since commit cb1e4e32c2d9, the inserted flag isn't transferred for
> locations of tracepoints.  This is because bl_address_is_meaningful used
> to be implemented like this:
> 
>     static int
>     breakpoint_address_is_meaningful (struct breakpoint *bpt)
>     {
>       enum bptype type = bpt->type;
> 
>       return (type != bp_watchpoint && type != bp_catchpoint);
>     }
> 
> and was changed to this:
> 
>     static bool
>     bl_address_is_meaningful (bp_location *loc)
>     {
>       return loc->loc_type != bp_loc_other;
>     }
> 
> Because locations for tracepoints have the bp_loc_other type,
> bl_address_is_meaningful started to return false for them, where it
> returned true before.  This made update_global_location_list skip the
> part where it calls swap_insertion.
> 
> I think this can be solved by introduced a new bp_loc_tracepoint
> bp_loc_type.
> 
> I don't know if it's accurate, but my understanding is that bp_loc_type
> describes roughly "how do we ask the target to insert that location".
> bp_loc_software_breakpoint are inserted using
> target_ops::insert_breakpoint_location.  bp_loc_hardware_breakpoint are
> inserted using target_ops::insert_hw_breakpoint.
> bp_loc_software_watchpoint and bp_loc_hardware_watchpoint are inserted
> using target_ops::insert_watchpoint.  For all these, the address is
> meaningful, as we ask the target to insert the point at a specific
> address.  bp_loc_other is a catch-all for "the rest", in practice for
> catchpoints that don't have a specific address (hence why
> bl_address_is_meaningful returns false for them).  For instance,
> inserting a signal catchpoint is done by asking the target to report
> that specific signal.  GDB doesn't associate an address to that.
> 
> But tracepoints do have a meaningful address to thems, so they can't be
> bp_loc_other, with that logic.  They also can't be
> bp_loc_software_breakpoint, because we don't want GDB to insert
> breakpoints for them (even though they might be implemented using
> software breakpoints by the remote side).  So, the new bp_loc_tracepoint
> type describes that the way to insert these locations is with
> target_ops::download_tracepoint.  It makes bl_address_is_meaningful
> return true for them.  And they'll be ignored by insert_bp_location and
> GDB won't try to insert a memory breakpoint for them.
> 
> With this, I see a few instances of 'Target returns error code: 01'
> disappearing from gdb.log, and the results of gdb.trace/*.exp improve a
> little bit:
> 
>     -# of expected passes       3765
>     +# of expected passes       3781
>     -# of unexpected failures   518
>     +# of unexpected failures   498
> 
> Things remain quite broken in that area though.
> 
> Change-Id: Ic40935c450410f4bfaba397c9ebc7faf97320dd3
> ---
>  gdb/breakpoint.c | 9 ++++++++-
>  gdb/breakpoint.h | 1 +
>  2 files changed, 9 insertions(+), 1 deletion(-)
> 
> diff --git a/gdb/breakpoint.c b/gdb/breakpoint.c
> index f0276a963c00..caa7d325b12f 100644
> --- a/gdb/breakpoint.c
> +++ b/gdb/breakpoint.c
> @@ -7345,20 +7345,27 @@ bp_location_from_bp_type (bptype type)
>      case bp_gnu_ifunc_resolver_return:
>      case bp_dprintf:
>        return bp_loc_software_breakpoint;
> +
>      case bp_hardware_breakpoint:
>        return bp_loc_hardware_breakpoint;
> +
>      case bp_hardware_watchpoint:
>      case bp_read_watchpoint:
>      case bp_access_watchpoint:
>        return bp_loc_hardware_watchpoint;
> +
>      case bp_watchpoint:
>        return bp_loc_software_watchpoint;
> -    case bp_catchpoint:
> +
>      case bp_tracepoint:
>      case bp_fast_tracepoint:
>      case bp_static_tracepoint:
>      case bp_static_marker_tracepoint:
> +      return bp_loc_tracepoint;
> +
> +    case bp_catchpoint:
>        return bp_loc_other;
> +
>      default:
>        internal_error (_("unknown breakpoint type"));
>      }
> diff --git a/gdb/breakpoint.h b/gdb/breakpoint.h
> index f7633d29cbfd..661527a9e1a0 100644
> --- a/gdb/breakpoint.h
> +++ b/gdb/breakpoint.h
> @@ -317,6 +317,7 @@ enum bp_loc_type
>    bp_loc_hardware_breakpoint,
>    bp_loc_software_watchpoint,
>    bp_loc_hardware_watchpoint,
> +  bp_loc_tracepoint,
>    bp_loc_other			/* Miscellaneous...  */
>  };
>  
> 
> base-commit: 8654c01f085f77e443185d0a61a106880bb060b9