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 3/5] Add support for ARM breakpoint types in GDBServer.

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

	* Add arm-common.o.
	* arm-tdep.c (thumb_insn_size): Move to arm-common.h/c.

"Move to arm-common.c".  Some macros are moved, we should mention the
move here as well.


	* common/arm-common.c: New file.
	* common/arm-common.h: New file.

arm-common.c is not a good file name to me, how about arm-insn.c?

This will contain more stuff as I post the next patch sets for single stepping etc.. I would like to keep a more general name.

It's basically all coming from arm-tdep.c but I don't want to name it common/arm-tdep.c to avoid confusion and Makefile problems.

arm-ctdep.c ?

Please move this file to arch/ directory rather than common/

It seems weird to me to have common objects somewhere else then in common/ , if common becomes more a lib we don't want it all over the place no ?

Would creating a common/arch be acceptable ? I guess we would have to move things like x86-xstate.h etc.. there too ?

	* configure.tgt: Add arm-common.o.
	* gdbserver/ Add arm-common.c/o.

ChangeLog entries for GDBserver should be written separately.

Oops my bad sorry about that.

	* gdbserver/configure.srv: Add arm-common.o.
	* gdbserver/linux-arm-low.c (arm_breakpoint_at): Adjust for
         breakpoint types.
	(arm_breakpoint_from_pc): New function.

arm_breakpoint_from_pc is not a new function, but arm_is_thumb_mode is.


diff --git a/gdb/common/arm-common.c b/gdb/common/arm-common.c
new file mode 100644
index 0000000..861b249
--- /dev/null
+++ b/gdb/common/arm-common.c
@@ -0,0 +1,32 @@
+/* Common code for generic ARM support.
+   Copyright (C) 2015 Free Software Foundation, Inc.

Contents in file are moved from existing file, so we should still keep
the year range of the original file.  It should be

      Copyright (C) 1988-2015 Free Software Foundation, Inc.

OK np

diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index c42b4df..e831f59 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -89,7 +89,7 @@ arm*-wince-pe | arm*-*-mingw32ce*)
  	# Target: ARM based machine running GNU/Linux
-	gdb_target_obs="arm-tdep.o arm-linux-tdep.o glibc-tdep.o \
+	gdb_target_obs="arm-common.o arm-tdep.o arm-linux-tdep.o glibc-tdep.o \
  			solib-svr4.o symfile-mem.o linux-tdep.o linux-record.o"

since arm-common.o is moved out of arm-tdep.o, so it should be added to
every target which uses arm-tdep.o, such as aarch64*-*-linux*,
arm*-wince-pe, etc.

Even if they don't use it ? But ok.

diff --git a/gdb/gdbserver/linux-arm-low.c b/gdb/gdbserver/linux-arm-low.c
index 367c704..15ecb70 100644
--- a/gdb/gdbserver/linux-arm-low.c
+++ b/gdb/gdbserver/linux-arm-low.c
@@ -30,6 +30,8 @@
  #include "nat/gdb_ptrace.h"
  #include <signal.h>

+#include "common/arm-common.h"
  /* Defined in auto-generated files.  */
  void init_registers_arm (void);
  extern const struct target_desc *tdesc_arm;
@@ -234,19 +236,28 @@ arm_set_pc (struct regcache *regcache, CORE_ADDR pc)

  /* Correct in either endianness.  */
-static const unsigned long arm_breakpoint = 0xef9f0001;
-#define arm_breakpoint_len 4
-static const unsigned short thumb_breakpoint = 0xde01;
-static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
+#define arm_abi_breakpoint 0xef9f0001UL

  /* For new EABI binaries.  We recognize it regardless of which ABI
     is used for gdbserver, so single threaded debugging should work
     OK, but for multi-threaded debugging we only insert the current
     ABI's breakpoint instruction.  For now at least.  */
-static const unsigned long arm_eabi_breakpoint = 0xe7f001f0;
+#define arm_eabi_breakpoint 0xe7f001f0UL
+#ifndef __ARM_EABI__
+static const unsigned long arm_breakpoint = arm_abi_breakpoint;
+static const unsigned long arm_breakpoint = arm_eabi_breakpoint;
+#define arm_breakpoint_len 4
+static const unsigned short thumb_breakpoint = 0xde01;
+#define thumb_breakpoint_len 2
+static const unsigned short thumb2_breakpoint[] = { 0xf7f0, 0xa000 };
+#define thumb2_breakpoint_len 4

I am confused by your changes here.  Why do you change them?

Before arm_breakpoint_from_pc would be :

#ifndef __ARM_EABI__
  return (const unsigned char *) &arm_breakpoint;
-  return (const unsigned char *) &arm_eabi_breakpoint;

And arm_breakpoint_at would check for the arm_breakpoint || arm_eabi_breakpoint.

Thus the selected arm_breakpoint would be what that function returned and arm_breakpoint was arm_abi_breakpoint.

It felt more clear to me to name those for what they are : 2 separate entities arm_abi_breakpoint and arm_eabi_breakpoint and set the current used one as arm_breakpoint.

This allows a cleaner breakpoint_from_pc as it just returns arm_breakpoint rather than having the #ifdef in that function.

Any other reference to the arm_breakpoint would also be clear of #ifdefs...

  static int
-arm_breakpoint_at (CORE_ADDR where)
+arm_is_thumb_mode (void)
    struct regcache *regcache = get_thread_regcache (current_thread, 1);
    unsigned long cpsr;
@@ -254,6 +265,17 @@ arm_breakpoint_at (CORE_ADDR where)
    collect_register_by_name (regcache, "cpsr", &cpsr);

    if (cpsr & 0x20)
+      return 1;
+  else
+      return 0;

Indentation looks odd.


+/* Returns 1 if there is a software breakpoint at location.  */
+static int
+arm_breakpoint_at (CORE_ADDR where)
+  if (arm_is_thumb_mode ())
        /* Thumb mode.  */
        unsigned short insn;
@@ -275,7 +297,7 @@ arm_breakpoint_at (CORE_ADDR where)
        unsigned long insn;

        (*the_target->read_memory) (where, (unsigned char *) &insn, 4);
-      if (insn == arm_breakpoint)
+      if (insn == arm_abi_breakpoint)
  	return 1;

        if (insn == arm_eabi_breakpoint)
@@ -285,6 +307,53 @@ arm_breakpoint_at (CORE_ADDR where)
    return 0;

+/* Determine the type and size of breakpoint to insert at PCPTR.  Uses
+   the program counter value to determine whether a 16-bit or 32-bit
+   breakpoint should be used.  It returns a pointer to a string of
+   bytes that encode a breakpoint instruction, stores the length of
+   the string to *lenptr, and adjusts the program counter (if
+   necessary) to point to the actual memory location where the
+   breakpoint should be inserted.  */
+static const unsigned char *
+arm_breakpoint_from_pc (CORE_ADDR *pcptr, int *lenptr)
+  /* Default if no pc is set to arm breakpoint.  */
+  if (pcptr == NULL)

Such NULL checking is no longer needed, right?

Indeed this is gone based on comments in patch 1/5.

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