Re: [PATCH] ARM Linux support for `catch syscall'

[Sergio Durigan Junior <sergiodj@redhat.com>]

On Wednesday, July 31 2013, Samuel Bronson wrote:

> This works alright except for the incomplete syscalls/arm-linux.xml,
> for which I am waiting on a copy of sergiodj's scripts.

Hey Samuel,

Thanks for the patch, and sorry about the delay.

Well, when I told you about some scripts that I used (back in 2009), I
was actually trying to say that it's just a matter of doing some grep's
and sed's in the Linux files.  I guess I didn't provide the script at
that time because the indentation inside the files does not always
follow a pattern, so I thought it would be easier to just deal with each
file separately (and it took me less than 10 minutes to create the XML
file, after all).

Unfortunately, I don't have this script anymore, but if you really want
it, I can create one for the ARM file and send it to you.

> (It only fails one test in catch-syscall.exp, and that's because of a
> syscall missing from the XML file, but I decided that adding just that
> syscall to the XML file would be cheating.)

I wouldn't call it "cheating" :-).

> gdb/
> 2013-08-02  Samuel Bronson  <naesten@gmail.com>
>
> 	ARM Linux support for`catch syscall'.

Missing space between "for`catch syscall'".

> 	* syscalls/arm-linux.xml: New (stub) syscall file for ARM Linux.

New file.

> 	* arm-linux-tdep.c (arm_linux_get_syscall_number): New function.
> 	(arm_linux_init_abi): Register the new function and syscall file.
> 	* data-directory/Makefile.in: Install the new syscall file.
>
> gdb/testsuite/
> 2013-08-02  Samuel Bronson  <naesten@gmail.com>
>
> 	ARM Linux support for`catch syscall'.

Missing space.

> 	* gdb.base/catch-syscall.exp: Test this on ARM now.
> 	(fill_all_syscalls_numbers): ARM has close/chroot on 6/61, too.
> ---
>  gdb/arm-linux-tdep.c                     |  56 ++++++++++
>  gdb/data-directory/Makefile.in           |   1 +
>  gdb/syscalls/arm-linux.xml               | 181 +++++++++++++++++++++++++++++++
>  gdb/testsuite/gdb.base/catch-syscall.exp |   9 +-
>  4 files changed, 243 insertions(+), 4 deletions(-)
>  create mode 100644 gdb/syscalls/arm-linux.xml
>
> diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
> index 1502bdc..9d8238f 100644
> --- a/gdb/arm-linux-tdep.c
> +++ b/gdb/arm-linux-tdep.c
> @@ -33,6 +33,7 @@
>  #include "tramp-frame.h"
>  #include "breakpoint.h"
>  #include "auxv.h"
> +#include "xml-syscall.h"
>  
>  #include "arm-tdep.h"
>  #include "arm-linux-tdep.h"
> @@ -794,6 +795,57 @@ arm_linux_sigreturn_return_addr (struct frame_info *frame,
>    return 0;
>  }
>  
> +/* At a ptrace syscall-stop, return the syscall number.  This either
> +   comes from the SVC instruction (OABI) or from r7 (EABI).
> +
> +   When the function fails, it should return -1.  */
> +
> +static LONGEST
> +arm_linux_get_syscall_number (struct gdbarch *gdbarch,
> +			      ptid_t ptid)
> +{
> +  struct regcache *regs = get_thread_regcache (ptid);
> +  struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
> +
> +  ULONGEST pc;
> +  ULONGEST cpsr;
> +  ULONGEST t_bit = arm_psr_thumb_bit (gdbarch);
> +  int is_thumb;
> +  ULONGEST svc_number = -1;
> +
> +  regcache_cooked_read_unsigned (regs, ARM_PC_REGNUM, &pc);
> +  regcache_cooked_read_unsigned (regs, ARM_PS_REGNUM, &cpsr);
> +  is_thumb = (cpsr & t_bit) != 0;
> +
> +  if (is_thumb)
> +    {
> +      regcache_cooked_read_unsigned (regs, 7, &svc_number);
> +    }
> +  else
> +    {
> +      enum bfd_endian byte_order_for_code = 
> +	gdbarch_byte_order_for_code (gdbarch);
> +
> +      /* PC gets incremented before the syscall-stop, so read the
> +	 previous instruction.  */
> +      unsigned long this_instr = 
> +	read_memory_unsigned_integer (pc - 4, 4, byte_order_for_code);
> +
> +      unsigned long svc_operand = (0x00ffffff & this_instr);
> +
> +      if (svc_operand)  /* OABI.  */
> +	{
> +	  svc_number = svc_operand - 0x900000;
> +	}
> +      else /* EABI.  */
> +	{
> +	  regcache_cooked_read_unsigned (regs, 7, &svc_number);
> +	}

I find it a little odd to write comments in the front of the "if/else".
I guess you could write them inside the blocks.

> +    }
> +
> +  return svc_number;
> +}
> +

Well, I'm not very familiar with ARM, but I guess your code looks OK
according to your explanation.

> diff --git a/gdb/syscalls/arm-linux.xml b/gdb/syscalls/arm-linux.xml
> new file mode 100644
> index 0000000..28e1b9d
> --- /dev/null
> +++ b/gdb/syscalls/arm-linux.xml
> @@ -0,0 +1,181 @@
> +<?xml version="1.0"?>
> +<!-- Copyright (C) 2009-2013 Free Software Foundation, Inc.
> +
> +     Copying and distribution of this file, with or without modification,
> +     are permitted in any medium without royalty provided the copyright
> +     notice and this notice are preserved.  -->
> +
> +<!DOCTYPE feature SYSTEM "gdb-syscalls.dtd">
> +
> +<!-- This file was copied from ppc-linux.xml because *somebody* didn't
> +     provide his generation script.  (It has been truncated where the
> +     syscall numbers diverge.) -->

This comment is unecessary IMO, given what I said above :-).

The rest looks OK to me.  I am not a GDB maintainer, so you may wish to
wait until someone gives his final word.

Thanks a lot,

-- 
Sergio