Re: [PATCH 3/5] Add support for ARM breakpoint types in GDBServer.

[Antoine Tremblay <antoine.tremblay@ericsson.com>]

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.