From 0b63c811efc38f50b72422ed6a9db1de053e8736 Mon Sep 17 00:00:00 2001
From: Simon Marchi <simon.marchi@polymtl.ca>
Date: Fri, 25 Nov 2022 13:56:14 -0500
Subject: [PATCH] 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
---
 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...  */
 };