[binutils-gdb] gdb: introduce bp_loc_tracepoint

[binutils-gdb] gdb: introduce bp_loc_tracepoint

[Simon Marchi]

https://sourceware.org/git/gitweb.cgi?p=binutils-gdb.git;h=0b63c811efc38f50b72422ed6a9db1de053e8736

commit 0b63c811efc38f50b72422ed6a9db1de053e8736
Author: Simon Marchi <simon.marchi@polymtl.ca>
Date:   Fri Nov 25 13:56:14 2022 -0500

    gdb: introduce bp_loc_tracepoint
    
    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

Diff:
---
 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 abee22cd162..51bea46256f 100644
--- a/gdb/breakpoint.c
+++ b/gdb/breakpoint.c
@@ -7359,20 +7359,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 03aecd15eff..7c5cf3f2bef 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...  */
 };