This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

Re: [PATCH 1/5] Support multiple breakpoint types per target in GDBServer.

On 09/23/2015 06:51 AM, Yao Qi wrote:
Antoine Tremblay <> writes:

+static const unsigned char *
+aarch64_breakpoint_from_pc (CORE_ADDR *pcptr, int *len)

Please add comment like this to all these $ARCH_breakpoint_from_pc functions.

/* Implementation of linux_target_ops method "breakpoint_from_pc".  */


diff --git a/gdb/gdbserver/linux-cris-low.c b/gdb/gdbserver/linux-cris-low.c
index e0bfa1a..da5876d 100644
--- a/gdb/gdbserver/linux-cris-low.c
+++ b/gdb/gdbserver/linux-cris-low.c
@@ -62,7 +62,7 @@ cris_cannot_fetch_register (int regno)
  extern int debug_threads;

  static CORE_ADDR
-cris_get_pc (struct regcache *regcache, void)
+cris_get_pc (struct regcache *regcache)
    unsigned long pc;
    collect_register_by_name (regcache, "pc", &pc);

This is a unrelated change.  Please move it out this patch, and submit
it separately.


diff --git a/gdb/gdbserver/linux-low.c b/gdb/gdbserver/linux-low.c
index f5b64ab..bb08761 100644
--- a/gdb/gdbserver/linux-low.c
+++ b/gdb/gdbserver/linux-low.c
@@ -3012,7 +3012,11 @@ linux_wait_1 (ptid_t ptid,
    if (!ptid_equal (step_over_bkpt, null_ptid)
        && event_child->stop_reason == TARGET_STOPPED_BY_SW_BREAKPOINT)
-      unsigned int increment_pc = the_low_target.breakpoint_len;
+      int increment_pc = 0;
+      CORE_ADDR stop_pc = event_child->stop_pc;
+      (*the_low_target.breakpoint_from_pc)
+	(&stop_pc, &increment_pc);

There was a problem here and your patch doesn't fix it.  I want to raise
it here first.  It is incorrect to get increment_pc by
the_low_target.breakpoint_len or (*the_low_target.breakpoint_from_pc)
for arm/thumb, because given the stop_pc, we can't tell the breakpoint
size (2-byte or 4-byte).We need a new target hook, say
breakpoint_from_current_state, and its default implementation is
breakpoint_from_pc.  However, its arm implementation checks whether
the program is in thumb mode by CPSR and return the right breakpoint size.

IIUC, the code here is used for step-over GDBserver breakpoint, so it is
not used for arm target until we support conditional breakpoint or
tracepoint, but we should fix it before supporting conditional
breakpoint and tracepoint for arm target.

Indeed good point. It's too bad another target hook needs to be there for this but there's no choice I think.

I will fix it in my next patchset introducing conditional breakpoints.

        if (debug_threads)
@@ -6932,6 +6936,17 @@ current_lwp_ptid (void)
    return ptid_of (current_thread);

+const unsigned char *
+linux_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
+  if (the_low_target.breakpoint_from_pc != NULL)
+    {
+      return (*the_low_target.breakpoint_from_pc) (pcptr, lenptr);
+    }

"{" and "}" is not needed.


diff --git a/gdb/gdbserver/linux-low.h b/gdb/gdbserver/linux-low.h
index f8f6e78..c623150 100644
--- a/gdb/gdbserver/linux-low.h
+++ b/gdb/gdbserver/linux-low.h
@@ -141,8 +141,13 @@ struct linux_target_ops

    CORE_ADDR (*get_pc) (struct regcache *regcache);
    void (*set_pc) (struct regcache *regcache, CORE_ADDR newpc);
-  const unsigned char *breakpoint;
-  int breakpoint_len;
+  /* Return the raw breakpoint for this target based on PC.  Note that the PC


+     can be NULL, the default breakpoint for the target should be returned in

PC can't be NULL after your patch #2.  You can remove the second
sentence in this patch or patch #2.

I think you mean after patch #3 ?

But it can still be NULL see in #3 :

/* Default if no pc is set to arm breakpoint.  */
+  if (pcptr == NULL)
+    {
+      *lenptr = arm_breakpoint_len;
+      return (unsigned char *) &arm_breakpoint;
+    }

+     this case. The PC is ajusted to the real memory location in case a flag was
+     present in the PC.  */
+  const unsigned char *(*breakpoint_from_pc) (CORE_ADDR *pcptr, int *lenptr);

Note also that I removed the win32 changes, they were not needed and broken, I can't test win32 properly anyway.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]