This is the mail archive of the gdb-patches@sourceware.org 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 <antoine.tremblay@ericsson.com> writes:

gdb/ChangeLog:
	* Makefile.in: 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.

Done.


	* 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/Makefile.in: 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.


Done.

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*)
  	;;
  arm*-*-linux*)
  	# 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"
  	build_gdbserver=yes

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;
+#else
+static const unsigned long arm_breakpoint = arm_eabi_breakpoint;
+#endif
+
+#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;
#else
-  return (const unsigned char *) &arm_eabi_breakpoint;
#endif

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.

Done.


+}
+
+/* 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]