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

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

[Sergio Durigan Junior]

This commit implements the needed bits for SystemTap SDT probe support
on AArch64 architectures.  The core of the patch is in the
aarch64-linux-tdep.c file; all the rest are support/cleanup
modifications.

First, I started by looking at AArch64 assembly specification and
filling the necessary options on gdbarch's stap machinery in order to
make the generic asm parser (implemented in stap-probe.c) recognize
AArch64's asm.

AArch64 has the same idiosincrasy as 32-bit ARM: the syntax for
register indirection is very specific, and demands its own function to
parse this special expression.  However, both AArch64 and 32-bit ARM
share the same syntax (with only one little difference between them),
which made me think of a way to share the specific parser between both
architectures.  That's why I came up with the modification on
configure.tgt: the idea is to mimic i386/x86_64 behavior, by sharing
the 32-bit linux-tdep.c file with the 64-bit relative.  Therefore, I
decided to make arm-linux-tdep.c and arm-tdep.c be built when we're
compiling for aarch64-linux-gnu.  I guess another option would be to
create a "generic" ARM tdep file to be shared among 32- and 64-bit,
but I didn't go that way because i386/x86_64 don't do that...

The other issue I had to fix was a bad decision on the code that
handles single operands on the generic asm parser.  If there is no
register prefix or a register indirection prefix, then we should
assume that the operand being parsed is a register, and not an error.
This case is covered now.

Now, about the tests...  I have been using ARM Foundation's simulator
on x86_64 (Fedora 18).  This is an incredibly slow simulator, to the
point that I gave up compiling GDB directly on it.  Instead, I am
cross-compiling it on my machine and testing only the binary there.
Therefore, I also gave up running the testsuite, and only ran
stap-related tests.  As far as I'm concerned, everything succeeded and
I didn't break anything.  The code is pretty much self contained that
it doesn't affect other areas of GDB, but of course this patch could
use more testing, and I won't complain if you have the means and the
will to do this!  Otherwise, I consider this patch OK, and of course I
will keep an eye for when AArch64 hardware becomes available, so that
I can test this on real machines.

Any comments?  OK to apply?

Thanks.

2013-11-24  Sergio Durigan Junior  <sergiodj@redhat.com>

	PR tdep/15653:
	* aarch64-linux-tdep.c: Include "arm-linux-tdep.h" and <ctype.h>.
	(aarch64_stap_is_single_operand): New function.
	(aarch64_stap_parse_special_token): Likewise.
	(aarch64_linux_init_abi): Intialize SystemTap SDT probes specific
	parameters.
	* arm-linux-tdep.c (arm_generic_stap_parse_special_token): New
	function, with the contents of...
	(arm_stap_parse_special_token): ...this one.  Rewrite this
	function to call the function above.
	* arm-linux-tdep.h: Forward declare "struct gdbarch" and "struct
	stap_parse_info".
	(arm_generic_stap_parse_special_token): New prototype.
	* configure.tgt (aarch64*-*-linux*): Add "arm-linux-tdep.o" and
	"arm-tdep.o" to be built.
	* stap-probe.c (stap_parse_single_operand): Handle the case when
	there is no register prefix or register indirection prefix.
---
 gdb/aarch64-linux-tdep.c | 31 +++++++++++++++++++++++++++++++
 gdb/arm-linux-tdep.c     | 27 +++++++++++++++------------
 gdb/arm-linux-tdep.h     | 20 ++++++++++++++++++++
 gdb/configure.tgt        |  3 ++-
 gdb/stap-probe.c         | 13 +++++++++++--
 5 files changed, 79 insertions(+), 15 deletions(-)

diff --git a/gdb/aarch64-linux-tdep.c b/gdb/aarch64-linux-tdep.c
index bcfcce2..f23468c 100644
--- a/gdb/aarch64-linux-tdep.c
+++ b/gdb/aarch64-linux-tdep.c
@@ -25,6 +25,7 @@
 #include "linux-tdep.h"
 #include "aarch64-tdep.h"
 #include "aarch64-linux-tdep.h"
+#include "arm-linux-tdep.h"
 #include "osabi.h"
 #include "solib-svr4.h"
 #include "symtab.h"
@@ -35,6 +36,8 @@
 #include "regcache.h"
 #include "regset.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,6 +266,25 @@ aarch64_linux_regset_from_core_section (struct gdbarch *gdbarch,
   return NULL;
 }
 
+/* Implementation of gdbarch_stap_is_single_operand.  */
+
+static int
+aarch64_stap_is_single_operand (struct gdbarch *gdbarch, const char *s)
+{
+  return (*s == '#' /* Literal number.  */
+	  || *s == '[' /* Register indirection or displacement.  */
+	  || isalpha (*s)); /* Register value.  */
+}
+
+/* Implementation of gdbarch_stap_parse_special_token.  */
+
+static int
+aarch64_stap_parse_special_token (struct gdbarch *gdbarch,
+				  struct stap_parse_info *p)
+{
+  return arm_generic_stap_parse_special_token (gdbarch, p, 0);
+}
+
 static void
 aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
 {
@@ -285,6 +307,15 @@ aarch64_linux_init_abi (struct gdbarch_info info, struct gdbarch *gdbarch)
   set_gdbarch_get_siginfo_type (gdbarch, linux_get_siginfo_type);
   tramp_frame_prepend_unwinder (gdbarch, &aarch64_linux_rt_sigframe);
 
+  /* SystemTap SDT probe related.  */
+  set_gdbarch_stap_integer_prefix (gdbarch, "#");
+  set_gdbarch_stap_register_indirection_prefix (gdbarch, "[");
+  set_gdbarch_stap_register_indirection_suffix (gdbarch, "]");
+  set_gdbarch_stap_is_single_operand (gdbarch,
+				      aarch64_stap_is_single_operand);
+  set_gdbarch_stap_parse_special_token (gdbarch,
+					aarch64_stap_parse_special_token);
+
   /* Enable longjmp.  */
   tdep->jb_pc = 11;
 
diff --git a/gdb/arm-linux-tdep.c b/gdb/arm-linux-tdep.c
index 9deed10..f316cde 100644
--- a/gdb/arm-linux-tdep.c
+++ b/gdb/arm-linux-tdep.c
@@ -1122,18 +1122,12 @@ arm_stap_is_single_operand (struct gdbarch *gdbarch, const char *s)
 	  || isalpha (*s)); /* Register value.  */
 }
 
-/* This routine is used to parse a special token in ARM's assembly.
+/* See comment on arm-linux-tdep.h.  */
 
-   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
-arm_stap_parse_special_token (struct gdbarch *gdbarch,
-			      struct stap_parse_info *p)
+int
+arm_generic_stap_parse_special_token (struct gdbarch *gdbarch,
+				      struct stap_parse_info *p,
+				      int skip_hash_sign)
 {
   if (*p->arg == '[')
     {
@@ -1183,7 +1177,7 @@ arm_stap_parse_special_token (struct gdbarch *gdbarch,
 
       ++tmp;
       tmp = skip_spaces_const (tmp);
-      if (*tmp++ != '#')
+      if (skip_hash_sign && *tmp++ != '#')
 	return 0;
 
       if (*tmp == '-')
@@ -1231,6 +1225,15 @@ arm_stap_parse_special_token (struct gdbarch *gdbarch,
   return 1;
 }
 
+/* Implementation of gdbarch_stap_parse_special_token.  */
+
+static int
+arm_stap_parse_special_token (struct gdbarch *gdbarch,
+			      struct stap_parse_info *p)
+{
+  return arm_generic_stap_parse_special_token (gdbarch, p, 1);
+}
+
 static void
 arm_linux_init_abi (struct gdbarch_info info,
 		    struct gdbarch *gdbarch)
diff --git a/gdb/arm-linux-tdep.h b/gdb/arm-linux-tdep.h
index 59518fd..1b3c349 100644
--- a/gdb/arm-linux-tdep.h
+++ b/gdb/arm-linux-tdep.h
@@ -19,6 +19,8 @@
 
 struct regset;
 struct regcache;
+struct gdbarch;
+struct stap_parse_info;
 
 #define ARM_LINUX_SIZEOF_NWFPE (8 * FP_REGISTER_SIZE \
 				+ 2 * INT_REGISTER_SIZE \
@@ -59,6 +61,24 @@ void arm_linux_collect_nwfpe (const struct regset *regset,
 			      const struct regcache *regcache,
 			      int regnum, void *regs_buf, size_t len);
 
+/* This routine is used to parse a special token in ARM's assembly.
+   It works for both 32-bit and 64-bit ARM architectures.
+
+   The special tokens parsed by it are:
+
+     - Register displacement (e.g, [fp, #-8] on ARM, and [fp, -8] on AArch64)
+
+   SKIP_HASH_SIGN is 1 is the parser should skip the hash sign
+   before integers (e.g., #-8 for ARM), or 0 if the parser should
+   not bother with it (e.g., -8 for AArch64).
+
+   It returns one if the special token has been parsed successfully,
+   or zero if the current token is not considered special.  */
+
+extern int arm_generic_stap_parse_special_token (struct gdbarch *gdbarch,
+						 struct stap_parse_info *p,
+						 int skip_hash_sign);
+
 /* ARM GNU/Linux HWCAP values.  These are in defined in
    <asm/elf.h> in current kernels.  */
 #define HWCAP_VFP       64
diff --git a/gdb/configure.tgt b/gdb/configure.tgt
index 47e98d9..49dbc60 100644
--- a/gdb/configure.tgt
+++ b/gdb/configure.tgt
@@ -41,7 +41,8 @@ aarch64*-*-linux*)
 	# Target: AArch64 linux
 	gdb_target_obs="aarch64-tdep.o aarch64-linux-tdep.o \
 			glibc-tdep.o linux-tdep.o solib-svr4.o \
-			symfile-mem.o"
+			symfile-mem.o
+			arm-linux-tdep.o arm-tdep.o"
 	build_gdbserver=yes
 	;;
 
diff --git a/gdb/stap-probe.c b/gdb/stap-probe.c
index a734793..b3ab397 100644
--- a/gdb/stap-probe.c
+++ b/gdb/stap-probe.c
@@ -699,8 +699,17 @@ stap_parse_single_operand (struct stap_parse_info *p)
   else if ((reg_prefix
 	    && strncmp (p->arg, reg_prefix, reg_prefix_len) == 0)
 	   || (reg_ind_prefix
-	       && strncmp (p->arg, reg_ind_prefix, reg_ind_prefix_len) == 0))
-    stap_parse_register_operand (p);
+	       && strncmp (p->arg, reg_ind_prefix, reg_ind_prefix_len) == 0)
+	   || reg_prefix == NULL || reg_ind_prefix == NULL)
+    {
+      /* We are dealing with a register here, or trying to...  If
+	 `reg_prefix' or `reg_ind_prefix' are NULL, it means we can
+	 expect anything to be a register (or a register indirection),
+	 and therefore there is no way to guarantee that we really
+	 have a register in hands now.  So we call our parser for
+	 register operand, and let it deal with the situation.  */
+      stap_parse_register_operand (p);
+    }
   else
     error (_("Operator `%c' not recognized on expression `%s'."),
 	   *p->arg, p->saved_arg);
-- 
1.7.11.7

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

[Marcus Shawcroft]

Hi,

On 24/11/13 23:34, Sergio Durigan Junior wrote:
> This commit implements the needed bits for SystemTap SDT probe support
> on AArch64 architectures.  The core of the patch is in the
> aarch64-linux-tdep.c file; all the rest are support/cleanup
> modifications.
>
> First, I started by looking at AArch64 assembly specification and
> filling the necessary options on gdbarch's stap machinery in order to
> make the generic asm parser (implemented in stap-probe.c) recognize
> AArch64's asm.
>
> AArch64 has the same idiosincrasy as 32-bit ARM: the syntax for
> register indirection is very specific, and demands its own function to
> parse this special expression.  However, both AArch64 and 32-bit ARM
> share the same syntax (with only one little difference between them),

A64 assembler syntax is similar to A32 and T32 in some areas and differs 
somewhat in others.  The syntax used specifically for indirect register 
offset addressing is very similar.  It might make sense in some 
circumstances to share code that encapsulates u-architectural details 
that underpin an a64/a32/t32 implementation, but, building a shared 
assembler syntax parser in the fashion proposed could be described as 
coincidental cohesion. Is that desirable going forward?

> which made me think of a way to share the specific parser between both

> +/* Implementation of gdbarch_stap_is_single_operand.  */
> +
> +static int
> +aarch64_stap_is_single_operand (struct gdbarch *gdbarch, const char *s)
> +{
> +  return (*s == '#' /* Literal number.  */

The '#' before a literal is optional, therefore this clause ought to 
permit isdigit()?

> +	  || *s == '[' /* Register indirection or displacement.  */
> +	  || isalpha (*s)); /* Register value.  */
> +}
> +
> +/* Implementation of gdbarch_stap_parse_special_token.  */
> +
> +static int
> +aarch64_stap_parse_special_token (struct gdbarch *gdbarch,
> +				  struct stap_parse_info *p)
> +{
> +  return arm_generic_stap_parse_special_token (gdbarch, p, 0);
> +}
>

Looking over in arm-linux-tdep.c:arm_generic_stap_parse_special_token():

"""
	  /* If we are dealing with a register whose name begins with a
	     digit, it means we should prefix the name with the letter
	     `r', because GDB expects this name pattern.  Otherwise (e.g.,
	     we are dealing with the register `fp'), we don't need to
	     add such a prefix.  */

"""

Does this really apply to a64 where there are no register names that 
begin with 'r'?


> +int
> +arm_generic_stap_parse_special_token (struct gdbarch *gdbarch,
> +				      struct stap_parse_info *p,
> +				      int skip_hash_sign)
>   {
>     if (*p->arg == '[')
>       {
> @@ -1183,7 +1177,7 @@ arm_stap_parse_special_token (struct gdbarch *gdbarch,
>
>         ++tmp;
>         tmp = skip_spaces_const (tmp);
> -      if (*tmp++ != '#')
> +      if (skip_hash_sign && *tmp++ != '#')

The '#' at this point is optional in a64.

> @@ -59,6 +61,24 @@ void arm_linux_collect_nwfpe (const struct regset *regset,
>   			      const struct regcache *regcache,
>   			      int regnum, void *regs_buf, size_t len);
>
> +/* This routine is used to parse a special token in ARM's assembly.
> +   It works for both 32-bit and 64-bit ARM architectures.
> +
> +   The special tokens parsed by it are:
> +
> +     - Register displacement (e.g, [fp, #-8] on ARM, and [fp, -8] on AArch64)
> +

A # is optional in a64, therefore [fp, #-8] is legal.

> +   SKIP_HASH_SIGN is 1 is the parser should skip the hash sign
> +   before integers (e.g., #-8 for ARM), or 0 if the parser should
> +   not bother with it (e.g., -8 for AArch64).


Cheers
/Marcus

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

[Sergio Durigan Junior]

Hey,

On Monday, November 25 2013, Marcus Shawcroft wrote:

> On 24/11/13 23:34, Sergio Durigan Junior wrote:
>> This commit implements the needed bits for SystemTap SDT probe support
>> on AArch64 architectures.  The core of the patch is in the
>> aarch64-linux-tdep.c file; all the rest are support/cleanup
>> modifications.
>>
>> First, I started by looking at AArch64 assembly specification and
>> filling the necessary options on gdbarch's stap machinery in order to
>> make the generic asm parser (implemented in stap-probe.c) recognize
>> AArch64's asm.
>>
>> AArch64 has the same idiosincrasy as 32-bit ARM: the syntax for
>> register indirection is very specific, and demands its own function to
>> parse this special expression.  However, both AArch64 and 32-bit ARM
>> share the same syntax (with only one little difference between them),
>
> A64 assembler syntax is similar to A32 and T32 in some areas and
> differs somewhat in others.  The syntax used specifically for indirect
> register offset addressing is very similar.  It might make sense in
> some circumstances to share code that encapsulates u-architectural
> details that underpin an a64/a32/t32 implementation, but, building a
> shared assembler syntax parser in the fashion proposed could be
> described as coincidental cohesion. Is that desirable going forward?

I am not sure I completely understood your comment.  Are you criticizing
the shared asm parser for ARM targets only, or the generic asm parser in
stap-probe.c?  I assume it is the former.  And I also fail to see why it
can be considered coincidental cohesion...  The shared parser covers one
specific ARM assembly syntax, which is pretty similar to both 32- and
64-bit ARM.

Correct me if I'm wrong, but you're actually against the way I've chosen
to share the code between the targets, right?  Or am I missing something
here.  If that's the case, I can try to come up with a simpler/cleaner
way to share this code...  Suggestions are appreciated, of course :-).

>> which made me think of a way to share the specific parser between both
>
>> +/* Implementation of gdbarch_stap_is_single_operand.  */
>> +
>> +static int
>> +aarch64_stap_is_single_operand (struct gdbarch *gdbarch, const char *s)
>> +{
>> +  return (*s == '#' /* Literal number.  */
>
> The '#' before a literal is optional, therefore this clause ought to
> permit isdigit()?

Thanks, I wasn't aware that it is optional.  I will fix the code.

>> +	  || *s == '[' /* Register indirection or displacement.  */
>> +	  || isalpha (*s)); /* Register value.  */
>> +}
>> +
>> +/* Implementation of gdbarch_stap_parse_special_token.  */
>> +
>> +static int
>> +aarch64_stap_parse_special_token (struct gdbarch *gdbarch,
>> +				  struct stap_parse_info *p)
>> +{
>> +  return arm_generic_stap_parse_special_token (gdbarch, p, 0);
>> +}
>>
>
> Looking over in arm-linux-tdep.c:arm_generic_stap_parse_special_token():
>
> """
> 	  /* If we are dealing with a register whose name begins with a
> 	     digit, it means we should prefix the name with the letter
> 	     `r', because GDB expects this name pattern.  Otherwise (e.g.,
> 	     we are dealing with the register `fp'), we don't need to
> 	     add such a prefix.  */
>
> """
>
> Does this really apply to a64 where there are no register names that
> begin with 'r'?

No, it doesn't apply, but then, AArch64 registers' names don't begin
with a digit, so this case is never triggered.

However, I agree that the comment should be expanded in order to explain
why it doesn't apply.

>> +int
>> +arm_generic_stap_parse_special_token (struct gdbarch *gdbarch,
>> +				      struct stap_parse_info *p,
>> +				      int skip_hash_sign)
>>   {
>>     if (*p->arg == '[')
>>       {
>> @@ -1183,7 +1177,7 @@ arm_stap_parse_special_token (struct gdbarch *gdbarch,
>>
>>         ++tmp;
>>         tmp = skip_spaces_const (tmp);
>> -      if (*tmp++ != '#')
>> +      if (skip_hash_sign && *tmp++ != '#')
>
> The '#' at this point is optional in a64.

OK, thanks.

>> @@ -59,6 +61,24 @@ void arm_linux_collect_nwfpe (const struct regset *regset,
>>   			      const struct regcache *regcache,
>>   			      int regnum, void *regs_buf, size_t len);
>>
>> +/* This routine is used to parse a special token in ARM's assembly.
>> +   It works for both 32-bit and 64-bit ARM architectures.
>> +
>> +   The special tokens parsed by it are:
>> +
>> +     - Register displacement (e.g, [fp, #-8] on ARM, and [fp, -8] on AArch64)
>> +
>
> A # is optional in a64, therefore [fp, #-8] is legal.

Aha, nice to know.  I will expand the code to take that into
consideration as well.

Thanks for the review.  I will post a refreshed patch soon.

-- 
Sergio

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

[Marcus Shawcroft]

Hi

On 25 November 2013 21:11, Sergio Durigan Junior <sergiodj@redhat.com> wrote:

> I am not sure I completely understood your comment.  Are you criticizing
> the shared asm parser for ARM targets only,  or the generic asm parser in
> stap-probe.c?  I assume it is the former.  And I also fail to see why it
> can be considered coincidental cohesion...  The shared parser covers one
> specific ARM assembly syntax, which is pretty similar to both 32- and
> 64-bit ARM.

> Correct me if I'm wrong, but you're actually against the way I've chosen
> to share the code between the targets, right?  Or am I missing something
> here.  If that's the case, I can try to come up with a simpler/cleaner
> way to share this code...  Suggestions are appreciated, of course :-).

Sorry, I was not clear. I think the a64 and a32 parsers should be kept
separate rather than using a common parser for a different, but
similar syntax, thus avoiding the need to pass the flag that
distinguishes the different syntaxes.

Cheers
/Marcus

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

[Tom Tromey]

>>>>> "Sergio" == Sergio Durigan Junior <sergiodj@redhat.com> writes:

Sergio> Now, about the tests...  I have been using ARM Foundation's simulator
Sergio> on x86_64 (Fedora 18).  This is an incredibly slow simulator, to the
Sergio> point that I gave up compiling GDB directly on it.  Instead, I am
Sergio> cross-compiling it on my machine and testing only the binary there.
Sergio> Therefore, I also gave up running the testsuite, and only ran
Sergio> stap-related tests.  As far as I'm concerned, everything succeeded and
Sergio> I didn't break anything.  The code is pretty much self contained that
Sergio> it doesn't affect other areas of GDB, but of course this patch could
Sergio> use more testing, and I won't complain if you have the means and the
Sergio> will to do this!

FWIW in this specific case I think this testing approach is ok.
The patch pretty clearly just touches the stap probe code.
Could you say, though, exactly which tests you ran?

Tom

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

[Tom Tromey]

>>>>> "Marcus" == Marcus Shawcroft <marcus.shawcroft@gmail.com> writes:

Marcus> Sorry, I was not clear. I think the a64 and a32 parsers should be kept
Marcus> separate rather than using a common parser for a different, but
Marcus> similar syntax, thus avoiding the need to pass the flag that
Marcus> distinguishes the different syntaxes.

It seems reasonable to me.
It looks to me like the current function arm_stap_parse_special_token is
the only code shared by the patch.  That isn't so long by itself, and
then if it needs tweaks for AArch64 -- one might as well just copy it.

Tom

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

[Sergio Durigan Junior]

On Wednesday, November 27 2013, Tom Tromey wrote:

> FWIW in this specific case I think this testing approach is ok.
> The patch pretty clearly just touches the stap probe code.
> Could you say, though, exactly which tests you ran?

Thanks.  For this case, I ran manually the tests from
gdb.base/stap-probe.exp, checking the results and making sure everything
is working OK.

Since the AArch64 asm permits more than one numeric literal prefix to be
used, I hacked the generated assembly in order to test this too.  Soon I
will post a patch that extends the current generic parser to accept
multiple prefixes/suffixes, and then I will repost this AArch64 support
patch on top of it.

-- 
Sergio

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

[Sergio Durigan Junior]

On Wednesday, November 27 2013, Tom Tromey wrote:

>>>>>> "Marcus" == Marcus Shawcroft <marcus.shawcroft@gmail.com> writes:
>
> Marcus> Sorry, I was not clear. I think the a64 and a32 parsers should be kept
> Marcus> separate rather than using a common parser for a different, but
> Marcus> similar syntax, thus avoiding the need to pass the flag that
> Marcus> distinguishes the different syntaxes.
>
> It seems reasonable to me.
> It looks to me like the current function arm_stap_parse_special_token is
> the only code shared by the patch.  That isn't so long by itself, and
> then if it needs tweaks for AArch64 -- one might as well just copy it.

Sorry for taking a long time to reply.

OK then, two voices against one, you win :-).  I will repost the AArch64
patch later without this specific part, i.e., replicating the code from
ARM.

-- 
Sergio