[patch] gdbarch_syscall_pc_increment

[patch] gdbarch_syscall_pc_increment

[Aleksandar Ristovski]

[-- Attachment #1: Type: text/plain, Size: 602 bytes --]

Hello all,


This patch fixes stepping over system call instruction on architectures 
that use software single stepping and also may increment PC upon return 
from the system call to communicate error. (e.g. this is what we do on 
Neutrino).

Note: I could only test this on Neutrino.


Thanks,

Aleksandar


ChangeLog:

       * gdbarch.sh (syscall_pc_increment): New function.
       * gdbarch.h, gdbarch.c: Regenerated.
       * arm-tdep.c (arm_software_single_step): Use
       gdbarch_syscall_pc_increment and if provided, insert second
       single step breakpoint at the incremented address.



[-- Attachment #2: gdbarch_syscall_pc_increment-201212111513.patch --]
[-- Type: text/x-patch, Size: 6822 bytes --]

Index: gdb/gdbarch.sh
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.sh,v
retrieving revision 1.550
diff -u -p -r1.550 gdbarch.sh
--- gdb/gdbarch.sh	21 Nov 2012 00:29:54 -0000	1.550
+++ gdb/gdbarch.sh	11 Dec 2012 20:13:20 -0000
@@ -798,6 +798,10 @@ M:void:record_special_symbol:struct objf
 # Get architecture-specific system calls information from registers.
 M:LONGEST:get_syscall_number:ptid_t ptid:ptid
 
+# Return number of bytes the architecture may increment PC after
+# syscall.
+M:int:syscall_pc_increment:const struct frame_info *frame:frame
+
 # SystemTap related fields and functions.
 
 # Prefix used to mark an integer constant on the architecture's assembly
Index: gdb/gdbarch.h
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.h,v
retrieving revision 1.449
diff -u -p -r1.449 gdbarch.h
--- gdb/gdbarch.h	21 Nov 2012 00:29:54 -0000	1.449
+++ gdb/gdbarch.h	11 Dec 2012 20:13:20 -0000
@@ -611,7 +611,7 @@ extern void set_gdbarch_addr_bits_remove
    FIXME/cagney/2001-01-18: The logic is backwards.  It should be asking if the
    target can single step.  If not, then implement single step using breakpoints.
   
-   A return value of 1 means that the software_single_step breakpoints
+   A return value of 1 means that the software_single_step breakpoints 
    were inserted; 0 means they were not. */
 
 extern int gdbarch_software_single_step_p (struct gdbarch *gdbarch);
@@ -998,6 +998,15 @@ typedef LONGEST (gdbarch_get_syscall_num
 extern LONGEST gdbarch_get_syscall_number (struct gdbarch *gdbarch, ptid_t ptid);
 extern void set_gdbarch_get_syscall_number (struct gdbarch *gdbarch, gdbarch_get_syscall_number_ftype *get_syscall_number);
 
+/* Return number of bytes the architecture may increment PC after
+   syscall. */
+
+extern int gdbarch_syscall_pc_increment_p (struct gdbarch *gdbarch);
+
+typedef int (gdbarch_syscall_pc_increment_ftype) (struct gdbarch *gdbarch, const struct frame_info *frame);
+extern int gdbarch_syscall_pc_increment (struct gdbarch *gdbarch, const struct frame_info *frame);
+extern void set_gdbarch_syscall_pc_increment (struct gdbarch *gdbarch, gdbarch_syscall_pc_increment_ftype *syscall_pc_increment);
+
 /* SystemTap related fields and functions.
    Prefix used to mark an integer constant on the architecture's assembly
    For example, on x86 integer constants are written as:
Index: gdb/gdbarch.c
===================================================================
RCS file: /cvs/src/src/gdb/gdbarch.c,v
retrieving revision 1.501
diff -u -p -r1.501 gdbarch.c
--- gdb/gdbarch.c	21 Nov 2012 00:29:54 -0000	1.501
+++ gdb/gdbarch.c	11 Dec 2012 20:13:20 -0000
@@ -263,6 +263,7 @@ struct gdbarch
   gdbarch_get_siginfo_type_ftype *get_siginfo_type;
   gdbarch_record_special_symbol_ftype *record_special_symbol;
   gdbarch_get_syscall_number_ftype *get_syscall_number;
+  gdbarch_syscall_pc_increment_ftype *syscall_pc_increment;
   const char * stap_integer_prefix;
   const char * stap_integer_suffix;
   const char * stap_register_prefix;
@@ -431,6 +432,7 @@ struct gdbarch startup_gdbarch =
   0,  /* get_siginfo_type */
   0,  /* record_special_symbol */
   0,  /* get_syscall_number */
+  0,  /* syscall_pc_increment */
   0,  /* stap_integer_prefix */
   0,  /* stap_integer_suffix */
   0,  /* stap_register_prefix */
@@ -731,6 +733,7 @@ verify_gdbarch (struct gdbarch *gdbarch)
   /* Skip verify of get_siginfo_type, has predicate.  */
   /* Skip verify of record_special_symbol, has predicate.  */
   /* Skip verify of get_syscall_number, has predicate.  */
+  /* Skip verify of syscall_pc_increment, has predicate.  */
   /* Skip verify of stap_integer_prefix, invalid_p == 0 */
   /* Skip verify of stap_integer_suffix, invalid_p == 0 */
   /* Skip verify of stap_register_prefix, invalid_p == 0 */
@@ -1345,6 +1348,12 @@ gdbarch_dump (struct gdbarch *gdbarch, s
                       "gdbarch_dump: static_transform_name = <%s>\n",
                       host_address_to_string (gdbarch->static_transform_name));
   fprintf_unfiltered (file,
+                      "gdbarch_dump: gdbarch_syscall_pc_increment_p() = %d\n",
+                      gdbarch_syscall_pc_increment_p (gdbarch));
+  fprintf_unfiltered (file,
+                      "gdbarch_dump: syscall_pc_increment = <%s>\n",
+                      host_address_to_string (gdbarch->syscall_pc_increment));
+  fprintf_unfiltered (file,
                       "gdbarch_dump: target_desc = %s\n",
                       host_address_to_string (gdbarch->target_desc));
   fprintf_unfiltered (file,
@@ -3890,6 +3899,30 @@ set_gdbarch_get_syscall_number (struct g
   gdbarch->get_syscall_number = get_syscall_number;
 }
 
+int
+gdbarch_syscall_pc_increment_p (struct gdbarch *gdbarch)
+{
+  gdb_assert (gdbarch != NULL);
+  return gdbarch->syscall_pc_increment != NULL;
+}
+
+int
+gdbarch_syscall_pc_increment (struct gdbarch *gdbarch, const struct frame_info *frame)
+{
+  gdb_assert (gdbarch != NULL);
+  gdb_assert (gdbarch->syscall_pc_increment != NULL);
+  if (gdbarch_debug >= 2)
+    fprintf_unfiltered (gdb_stdlog, "gdbarch_syscall_pc_increment called\n");
+  return gdbarch->syscall_pc_increment (gdbarch, frame);
+}
+
+void
+set_gdbarch_syscall_pc_increment (struct gdbarch *gdbarch,
+                                  gdbarch_syscall_pc_increment_ftype syscall_pc_increment)
+{
+  gdbarch->syscall_pc_increment = syscall_pc_increment;
+}
+
 const char *
 gdbarch_stap_integer_prefix (struct gdbarch *gdbarch)
 {
Index: gdb/arm-tdep.c
===================================================================
RCS file: /cvs/src/src/gdb/arm-tdep.c,v
retrieving revision 1.373
diff -u -p -r1.373 arm-tdep.c
--- gdb/arm-tdep.c	21 Nov 2012 00:29:54 -0000	1.373
+++ gdb/arm-tdep.c	11 Dec 2012 20:13:21 -0000
@@ -5242,6 +5242,31 @@ arm_software_single_step (struct frame_i
   next_pc = arm_get_next_pc (frame, get_frame_pc (frame));
   arm_insert_single_step_breakpoint (gdbarch, aspace, next_pc);
 
+  if (gdbarch_syscall_pc_increment_p (gdbarch))
+    {
+      /* Some OS-es will increment pc to differentiate success vs
+       * error returning from a sys call. */
+      const int inc = gdbarch_syscall_pc_increment (gdbarch, frame);
+      const enum bfd_endian byte_order_for_code
+	= gdbarch_byte_order_for_code (gdbarch);
+      const LONGEST this_pc = get_frame_pc (frame);
+      LONGEST insn, mask, match;
+
+      if (arm_frame_is_thumb (frame))
+	{
+	  mask = 0xff000000;
+	  match = 0xdf000000;
+	}
+      else
+	{
+	  mask = 0x0f000000;
+	  match = 0x0f000000;
+	}
+      if (safe_read_memory_integer (this_pc, 4, byte_order_for_code, &insn)
+	  && (insn & mask) == match)
+	  arm_insert_single_step_breakpoint (gdbarch, aspace, next_pc + inc);
+    }
+
   return 1;
 }
 

Re: [patch] gdbarch_syscall_pc_increment

[Yao Qi]

On 12/12/2012 10:23 PM, Aleksandar Ristovski wrote:
> This patch fixes stepping over system call instruction on architectures
> that use software single stepping and also may increment PC upon return
> from the system call to communicate error. (e.g. this is what we do on
> Neutrino).
>
> Note: I could only test this on Neutrino.
>

Hi,
I don't understand why do you add a gdbarch hook, but use it only in a 
target-specific part?  The goal of gdbarch hooks is about hiding the 
difference of ports and giving a common interface to the common part of 
GDB.  If your issue is arm specific, we don't need this new gdbarch hook 
at all.

If I understand your problem correctly, you have to define your own 
function 'arm_neutrino_syscall_next_pc' in your file 
arm-neutrino-tdep.c, and install it on function pointer 
'syscall_next_pc' (in 'struct gdbarch_tdep' in arm-tdep.h) in 
'arm_neutrino_init_abi'.  Please have a look on how 'syscall_next_pc' is 
set in arm-linux-tdep.c.  Then you can compute the pc for your own os in 
'arm_neutrino_syscall_next_pc'.  Hope it helps.

>
> ChangeLog:
>
>         * gdbarch.sh (syscall_pc_increment): New function.
>         * gdbarch.h, gdbarch.c: Regenerated.
>         * arm-tdep.c (arm_software_single_step): Use
>         gdbarch_syscall_pc_increment and if provided, insert second
>         single step breakpoint at the incremented address.

I guess the reason you need the 2nd single step breakpoint is that GDB 
computes the 'next pc' by mistake, so you need the 2nd single step 
breakpoint setting on the 'right' address of 'next pc'.  In other words, 
do we really need the 2nd single step breakpoint if the address of 'next 
pc' is computed correctly?

-- 
Yao (齐尧)

Re: [patch] gdbarch_syscall_pc_increment

[Aleksandar Ristovski]

On 12-12-12 10:30 AM, Yao Qi wrote:
> Hi,
> I don't understand why do you add a gdbarch hook, but use it only in a
> target-specific part?  The goal of gdbarch hooks is about hiding the
> difference of ports and giving a common interface to the common part of
> GDB.  If your issue is arm specific, we don't need this new gdbarch hook
> at all.


This is generic for a given OS that happens to increment instruction 
pointer to allow user code to e.g. set errno.

I provided only arm implementation, but other target cpus would need the 
same if they implement software single stepping.

Increment is cpu specific for a given architecture.


>
> If I understand your problem correctly, you have to define your own
> function 'arm_neutrino_syscall_next_pc' in your file
> arm-neutrino-tdep.c, and install it on function pointer
> 'syscall_next_pc' (in 'struct gdbarch_tdep' in arm-tdep.h) in
> 'arm_neutrino_init_abi'.  Please have a look on how 'syscall_next_pc' is
> set in arm-linux-tdep.c.  Then you can compute the pc for your own os in
> 'arm_neutrino_syscall_next_pc'.  Hope it helps.


No, the destination is not a single address as we do not know the 
outcome of the syscall. It may come back with the instruction pointer of 
the next instruction after 'svc' but also 4 bytes later (4 bytes in our 
case, some other kernel may implement it differently).


Hope this clarifies,

Aleksandar

Re: [patch] gdbarch_syscall_pc_increment

[Yao Qi]

On 12/12/2012 11:42 PM, Aleksandar Ristovski wrote:
>
> This is generic for a given OS that happens to increment instruction
> pointer to allow user code to e.g. set errno.
>
> I provided only arm implementation, but other target cpus would need the
> same if they implement software single stepping.
>
> Increment is cpu specific for a given architecture.
>

'software single step' is implemented differently in the backend of each 
port and your 'syscall_pc_increment' depends on the arch as well,  so a 
gdbarch hook is not needed here.

'gdbarch' stands for a certain general architecture, such as arm, mips, 
and etc.  'gdbarch_tdep' contains the details of the cpus under this 
architecture.

>
>> >
>> >If I understand your problem correctly, you have to define your own
>> >function 'arm_neutrino_syscall_next_pc' in your file
>> >arm-neutrino-tdep.c, and install it on function pointer
>> >'syscall_next_pc' (in 'struct gdbarch_tdep' in arm-tdep.h) in
>> >'arm_neutrino_init_abi'.  Please have a look on how 'syscall_next_pc' is
>> >set in arm-linux-tdep.c.  Then you can compute the pc for your own os in
>> >'arm_neutrino_syscall_next_pc'.  Hope it helps.
>
> No, the destination is not a single address as we do not know the
> outcome of the syscall. It may come back with the instruction pointer of
> the next instruction after 'svc' but also 4 bytes later (4 bytes in our
> case, some other kernel may implement it differently).

You may need a specified field in 'struct gdbarch_tdep', for example,

   /* Do a post fix of a software single step.  */
   void (*software_single_step_fixup) (struct frame_info *frame);

In arm_software_single_step,

@@ -5242,6 +5242,31 @@ arm_software_single_step (struct frame_i
    next_pc = arm_get_next_pc (frame, get_frame_pc (frame));
    arm_insert_single_step_breakpoint (gdbarch, aspace, next_pc);

+  tdep = gdbarch_tdep (gdbarch);
+  if (tdep->software_single_step_fixup)
+    tdep->software_single_step_fixup (frame);
+

and you need to initialize field 'software_single_step_fixup' somewhere, 
and do what you need in it.

b.t.w, I don't see how 'set_gdbarch_syscall_pc_increment' is called in 
your patch.

-- 
Yao (齐尧)

Re: [patch] gdbarch_syscall_pc_increment

[Aleksandar Ristovski]

On 12-12-12 08:32 PM, Yao Qi wrote:
> On 12/12/2012 11:42 PM, Aleksandar Ristovski wrote:
>>
>> This is generic for a given OS that happens to increment instruction
>> pointer to allow user code to e.g. set errno.
>>
>> I provided only arm implementation, but other target cpus would need the
>> same if they implement software single stepping.
>>
>> Increment is cpu specific for a given architecture.
>>
>
> 'software single step' is implemented differently in the backend of each
> port and your 'syscall_pc_increment' depends on the arch as well,  so a
> gdbarch hook is not needed here.
>
> 'gdbarch' stands for a certain general architecture, such as arm, mips,
> and etc.  'gdbarch_tdep' contains the details of the cpus under this
> architecture.

Yes, I see your point, though I would say gdbarch stands for a certain 
architecture such as arm-nto, arm-linux, etc... therefore, cpu/os 
combination, not cpu only.

In this patch, we are talking about two things:

a) the concept of syscall behaving as a conditional branch with 
uncomputable condition (i.e. gdb can not really compute whether the 
branch will be taken or not). This is a concept specific to a given 
kernel implementation of system calls and as such belongs to gdbarch.

b) The amount by which pc may be incremented if the branch is taken. For 
an os that has a), this is cpu specific. It may also be dependent on the 
instruction set mode on e.g. arm (this is why frame is being passed).

Where does this belong: in the case here, the line is a bit blurry since 
a) is generic concept applicable for all cpus for a given os thus 
definitely belonging to gdbarch. b) is cpu concept and strictly speaking 
belongs to gdbarch_tdep. However, since a) can also answer b) I see no 
point in further breaking it down.

Of course, the alternative is to not introduce concept from a) at all 
and do everything in my cpu-os-tdep.c which is how I have it set up now 
in our port.

>
> b.t.w, I don't see how 'set_gdbarch_syscall_pc_increment' is called in
> your patch.
>

No, as we have not yet contributed arm-nto-tdep.c to FSF.

---
Aleksandar

Re: [patch] gdbarch_syscall_pc_increment

[Yao Qi]

On 12/13/2012 10:07 PM, Aleksandar Ristovski wrote:
> Yes, I see your point, though I would say gdbarch stands for a certain
> architecture such as arm-nto, arm-linux, etc... therefore, cpu/os
> combination, not cpu only.

Yeah, that is more precise.  We do have some gdbarch hook methods, such 
as gdbarch_skip_trampoline_code, gdbarch_get_syscall_number, 
gdbarch_skip_solib_resolver, they are about the cpu/os (or runtime lib) 
combination.

-- 
Yao (齐尧)

Re: [patch] gdbarch_syscall_pc_increment

[Pedro Alves]

On 12/13/2012 02:07 PM, Aleksandar Ristovski wrote:

> In this patch, we are talking about two things:
> 
> a) the concept of syscall behaving as a conditional branch with uncomputable condition (i.e. gdb can not really compute whether the branch will be taken or not). This is a concept specific to a given kernel implementation of system calls and as such belongs to gdbarch.
> 
> b) The amount by which pc may be incremented if the branch is taken. For an os that has a), this is cpu specific. It may also be dependent on the instruction set mode on e.g. arm (this is why frame is being passed).
> 
> Where does this belong: in the case here, the line is a bit blurry since a) is generic concept applicable for all cpus for a given os thus definitely belonging to gdbarch. b) is cpu concept and strictly speaking belongs to gdbarch_tdep. However, since a) can also answer b) I see no point in further breaking it down.

The interface is not pretty, but you can just install an additional
breakpoint from your OS's tdep->syscall_next_pc hook or
gdbarch_software_single_step wrapper hook.  See e.g.,:

/* Find the next PC after the current instruction executes.  In some
   cases we can not statically determine the answer (see the IT state
   handling in this function); in that case, a breakpoint may be
   inserted in addition to the returned PC, which will be used to set
   another breakpoint by our caller.  */

static CORE_ADDR
thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
{
...
	      /* Set a breakpoint on the following instruction.  */
	      gdb_assert ((itstate & 0x0f) != 0);
	      arm_insert_single_step_breakpoint (gdbarch, aspace,
						 MAKE_THUMB_ADDR (pc));
...

I believe other ports do similar things in similar cases.

>> b.t.w, I don't see how 'set_gdbarch_syscall_pc_increment' is called in
>> your patch.
>>
> 
> No, as we have not yet contributed arm-nto-tdep.c to FSF.

When do you plan to do so?  I'm afraid we shouldn't be accepting
unused hooks in the tree.

-- 
Pedro Alves

Re: [patch] gdbarch_syscall_pc_increment

[Aleksandar Ristovski]

On 12-12-13 04:47 PM, Pedro Alves wrote:
> On 12/13/2012 02:07 PM, Aleksandar Ristovski wrote:
>
>> In this patch, we are talking about two things:
>>
>> a) the concept of syscall behaving as a conditional branch with uncomputable condition (i.e. gdb can not really compute whether the branch will be taken or not). This is a concept specific to a given kernel implementation of system calls and as such belongs to gdbarch.
>>
>> b) The amount by which pc may be incremented if the branch is taken. For an os that has a), this is cpu specific. It may also be dependent on the instruction set mode on e.g. arm (this is why frame is being passed).
>>
>> Where does this belong: in the case here, the line is a bit blurry since a) is generic concept applicable for all cpus for a given os thus definitely belonging to gdbarch. b) is cpu concept and strictly speaking belongs to gdbarch_tdep. However, since a) can also answer b) I see no point in further breaking it down.
>
> The interface is not pretty, but you can just install an additional
> breakpoint from your OS's tdep->syscall_next_pc hook or
> gdbarch_software_single_step wrapper hook.  See e.g.,:
>
> /* Find the next PC after the current instruction executes.  In some
>     cases we can not statically determine the answer (see the IT state
>     handling in this function); in that case, a breakpoint may be
>     inserted in addition to the returned PC, which will be used to set
>     another breakpoint by our caller.  */
>
> static CORE_ADDR
> thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
> {
> ...
> 	      /* Set a breakpoint on the following instruction.  */
> 	      gdb_assert ((itstate & 0x0f) != 0);
> 	      arm_insert_single_step_breakpoint (gdbarch, aspace,
> 						 MAKE_THUMB_ADDR (pc));
> ...
>
> I believe other ports do similar things in similar cases.


This seems just wrong to me, to install a breakpoint when answering a 
question.


>
>>> b.t.w, I don't see how 'set_gdbarch_syscall_pc_increment' is called in
>>> your patch.
>>>
>>
>> No, as we have not yet contributed arm-nto-tdep.c to FSF.
>
> When do you plan to do so?  I'm afraid we shouldn't be accepting
> unused hooks in the tree.
>

Well, I plan to, but can't provide any ETA.

No problem - there is no urgency in this, at least we have exchanged 
thoughts on where would this fit (it doesn't fit anywhere ATM :-) ). 
I'll stick with providing my own software_single_step.


Consider the patch abandoned.


Thanks,

Aleksandar

Re: [patch] gdbarch_syscall_pc_increment

[Pedro Alves]

On 12/14/2012 02:23 PM, Aleksandar Ristovski wrote:
> On 12-12-13 04:47 PM, Pedro Alves wrote:
>> On 12/13/2012 02:07 PM, Aleksandar Ristovski wrote:
>>
>>> In this patch, we are talking about two things:
>>>
>>> a) the concept of syscall behaving as a conditional branch with uncomputable condition (i.e. gdb can not really compute whether the branch will be taken or not). This is a concept specific to a given kernel implementation of system calls and as such belongs to gdbarch.
>>>
>>> b) The amount by which pc may be incremented if the branch is taken. For an os that has a), this is cpu specific. It may also be dependent on the instruction set mode on e.g. arm (this is why frame is being passed).
>>>
>>> Where does this belong: in the case here, the line is a bit blurry since a) is generic concept applicable for all cpus for a given os thus definitely belonging to gdbarch. b) is cpu concept and strictly speaking belongs to gdbarch_tdep. However, since a) can also answer b) I see no point in further breaking it down.
>>
>> The interface is not pretty, but you can just install an additional
>> breakpoint from your OS's tdep->syscall_next_pc hook or
>> gdbarch_software_single_step wrapper hook.  See e.g.,:
>>
>> /* Find the next PC after the current instruction executes.  In some
>>     cases we can not statically determine the answer (see the IT state
>>     handling in this function); in that case, a breakpoint may be
>>     inserted in addition to the returned PC, which will be used to set
>>     another breakpoint by our caller.  */
>>
>> static CORE_ADDR
>> thumb_get_next_pc_raw (struct frame_info *frame, CORE_ADDR pc)
>> {
>> ...
>>           /* Set a breakpoint on the following instruction.  */
>>           gdb_assert ((itstate & 0x0f) != 0);
>>           arm_insert_single_step_breakpoint (gdbarch, aspace,
>>                          MAKE_THUMB_ADDR (pc));
>> ...
>>
>> I believe other ports do similar things in similar cases.
> 
> 
> This seems just wrong to me, to install a breakpoint when answering a question.

I did say the interface wasn't pretty.  :-) "get_next_pc" returning a single address
is what IMO is broken.  These hooks should be returning a list of locations, so that
the callers could install the multiple breakpoints themselves.

-- 
Pedro Alves