[PATCH v2] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64

[PATCH v2] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64

[Sergio Durigan Junior]

Hi,

This is the second attempt to implement SystemTap SDT probe support for
AArch64 GNU/Linux targets.  You can see the first attempt here:

<https://sourceware.org/ml/gdb-patches/2013-11/msg00745.html>

After my last patch for the SystemTap SDT API, which extends it in order
to accept multiple prefixes and suffixes, this patch became simpler.  I
also followed Marcus suggestion and did not shared code between 32- and
64-bit ARM.

Tom asked me in a previous message how I did my tests.  I believe I
replied that, but just in case: I ran the tests on
gdb.base/stap-probe.exp by hand.  As I write this message, I'm working
on getting a way to run the full testsuite.  Will update this thread
if/when I succeed.

I'd like to apply this before Joel creates the 7.7 branch.  Let's see
:-).

OK to apply?

Thanks,

-- 
Sergio

2013-12-19  Sergio Durigan Junior  <sergiodj@redhat.com>

	* NEWS: Mention SystemTap SDT probe support for AArch64 GNU/Linux.
	* aarch64-linux-tdep.c: Include necessary headers for parsing of
	SystemTap SDT probes.
	(aarch64_stap_is_single_operand): New function.
	(aarch64_stap_parse_special_token): Likewise.
	(aarch64_linux_init_abi): Declare SystemTap SDT probe argument
	prefixes and suffixes.  Initialize gdbarch with them.

diff --git a/gdb/NEWS b/gdb/NEWS
index e4baf50..b64967e 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,8 @@
 
 *** Changes since GDB 7.6
 
+* GDB now supports SystemTap SDT probes on AArch64 GNU/Linux.
+
 * GDB now supports Fission DWP file format version 2.
   http://gcc.gnu.org/wiki/DebugFission
 
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index bcfcce2..d2e3ee0 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -35,6 +35,12 @@
 #include "regcache.h"
 #include "regset.h"
 
+#include "cli/cli-utils.h"
+#include "stap-probe.h"
+#include "parser-defs.h"
+#include "user-regs.h"
+#include <ctype.h>
+
 /* The general-purpose regset consists of 31 X registers, plus SP, PC,
    and PSTATE registers, as defined in the AArch64 port of the Linux
    kernel.  */
@@ -263,9 +269,126 @@ aarch64_linux_regset_from_core_section (struct gdbarch *gdbarch,
   return NULL;
 }
 
+static int
+aarch64_stap_is_single_operand (struct gdbarch *gdbarch, const char *s)
+{
+  return (*s == '#' || *s == '$' || isdigit (*s) /* Literal number.  */
+	  || *s == '[' /* Register indirection.  */
+	  || isalpha (*s)); /* Register value.  */
+}
+
+/* This routine is used to parse a special token in AArch64's assembly.
+
+   The special tokens parsed by it are:
+
+      - Register displacement (e.g, [fp, #-8])
+
+   It returns one if the special token has been parsed successfully,
+   or zero if the current token is not considered special.  */
+
+static int
+aarch64_stap_parse_special_token (struct gdbarch *gdbarch,
+				  struct stap_parse_info *p)
+{
+  if (*p->arg == '[')
+    {
+      /* Temporary holder for lookahead.  */
+      const char *tmp = p->arg;
+      char *endp;
+      /* Used to save the register name.  */
+      const char *start;
+      char *regname;
+      int len;
+      int got_minus = 0;
+      long displacement;
+      struct stoken str;
+
+      ++tmp;
+      start = tmp;
+
+      /* Register name.  */
+      while (isalnum (*tmp))
+	++tmp;
+
+      if (*tmp != ',')
+	return 0;
+
+      len = tmp - start;
+      regname = alloca (len + 2);
+
+      strncpy (regname, start, len);
+      regname[len] = '\0';
+
+      if (user_reg_map_name_to_regnum (gdbarch, regname, len) == -1)
+	error (_("Invalid register name `%s' on expression `%s'."),
+	       regname, p->saved_arg);
+
+      ++tmp;
+      tmp = skip_spaces_const (tmp);
+      /* Now we expect a number.  It can begin with '#', '$' or simply
+	 a digit.  */
+      if (*tmp != '#' && *tmp != '$' && !isdigit (*tmp))
+	return 0;
+
+      if (*tmp == '#' || *tmp == '$')
+	++tmp;
+
+      if (*tmp == '-')
+	{
+	  ++tmp;
+	  got_minus = 1;
+	}
+      else if (*tmp == '+')
+	++tmp;
+
+      displacement = strtol (tmp, &endp, 10);
+      tmp = endp;
+
+      /* Skipping last `]'.  */
+      if (*tmp++ != ']')
+	return 0;
+
+      /* The displacement.  */
+      write_exp_elt_opcode (OP_LONG);
+      write_exp_elt_type (builtin_type (gdbarch)->builtin_long);
+      write_exp_elt_longcst (displacement);
+      write_exp_elt_opcode (OP_LONG);
+      if (got_minus)
+	write_exp_elt_opcode (UNOP_NEG);
+
+      /* The register name.  */
+      write_exp_elt_opcode (OP_REGISTER);
+      str.ptr = regname;
+      str.length = len;
+      write_exp_string (str);
+      write_exp_elt_opcode (OP_REGISTER);
+
+      write_exp_elt_opcode (BINOP_ADD);
+
+      /* Casting to the expected type.  */
+      write_exp_elt_opcode (UNOP_CAST);
+      write_exp_elt_type (lookup_pointer_type (p->arg_type));
+      write_exp_elt_opcode (UNOP_CAST);
+
+      write_exp_elt_opcode (UNOP_IND);
+
+      p->arg = tmp;
+    }
+  else
+    return 0;
+
+  return 1;
+}
+
 static void
 aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
+  static const char *const stap_integer_prefixes[] = { "#", "$", "", NULL };
+  static const char *const stap_register_prefixes[] = { "", NULL };
+  static const char *const stap_register_indirection_prefixes[] = { "[",
+								    NULL };
+  static const char *const stap_register_indirection_suffixes[] = { "]",
+								    NULL };
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
   tdep->lowest_pc = 0x8000;
@@ -290,6 +413,17 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   set_gdbarch_regset_from_core_section (gdbarch,
 					aarch64_linux_regset_from_core_section);
+
+  /* SystemTap related.  */
+  set_gdbarch_stap_integer_prefixes (gdbarch, stap_integer_prefixes);
+  set_gdbarch_stap_register_prefixes (gdbarch, stap_register_prefixes);
+  set_gdbarch_stap_register_indirection_prefixes (gdbarch,
+					    stap_register_indirection_prefixes);
+  set_gdbarch_stap_register_indirection_suffixes (gdbarch,
+					    stap_register_indirection_suffixes);
+  set_gdbarch_stap_is_single_operand (gdbarch, aarch64_stap_is_single_operand);
+  set_gdbarch_stap_parse_special_token (gdbarch,
+					aarch64_stap_parse_special_token);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */

Re: [PATCH v2] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64

[Sergio Durigan Junior]

On Thursday, December 19 2013, I wrote:

> Tom asked me in a previous message how I did my tests.  I believe I
> replied that, but just in case: I ran the tests on
> gdb.base/stap-probe.exp by hand.  As I write this message, I'm working
> on getting a way to run the full testsuite.  Will update this thread
> if/when I succeed.

I ran the regression test.  The patch did not introduce any errors, and
actually made gdb.cp/nextoverthrow.exp PASS in many tests (because of
the helper probes in libgcc).

-- 
Sergio

Re: [PATCH v2] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64

[Yufeng Zhang]

Hi Sergio,

Thanks for the updated patch.  It looks OK to me and the only comment I 
have is about the '$' character.  In A64, only '#' can be the optional 
prefix for immediate.  I didn't see '$' checked in your original patch; 
is there a particular reason that you added support for '$' in this patch?

Thanks,
Yufeng

p.s. I cannot approve patch and Marcus is on holiday, so you may need to 
seek an approval from a global maintainer if you need to get the patch 
in before 7.7 is branched.

On 12/19/13 22:20, Sergio Durigan Junior wrote:
> Hi,
>
> This is the second attempt to implement SystemTap SDT probe support for
> AArch64 GNU/Linux targets.  You can see the first attempt here:
>
> <https://sourceware.org/ml/gdb-patches/2013-11/msg00745.html>
>
> After my last patch for the SystemTap SDT API, which extends it in order
> to accept multiple prefixes and suffixes, this patch became simpler.  I
> also followed Marcus suggestion and did not shared code between 32- and
> 64-bit ARM.
>
> Tom asked me in a previous message how I did my tests.  I believe I
> replied that, but just in case: I ran the tests on
> gdb.base/stap-probe.exp by hand.  As I write this message, I'm working
> on getting a way to run the full testsuite.  Will update this thread
> if/when I succeed.
>
> I'd like to apply this before Joel creates the 7.7 branch.  Let's see
> :-).
>
> OK to apply?
>
> Thanks,
>

Re: [PATCH v2] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64

[Sergio Durigan Junior]

On Monday, December 23 2013, Yufeng Zhang wrote:

> Hi Sergio,

Hey Yufeng,

> Thanks for the updated patch.  It looks OK to me and the only comment
> I have is about the '$' character.  In A64, only '#' can be the
> optional prefix for immediate.  I didn't see '$' checked in your
> original patch; is there a particular reason that you added support
> for '$' in this patch?

Thanks a lot.  You are right, only '#' is an optional prefix in
AArch64.  Here's an updated patch.

-- 
Sergio

2013-12-23  Sergio Durigan Junior  <sergiodj@redhat.com>

	* NEWS: Mention SystemTap SDT probe support for AArch64 GNU/Linux.
	* aarch64-linux-tdep.c: Include necessary headers for parsing of
	SystemTap SDT probes.
	(aarch64_stap_is_single_operand): New function.
	(aarch64_stap_parse_special_token): Likewise.
	(aarch64_linux_init_abi): Declare SystemTap SDT probe argument
	prefixes and suffixes.  Initialize gdbarch with them.

diff --git a/gdb/NEWS b/gdb/NEWS
index e4baf50..b64967e 100644
--- a/gdb/NEWS
+++ b/gdb/NEWS
@@ -3,6 +3,8 @@
 
 *** Changes since GDB 7.6
 
+* GDB now supports SystemTap SDT probes on AArch64 GNU/Linux.
+
 * GDB now supports Fission DWP file format version 2.
   http://gcc.gnu.org/wiki/DebugFission
 
diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index bcfcce2..b828afd 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -35,6 +35,12 @@
 #include "regcache.h"
 #include "regset.h"
 
+#include "cli/cli-utils.h"
+#include "stap-probe.h"
+#include "parser-defs.h"
+#include "user-regs.h"
+#include <ctype.h>
+
 /* The general-purpose regset consists of 31 X registers, plus SP, PC,
    and PSTATE registers, as defined in the AArch64 port of the Linux
    kernel.  */
@@ -263,9 +269,126 @@ aarch64_linux_regset_from_core_section (struct gdbarch *gdbarch,
   return NULL;
 }
 
+static int
+aarch64_stap_is_single_operand (struct gdbarch *gdbarch, const char *s)
+{
+  return (*s == '#' || isdigit (*s) /* Literal number.  */
+	  || *s == '[' /* Register indirection.  */
+	  || isalpha (*s)); /* Register value.  */
+}
+
+/* This routine is used to parse a special token in AArch64's assembly.
+
+   The special tokens parsed by it are:
+
+      - Register displacement (e.g, [fp, #-8])
+
+   It returns one if the special token has been parsed successfully,
+   or zero if the current token is not considered special.  */
+
+static int
+aarch64_stap_parse_special_token (struct gdbarch *gdbarch,
+				  struct stap_parse_info *p)
+{
+  if (*p->arg == '[')
+    {
+      /* Temporary holder for lookahead.  */
+      const char *tmp = p->arg;
+      char *endp;
+      /* Used to save the register name.  */
+      const char *start;
+      char *regname;
+      int len;
+      int got_minus = 0;
+      long displacement;
+      struct stoken str;
+
+      ++tmp;
+      start = tmp;
+
+      /* Register name.  */
+      while (isalnum (*tmp))
+	++tmp;
+
+      if (*tmp != ',')
+	return 0;
+
+      len = tmp - start;
+      regname = alloca (len + 2);
+
+      strncpy (regname, start, len);
+      regname[len] = '\0';
+
+      if (user_reg_map_name_to_regnum (gdbarch, regname, len) == -1)
+	error (_("Invalid register name `%s' on expression `%s'."),
+	       regname, p->saved_arg);
+
+      ++tmp;
+      tmp = skip_spaces_const (tmp);
+      /* Now we expect a number.  It can begin with '#' or simply
+	 a digit.  */
+      if (*tmp == '#')
+	++tmp;
+
+      if (*tmp == '-')
+	{
+	  ++tmp;
+	  got_minus = 1;
+	}
+      else if (*tmp == '+')
+	++tmp;
+
+      if (!isdigit (*tmp))
+	return 0;
+
+      displacement = strtol (tmp, &endp, 10);
+      tmp = endp;
+
+      /* Skipping last `]'.  */
+      if (*tmp++ != ']')
+	return 0;
+
+      /* The displacement.  */
+      write_exp_elt_opcode (OP_LONG);
+      write_exp_elt_type (builtin_type (gdbarch)->builtin_long);
+      write_exp_elt_longcst (displacement);
+      write_exp_elt_opcode (OP_LONG);
+      if (got_minus)
+	write_exp_elt_opcode (UNOP_NEG);
+
+      /* The register name.  */
+      write_exp_elt_opcode (OP_REGISTER);
+      str.ptr = regname;
+      str.length = len;
+      write_exp_string (str);
+      write_exp_elt_opcode (OP_REGISTER);
+
+      write_exp_elt_opcode (BINOP_ADD);
+
+      /* Casting to the expected type.  */
+      write_exp_elt_opcode (UNOP_CAST);
+      write_exp_elt_type (lookup_pointer_type (p->arg_type));
+      write_exp_elt_opcode (UNOP_CAST);
+
+      write_exp_elt_opcode (UNOP_IND);
+
+      p->arg = tmp;
+    }
+  else
+    return 0;
+
+  return 1;
+}
+
 static void
 aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
+  static const char *const stap_integer_prefixes[] = { "#", "", NULL };
+  static const char *const stap_register_prefixes[] = { "", NULL };
+  static const char *const stap_register_indirection_prefixes[] = { "[",
+								    NULL };
+  static const char *const stap_register_indirection_suffixes[] = { "]",
+								    NULL };
   struct gdbarch_tdep *tdep = gdbarch_tdep (gdbarch);
 
   tdep->lowest_pc = 0x8000;
@@ -290,6 +413,17 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 
   set_gdbarch_regset_from_core_section (gdbarch,
 					aarch64_linux_regset_from_core_section);
+
+  /* SystemTap related.  */
+  set_gdbarch_stap_integer_prefixes (gdbarch, stap_integer_prefixes);
+  set_gdbarch_stap_register_prefixes (gdbarch, stap_register_prefixes);
+  set_gdbarch_stap_register_indirection_prefixes (gdbarch,
+					    stap_register_indirection_prefixes);
+  set_gdbarch_stap_register_indirection_suffixes (gdbarch,
+					    stap_register_indirection_suffixes);
+  set_gdbarch_stap_is_single_operand (gdbarch, aarch64_stap_is_single_operand);
+  set_gdbarch_stap_parse_special_token (gdbarch,
+					aarch64_stap_parse_special_token);
 }
 
 /* Provide a prototype to silence -Wmissing-prototypes.  */

Re: [PATCH v2] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64

[Yufeng Zhang]

On 12/23/13 17:18, Sergio Durigan Junior wrote:
> On Monday, December 23 2013, Yufeng Zhang wrote:
>
>> Hi Sergio,
>
> Hey Yufeng,
>
>> Thanks for the updated patch.  It looks OK to me and the only comment
>> I have is about the '$' character.  In A64, only '#' can be the
>> optional prefix for immediate.  I didn't see '$' checked in your
>> original patch; is there a particular reason that you added support
>> for '$' in this patch?
>
> Thanks a lot.  You are right, only '#' is an optional prefix in
> AArch64.  Here's an updated patch.

Thank you for your prompt action.  It looks good to me.  Please ask Tom 
(or other global maintainer) for an approval if you need to get it in today.

Thanks,
Yufeng

Re: [PATCH v2] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64

[Joel Brobecker]

> 2013-12-23  Sergio Durigan Junior  <sergiodj@redhat.com>
> 
> 	* NEWS: Mention SystemTap SDT probe support for AArch64 GNU/Linux.
> 	* aarch64-linux-tdep.c: Include necessary headers for parsing of
> 	SystemTap SDT probes.
> 	(aarch64_stap_is_single_operand): New function.
> 	(aarch64_stap_parse_special_token): Likewise.
> 	(aarch64_linux_init_abi): Declare SystemTap SDT probe argument
> 	prefixes and suffixes.  Initialize gdbarch with them.

First of all, thanks to Yufeng and Marcus for reviewing the patch.
That was very useful.

The patch is therefore approved, with one small request:

>  }
>  
> +static int
> +aarch64_stap_is_single_operand (struct gdbarch *gdbarch, const char *s)
> +{
> +  return (*s == '#' || isdigit (*s) /* Literal number.  */
> +	  || *s == '[' /* Register indirection.  */
> +	  || isalpha (*s)); /* Register value.  */
> +}

Can you document this function?

Thank you,
-- 
Joel

Re: [PATCH v2] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64

[Sergio Durigan Junior]

On Saturday, December 28 2013, Joel Brobecker wrote:

>> 2013-12-23  Sergio Durigan Junior  <sergiodj@redhat.com>
>> 
>> 	* NEWS: Mention SystemTap SDT probe support for AArch64 GNU/Linux.
>> 	* aarch64-linux-tdep.c: Include necessary headers for parsing of
>> 	SystemTap SDT probes.
>> 	(aarch64_stap_is_single_operand): New function.
>> 	(aarch64_stap_parse_special_token): Likewise.
>> 	(aarch64_linux_init_abi): Declare SystemTap SDT probe argument
>> 	prefixes and suffixes.  Initialize gdbarch with them.
>
> First of all, thanks to Yufeng and Marcus for reviewing the patch.
> That was very useful.
>
> The patch is therefore approved, with one small request:

Thanks a lot, Joel.

>>  }
>>  
>> +static int
>> +aarch64_stap_is_single_operand (struct gdbarch *gdbarch, const char *s)
>> +{
>> +  return (*s == '#' || isdigit (*s) /* Literal number.  */
>> +	  || *s == '[' /* Register indirection.  */
>> +	  || isalpha (*s)); /* Register value.  */
>> +}
>
> Can you document this function?

Yes, fixed.

Checked in:

        <https://sourceware.org/ml/gdb-cvs/2013-12/msg00123.html>

Thanks,

-- 
Sergio

Re: [PATCH v2] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64

[Sergio Durigan Junior]

On Saturday, December 28 2013, I wrote:

> Checked in:
>
>         <https://sourceware.org/ml/gdb-cvs/2013-12/msg00123.html>

BTW, I also updated the wiki page of the 7.7 release in order to reflect
this commit.

Thanks,

-- 
Sergio

Re: [PATCH v2] Fix for PR tdep/15653: Implement SystemTap SDT probe support for AArch64

[Joel Brobecker]

> BTW, I also updated the wiki page of the 7.7 release in order to reflect
> this commit.

Thank you, Sergio!

-- 
Joel