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

[Marcus Shawcroft <marcus.shawcroft@arm.com>]

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