This is the mail archive of the mailing list for the GDB project.

Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]
Other format: [Raw text]

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


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.


Index Nav: [Date Index] [Subject Index] [Author Index] [Thread Index]
Message Nav: [Date Prev] [Date Next] [Thread Prev] [Thread Next]