[PATCH] gdb: introduce bp_loc_tracepoint

[PATCH] gdb: introduce bp_loc_tracepoint

[Simon Marchi]

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
-- 
2.38.1

Re: [PATCH] gdb: introduce bp_loc_tracepoint

[Simon Marchi]

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
> -- 
> 2.38.1
> 

Re: [PATCH] gdb: introduce bp_loc_tracepoint

[Simon Marchi]

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

Re: [PATCH] gdb: introduce bp_loc_tracepoint

[Tom Tromey]

>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:

Simon> Because locations for tracepoints have the bp_loc_other type,
Simon> bl_address_is_meaningful started to return false for them, where it
Simon> returned true before.  This made update_global_location_list skip the
Simon> part where it calls swap_insertion.

Simon> I think this can be solved by introduced a new bp_loc_tracepoint
Simon> bp_loc_type.

FWIW this patch looks good to me.

Tom

Re: [PATCH] gdb: introduce bp_loc_tracepoint

[Simon Marchi]

On 1/18/23 12:46, Tom Tromey wrote:
>>>>>> "Simon" == Simon Marchi via Gdb-patches <gdb-patches@sourceware.org> writes:
> 
> Simon> Because locations for tracepoints have the bp_loc_other type,
> Simon> bl_address_is_meaningful started to return false for them, where it
> Simon> returned true before.  This made update_global_location_list skip the
> Simon> part where it calls swap_insertion.
> 
> Simon> I think this can be solved by introduced a new bp_loc_tracepoint
> Simon> bp_loc_type.
> 
> FWIW this patch looks good to me.
> 
> Tom

Thanks, pushed.

Simon