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

Sergio Durigan Junior
2013-11-25T21:11:36Z

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