RE: [patch v8 17/24] record-btrace: provide xfer_partial target method

["Metzger, Markus T" <markus dot t dot metzger at intel dot com>]

> -----Original Message-----
> From: Pedro Alves [mailto:palves@redhat.com]
> Sent: Monday, December 16, 2013 8:16 PM


> > I changed the return -1 to throw_error (...) and added a check for
> > writebuf != NULL.  Suddenly I got tons of errors when GDB can't insert
> > breakpoints any longer for (reverse-)stepping.
> 
> This is why precord keeps track of breakpoints itself too:
[...] 
> >   Also stepping gets broken.
> 
> I can't immediately why that would be.

Because we can't set temporary breakpoints.


> > I now get an error when trying to access a variable with static storage
> > duration or when trying to access memory directly via its address.
> > It would be nice to also get an <unavailable> in those cases.  This would
> > require the respective layer to catch my exception.
> 
> Please try returning TARGET_XFER_E_UNAVAILABLE instead.

That is ignored just like the -1 I returned earlier.  I nevertheless changed
the default return to that since it is more descriptive.


> > To avoid those errors when trying to set breakpoints, I could try
> > providing to_insert_breakpoint and to_remove_breakpoint methods
> > and maintain my own breakpoints.
> 
> Right.

I have something to temporarily disable the xfer checks during
to_insert_breakpoint and to_remove_breakpoint.

Not sure whether this is considered too hacky or what else I'm missing.
My tests all pass.  Any idea where else GDB would need to access
target memory in order to function correctly?


Here's the patch.  I omit a preparation patch to pass target_ops to
to_insert_breakpoint and to_remove_breakpoint so that the request
can be forwarded to the target beneath.

diff --git a/gdb/record-btrace.c b/gdb/record-btrace.c
index 00a056d..0536071 100644
--- a/gdb/record-btrace.c
+++ b/gdb/record-btrace.c
@@ -42,6 +42,9 @@ static struct target_ops record_btrace_ops;
 /* A new thread observer enabling branch tracing for the new thread.  */
 static struct observer *record_btrace_thread_observer;
 
+/* Temporarily allow memory accesses.  */
+static int record_btrace_allow_memory_access;
+
 /* Print a record-btrace debug message.  Use do ... while (0) to avoid
    ambiguities when used in if statements.  */
 
@@ -805,7 +808,7 @@ record_btrace_xfer_partial (struct target_ops *ops, enum target_object object,
   struct target_ops *t;
 
   /* Filter out requests that don't make sense during replay.  */
-  if (record_btrace_is_replaying ())
+  if (record_btrace_allow_memory_access == 0 && record_btrace_is_replaying ())
     {
       switch (object)
 	{
@@ -850,6 +853,58 @@ record_btrace_xfer_partial (struct target_ops *ops, enum target_object object,
   return TARGET_XFER_E_UNAVAILABLE;
 }
 
+/* The to_insert_breakpoint method of target record-btrace.  */
+
+static int
+record_btrace_insert_breakpoint (struct target_ops *ops,
+				 struct gdbarch *gdbarch,
+				 struct bp_target_info *bp_tgt)
+{
+  volatile struct gdb_exception except;
+  int old, ret;
+
+  /* Inserting breakpoints requires accessing memory.  Allow it for the
+     duration of this function.  */
+  old = record_btrace_allow_memory_access;
+  record_btrace_allow_memory_access = 1;
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    ret = forward_target_insert_breakpoint (ops->beneath, gdbarch, bp_tgt);
+
+  record_btrace_allow_memory_access = old;
+
+  if (except.reason < 0)
+    throw_exception (except);
+
+  return ret;
+}
+
+/* The to_remove_breakpoint method of target record-btrace.  */
+
+static int
+record_btrace_remove_breakpoint (struct target_ops *ops,
+				 struct gdbarch *gdbarch,
+				 struct bp_target_info *bp_tgt)
+{
+  volatile struct gdb_exception except;
+  int old, ret;
+
+  /* Removing breakpoints requires accessing memory.  Allow it for the
+     duration of this function.  */
+  old = record_btrace_allow_memory_access;
+  record_btrace_allow_memory_access = 1;
+
+  TRY_CATCH (except, RETURN_MASK_ALL)
+    ret = forward_target_remove_breakpoint (ops->beneath, gdbarch, bp_tgt);
+
+  record_btrace_allow_memory_access = old;
+
+  if (except.reason < 0)
+    throw_exception (except);
+
+  return ret;
+}
+
 /* The to_fetch_registers method of target record-btrace.  */
 
 static void
@@ -1804,6 +1859,8 @@ init_record_btrace_ops (void)
   ops->to_call_history_range = record_btrace_call_history_range;
   ops->to_record_is_replaying = record_btrace_is_replaying;
   ops->to_xfer_partial = record_btrace_xfer_partial;
+  ops->to_remove_breakpoint = record_btrace_remove_breakpoint;
+  ops->to_insert_breakpoint = record_btrace_insert_breakpoint;
   ops->to_fetch_registers = record_btrace_fetch_registers;
   ops->to_store_registers = record_btrace_store_registers;
   ops->to_prepare_to_store = record_btrace_prepare_to_store;

Regards,
Markus.

Intel GmbH
Dornacher Strasse 1
85622 Feldkirchen/Muenchen, Deutschland
Sitz der Gesellschaft: Feldkirchen bei Muenchen
Geschaeftsfuehrer: Christian Lamprechter, Hannes Schwaderer, Douglas Lusk
Registergericht: Muenchen HRB 47456
Ust.-IdNr./VAT Registration No.: DE129385895
Citibank Frankfurt a.M. (BLZ 502 109 00) 600119052