[AArch64] Rename boolean arguments in decoding functions

  Hi all,

This patch cleans up the decoding functions using booleans when they can
decode two instructions.  The boolean argument is used to know which of
the two instructions was decoded.

The instructions affected are BR/BLR, B/BL, CBZ/CBNZ and TBZ/TBNZ.

These arguments would be named after a named bit in the instruction
encoding, this patch renames them to 'is_XXX'.  Furthermore, the
'unsigned' type would be used to describe a boolean while
aarch64_decode_cb would use 'int' (see the 'is64' argument).  This patch
makes all booleans be 'int' and decoded bitfields be 'unsigned'.

Thanks,
Pierre

gdb/ChangeLog:

	* aarch64-tdep.c (decode_b): Rename link argument to is_bl.
	Change its type to int *.
	(decode_br): Rename link argument to is_blr.  Change its type to
	int *.
	(decode_cb): Rename op argument to is_cbnz.  Change its type to
	int *.
	(decode_tb): Rename op argument to is_tbnz.  Change its type to
	int *.  Set is_tbnz to either 1 or 0.
	(aarch64_analyze_prologue): Change type of is_link to int.  Add
	new variables is_cbnz and is_tbnz.  Adjust call to
	aarch64_decode_cb and aarch64_decode_tb.
---
 gdb/aarch64-tdep.c | 47 +++++++++++++++++++++++------------------------
 1 file changed, 23 insertions(+), 24 deletions(-)
  

> On Jul 29, 2015, at 2:33 AM, Pierre Langlois <pierre.langlois@arm.com> wrote:
> 
> Hi all,
> 
> This patch cleans up the decoding functions using booleans when they can
> decode two instructions.  The boolean argument is used to know which of
> the two instructions was decoded.
> 
> The instructions affected are BR/BLR, B/BL, CBZ/CBNZ and TBZ/TBNZ.
> 
> These arguments would be named after a named bit in the instruction
> encoding, this patch renames them to 'is_XXX'.  Furthermore, the
> 'unsigned' type would be used to describe a boolean while
> aarch64_decode_cb would use 'int' (see the 'is64' argument).  This patch
> makes all booleans be 'int' and decoded bitfields be 'unsigned'.

Why not use the bool type instead? Since that seems like a better option and might even be better optimized. 

Thanks,
Andrew

> 
> Thanks,
> Pierre
> 
> gdb/ChangeLog:
> 
>    * aarch64-tdep.c (decode_b): Rename link argument to is_bl.
>    Change its type to int *.
>    (decode_br): Rename link argument to is_blr.  Change its type to
>    int *.
>    (decode_cb): Rename op argument to is_cbnz.  Change its type to
>    int *.
>    (decode_tb): Rename op argument to is_tbnz.  Change its type to
>    int *.  Set is_tbnz to either 1 or 0.
>    (aarch64_analyze_prologue): Change type of is_link to int.  Add
>    new variables is_cbnz and is_tbnz.  Adjust call to
>    aarch64_decode_cb and aarch64_decode_tb.
> ---
> gdb/aarch64-tdep.c | 47 +++++++++++++++++++++++------------------------
> 1 file changed, 23 insertions(+), 24 deletions(-)
> 
> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
> index cec4d3e..c722dc5 100644
> --- a/gdb/aarch64-tdep.c
> +++ b/gdb/aarch64-tdep.c
> @@ -299,26 +299,26 @@ decode_adrp (CORE_ADDR addr, uint32_t insn, unsigned *rd)
> 
>    ADDR specifies the address of the opcode.
>    INSN specifies the opcode to test.
> -   LINK receives the 'link' bit from the decoded instruction.
> +   IS_BL receives the 'op' bit from the decoded instruction.
>    OFFSET receives the immediate offset from the decoded instruction.
> 
>    Return 1 if the opcodes matches and is decoded, otherwise 0.  */
> 
> static int
> -decode_b (CORE_ADDR addr, uint32_t insn, unsigned *link, int32_t *offset)
> +decode_b (CORE_ADDR addr, uint32_t insn, int *is_bl, int32_t *offset)
> {
>   /* b  0001 01ii iiii iiii iiii iiii iiii iiii */
>   /* bl 1001 01ii iiii iiii iiii iiii iiii iiii */
>   if (decode_masked_match (insn, 0x7c000000, 0x14000000))
>     {
> -      *link = insn >> 31;
> +      *is_bl = (insn >> 31) & 0x1;
>       *offset = extract_signed_bitfield (insn, 26, 0) << 2;
> 
>       if (aarch64_debug)
>    fprintf_unfiltered (gdb_stdlog,
>                "decode: 0x%s 0x%x %s 0x%s\n",
>                core_addr_to_string_nz (addr), insn,
> -                *link ? "bl" : "b",
> +                *is_bl ? "bl" : "b",
>                core_addr_to_string_nz (addr + *offset));
> 
>       return 1;
> @@ -358,27 +358,27 @@ decode_bcond (CORE_ADDR addr, uint32_t insn, unsigned *cond, int32_t *offset)
> 
>    ADDR specifies the address of the opcode.
>    INSN specifies the opcode to test.
> -   LINK receives the 'link' bit from the decoded instruction.
> +   IS_BLR receives the 'op' bit from the decoded instruction.
>    RN receives the 'rn' field from the decoded instruction.
> 
>    Return 1 if the opcodes matches and is decoded, otherwise 0.  */
> 
> static int
> -decode_br (CORE_ADDR addr, uint32_t insn, unsigned *link, unsigned *rn)
> +decode_br (CORE_ADDR addr, uint32_t insn, int *is_blr, unsigned *rn)
> {
>   /*         8   4   0   6   2   8   4   0 */
>   /* blr  110101100011111100000000000rrrrr */
>   /* br   110101100001111100000000000rrrrr */
>   if (decode_masked_match (insn, 0xffdffc1f, 0xd61f0000))
>     {
> -      *link = (insn >> 21) & 1;
> +      *is_blr = (insn >> 21) & 1;
>       *rn = (insn >> 5) & 0x1f;
> 
>       if (aarch64_debug)
>    fprintf_unfiltered (gdb_stdlog,
>                "decode: 0x%s 0x%x %s 0x%x\n",
>                core_addr_to_string_nz (addr), insn,
> -                *link ? "blr" : "br", *rn);
> +                *is_blr ? "blr" : "br", *rn);
> 
>       return 1;
>     }
> @@ -390,16 +390,15 @@ decode_br (CORE_ADDR addr, uint32_t insn, unsigned *link, unsigned *rn)
>    ADDR specifies the address of the opcode.
>    INSN specifies the opcode to test.
>    IS64 receives the 'sf' field from the decoded instruction.
> -   OP receives the 'op' field from the decoded instruction.
> +   IS_CBNZ receives the 'op' field from the decoded instruction.
>    RN receives the 'rn' field from the decoded instruction.
>    OFFSET receives the 'imm19' field from the decoded instruction.
> 
>    Return 1 if the opcodes matches and is decoded, otherwise 0.  */
> 
> static int
> -decode_cb (CORE_ADDR addr,
> -       uint32_t insn, int *is64, unsigned *op, unsigned *rn,
> -       int32_t *offset)
> +decode_cb (CORE_ADDR addr, uint32_t insn, int *is64, int *is_cbnz,
> +       unsigned *rn, int32_t *offset)
> {
>   if (decode_masked_match (insn, 0x7e000000, 0x34000000))
>     {
> @@ -408,14 +407,14 @@ decode_cb (CORE_ADDR addr,
> 
>       *rn = (insn >> 0) & 0x1f;
>       *is64 = (insn >> 31) & 0x1;
> -      *op = (insn >> 24) & 0x1;
> +      *is_cbnz = (insn >> 24) & 0x1;
>       *offset = extract_signed_bitfield (insn, 19, 5) << 2;
> 
>       if (aarch64_debug)
>    fprintf_unfiltered (gdb_stdlog,
>                "decode: 0x%s 0x%x %s 0x%s\n",
>                core_addr_to_string_nz (addr), insn,
> -                *op ? "cbnz" : "cbz",
> +                *is_cbnz ? "cbnz" : "cbz",
>                core_addr_to_string_nz (addr + *offset));
>       return 1;
>     }
> @@ -632,7 +631,7 @@ decode_stur (CORE_ADDR addr, uint32_t insn, int *is64, unsigned *rt,
> 
>    ADDR specifies the address of the opcode.
>    INSN specifies the opcode to test.
> -   OP receives the 'op' field from the decoded instruction.
> +   IS_TBNZ receives the 'op' field from the decoded instruction.
>    BIT receives the bit position field from the decoded instruction.
>    RT receives 'rt' field from the decoded instruction.
>    IMM receives 'imm' field from the decoded instruction.
> @@ -640,9 +639,8 @@ decode_stur (CORE_ADDR addr, uint32_t insn, int *is64, unsigned *rt,
>    Return 1 if the opcodes matches and is decoded, otherwise 0.  */
> 
> static int
> -decode_tb (CORE_ADDR addr,
> -       uint32_t insn, unsigned *op, unsigned *bit, unsigned *rt,
> -       int32_t *imm)
> +decode_tb (CORE_ADDR addr, uint32_t insn, int *is_tbnz, unsigned *bit,
> +       unsigned *rt, int32_t *imm)
> {
>   if (decode_masked_match (insn, 0x7e000000, 0x36000000))
>     {
> @@ -650,7 +648,7 @@ decode_tb (CORE_ADDR addr,
>       /* tbnz B011 0111 bbbb biii iiii iiii iiir rrrr */
> 
>       *rt = (insn >> 0) & 0x1f;
> -      *op = insn & (1 << 24);
> +      *is_tbnz = (insn >> 24) & 0x1;
>       *bit = ((insn >> (31 - 4)) & 0x20) | ((insn >> 19) & 0x1f);
>       *imm = extract_signed_bitfield (insn, 14, 5) << 2;
> 
> @@ -658,7 +656,7 @@ decode_tb (CORE_ADDR addr,
>    fprintf_unfiltered (gdb_stdlog,
>                "decode: 0x%s 0x%x %s x%u, #%u, 0x%s\n",
>                core_addr_to_string_nz (addr), insn,
> -                *op ? "tbnz" : "tbz", *rt, *bit,
> +                *is_tbnz ? "tbnz" : "tbz", *rt, *bit,
>                core_addr_to_string_nz (addr + *imm));
>       return 1;
>     }
> @@ -698,8 +696,9 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>       int32_t imm;
>       unsigned cond;
>       int is64;
> -      unsigned is_link;
> -      unsigned op;
> +      int is_link;
> +      int is_cbnz;
> +      int is_tbnz;
>       unsigned bit;
>       int32_t offset;
> 
> @@ -724,7 +723,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>      /* Stop analysis on branch.  */
>      break;
>    }
> -      else if (decode_cb (start, insn, &is64, &op, &rn, &offset))
> +      else if (decode_cb (start, insn, &is64, &is_cbnz, &rn, &offset))
>    {
>      /* Stop analysis on branch.  */
>      break;
> @@ -800,7 +799,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>             regs[rt2]);
>      regs[rn] = pv_add_constant (regs[rn], imm);
>    }
> -      else if (decode_tb (start, insn, &op, &bit, &rn, &offset))
> +      else if (decode_tb (start, insn, &is_tbnz, &bit, &rn, &offset))
>    {
>      /* Stop analysis on branch.  */
>      break;
> -- 
> 2.4.6
>
  

On 29/07/15 10:42, pinskia@gmail.com wrote:
> 
> 
> 
> 
>> On Jul 29, 2015, at 2:33 AM, Pierre Langlois <pierre.langlois@arm.com> wrote:
>>
>> Hi all,
>>
>> This patch cleans up the decoding functions using booleans when they can
>> decode two instructions.  The boolean argument is used to know which of
>> the two instructions was decoded.
>>
>> The instructions affected are BR/BLR, B/BL, CBZ/CBNZ and TBZ/TBNZ.
>>
>> These arguments would be named after a named bit in the instruction
>> encoding, this patch renames them to 'is_XXX'.  Furthermore, the
>> 'unsigned' type would be used to describe a boolean while
>> aarch64_decode_cb would use 'int' (see the 'is64' argument).  This patch
>> makes all booleans be 'int' and decoded bitfields be 'unsigned'.
> 
> Why not use the bool type instead? Since that seems like a better option and might even be better optimized. 

Hi Andrew,

I agree it'd be better, but I saw that using <stdbool.h> in GDB was removed
here because of issues building on Solaris and AiX:
https://sourceware.org/ml/gdb-patches/2015-02/msg00804.html

Thanks,
Pierre

> 
> Thanks,
> Andrew
> 
>>
>> Thanks,
>> Pierre
>>
>> gdb/ChangeLog:
>>
>>    * aarch64-tdep.c (decode_b): Rename link argument to is_bl.
>>    Change its type to int *.
>>    (decode_br): Rename link argument to is_blr.  Change its type to
>>    int *.
>>    (decode_cb): Rename op argument to is_cbnz.  Change its type to
>>    int *.
>>    (decode_tb): Rename op argument to is_tbnz.  Change its type to
>>    int *.  Set is_tbnz to either 1 or 0.
>>    (aarch64_analyze_prologue): Change type of is_link to int.  Add
>>    new variables is_cbnz and is_tbnz.  Adjust call to
>>    aarch64_decode_cb and aarch64_decode_tb.
>> ---
>> gdb/aarch64-tdep.c | 47 +++++++++++++++++++++++------------------------
>> 1 file changed, 23 insertions(+), 24 deletions(-)
>>
>> diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c
>> index cec4d3e..c722dc5 100644
>> --- a/gdb/aarch64-tdep.c
>> +++ b/gdb/aarch64-tdep.c
>> @@ -299,26 +299,26 @@ decode_adrp (CORE_ADDR addr, uint32_t insn, unsigned *rd)
>>
>>    ADDR specifies the address of the opcode.
>>    INSN specifies the opcode to test.
>> -   LINK receives the 'link' bit from the decoded instruction.
>> +   IS_BL receives the 'op' bit from the decoded instruction.
>>    OFFSET receives the immediate offset from the decoded instruction.
>>
>>    Return 1 if the opcodes matches and is decoded, otherwise 0.  */
>>
>> static int
>> -decode_b (CORE_ADDR addr, uint32_t insn, unsigned *link, int32_t *offset)
>> +decode_b (CORE_ADDR addr, uint32_t insn, int *is_bl, int32_t *offset)
>> {
>>   /* b  0001 01ii iiii iiii iiii iiii iiii iiii */
>>   /* bl 1001 01ii iiii iiii iiii iiii iiii iiii */
>>   if (decode_masked_match (insn, 0x7c000000, 0x14000000))
>>     {
>> -      *link = insn >> 31;
>> +      *is_bl = (insn >> 31) & 0x1;
>>       *offset = extract_signed_bitfield (insn, 26, 0) << 2;
>>
>>       if (aarch64_debug)
>>    fprintf_unfiltered (gdb_stdlog,
>>                "decode: 0x%s 0x%x %s 0x%s\n",
>>                core_addr_to_string_nz (addr), insn,
>> -                *link ? "bl" : "b",
>> +                *is_bl ? "bl" : "b",
>>                core_addr_to_string_nz (addr + *offset));
>>
>>       return 1;
>> @@ -358,27 +358,27 @@ decode_bcond (CORE_ADDR addr, uint32_t insn, unsigned *cond, int32_t *offset)
>>
>>    ADDR specifies the address of the opcode.
>>    INSN specifies the opcode to test.
>> -   LINK receives the 'link' bit from the decoded instruction.
>> +   IS_BLR receives the 'op' bit from the decoded instruction.
>>    RN receives the 'rn' field from the decoded instruction.
>>
>>    Return 1 if the opcodes matches and is decoded, otherwise 0.  */
>>
>> static int
>> -decode_br (CORE_ADDR addr, uint32_t insn, unsigned *link, unsigned *rn)
>> +decode_br (CORE_ADDR addr, uint32_t insn, int *is_blr, unsigned *rn)
>> {
>>   /*         8   4   0   6   2   8   4   0 */
>>   /* blr  110101100011111100000000000rrrrr */
>>   /* br   110101100001111100000000000rrrrr */
>>   if (decode_masked_match (insn, 0xffdffc1f, 0xd61f0000))
>>     {
>> -      *link = (insn >> 21) & 1;
>> +      *is_blr = (insn >> 21) & 1;
>>       *rn = (insn >> 5) & 0x1f;
>>
>>       if (aarch64_debug)
>>    fprintf_unfiltered (gdb_stdlog,
>>                "decode: 0x%s 0x%x %s 0x%x\n",
>>                core_addr_to_string_nz (addr), insn,
>> -                *link ? "blr" : "br", *rn);
>> +                *is_blr ? "blr" : "br", *rn);
>>
>>       return 1;
>>     }
>> @@ -390,16 +390,15 @@ decode_br (CORE_ADDR addr, uint32_t insn, unsigned *link, unsigned *rn)
>>    ADDR specifies the address of the opcode.
>>    INSN specifies the opcode to test.
>>    IS64 receives the 'sf' field from the decoded instruction.
>> -   OP receives the 'op' field from the decoded instruction.
>> +   IS_CBNZ receives the 'op' field from the decoded instruction.
>>    RN receives the 'rn' field from the decoded instruction.
>>    OFFSET receives the 'imm19' field from the decoded instruction.
>>
>>    Return 1 if the opcodes matches and is decoded, otherwise 0.  */
>>
>> static int
>> -decode_cb (CORE_ADDR addr,
>> -       uint32_t insn, int *is64, unsigned *op, unsigned *rn,
>> -       int32_t *offset)
>> +decode_cb (CORE_ADDR addr, uint32_t insn, int *is64, int *is_cbnz,
>> +       unsigned *rn, int32_t *offset)
>> {
>>   if (decode_masked_match (insn, 0x7e000000, 0x34000000))
>>     {
>> @@ -408,14 +407,14 @@ decode_cb (CORE_ADDR addr,
>>
>>       *rn = (insn >> 0) & 0x1f;
>>       *is64 = (insn >> 31) & 0x1;
>> -      *op = (insn >> 24) & 0x1;
>> +      *is_cbnz = (insn >> 24) & 0x1;
>>       *offset = extract_signed_bitfield (insn, 19, 5) << 2;
>>
>>       if (aarch64_debug)
>>    fprintf_unfiltered (gdb_stdlog,
>>                "decode: 0x%s 0x%x %s 0x%s\n",
>>                core_addr_to_string_nz (addr), insn,
>> -                *op ? "cbnz" : "cbz",
>> +                *is_cbnz ? "cbnz" : "cbz",
>>                core_addr_to_string_nz (addr + *offset));
>>       return 1;
>>     }
>> @@ -632,7 +631,7 @@ decode_stur (CORE_ADDR addr, uint32_t insn, int *is64, unsigned *rt,
>>
>>    ADDR specifies the address of the opcode.
>>    INSN specifies the opcode to test.
>> -   OP receives the 'op' field from the decoded instruction.
>> +   IS_TBNZ receives the 'op' field from the decoded instruction.
>>    BIT receives the bit position field from the decoded instruction.
>>    RT receives 'rt' field from the decoded instruction.
>>    IMM receives 'imm' field from the decoded instruction.
>> @@ -640,9 +639,8 @@ decode_stur (CORE_ADDR addr, uint32_t insn, int *is64, unsigned *rt,
>>    Return 1 if the opcodes matches and is decoded, otherwise 0.  */
>>
>> static int
>> -decode_tb (CORE_ADDR addr,
>> -       uint32_t insn, unsigned *op, unsigned *bit, unsigned *rt,
>> -       int32_t *imm)
>> +decode_tb (CORE_ADDR addr, uint32_t insn, int *is_tbnz, unsigned *bit,
>> +       unsigned *rt, int32_t *imm)
>> {
>>   if (decode_masked_match (insn, 0x7e000000, 0x36000000))
>>     {
>> @@ -650,7 +648,7 @@ decode_tb (CORE_ADDR addr,
>>       /* tbnz B011 0111 bbbb biii iiii iiii iiir rrrr */
>>
>>       *rt = (insn >> 0) & 0x1f;
>> -      *op = insn & (1 << 24);
>> +      *is_tbnz = (insn >> 24) & 0x1;
>>       *bit = ((insn >> (31 - 4)) & 0x20) | ((insn >> 19) & 0x1f);
>>       *imm = extract_signed_bitfield (insn, 14, 5) << 2;
>>
>> @@ -658,7 +656,7 @@ decode_tb (CORE_ADDR addr,
>>    fprintf_unfiltered (gdb_stdlog,
>>                "decode: 0x%s 0x%x %s x%u, #%u, 0x%s\n",
>>                core_addr_to_string_nz (addr), insn,
>> -                *op ? "tbnz" : "tbz", *rt, *bit,
>> +                *is_tbnz ? "tbnz" : "tbz", *rt, *bit,
>>                core_addr_to_string_nz (addr + *imm));
>>       return 1;
>>     }
>> @@ -698,8 +696,9 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>>       int32_t imm;
>>       unsigned cond;
>>       int is64;
>> -      unsigned is_link;
>> -      unsigned op;
>> +      int is_link;
>> +      int is_cbnz;
>> +      int is_tbnz;
>>       unsigned bit;
>>       int32_t offset;
>>
>> @@ -724,7 +723,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>>      /* Stop analysis on branch.  */
>>      break;
>>    }
>> -      else if (decode_cb (start, insn, &is64, &op, &rn, &offset))
>> +      else if (decode_cb (start, insn, &is64, &is_cbnz, &rn, &offset))
>>    {
>>      /* Stop analysis on branch.  */
>>      break;
>> @@ -800,7 +799,7 @@ aarch64_analyze_prologue (struct gdbarch *gdbarch,
>>             regs[rt2]);
>>      regs[rn] = pv_add_constant (regs[rn], imm);
>>    }
>> -      else if (decode_tb (start, insn, &op, &bit, &rn, &offset))
>> +      else if (decode_tb (start, insn, &is_tbnz, &bit, &rn, &offset))
>>    {
>>      /* Stop analysis on branch.  */
>>      break;
>> -- 
>> 2.4.6
>>
>
  

Pierre Langlois <pierre.langlois@arm.com> writes:

> 	* aarch64-tdep.c (decode_b): Rename link argument to is_bl.
> 	Change its type to int *.
> 	(decode_br): Rename link argument to is_blr.  Change its type to
> 	int *.
> 	(decode_cb): Rename op argument to is_cbnz.  Change its type to
> 	int *.
> 	(decode_tb): Rename op argument to is_tbnz.  Change its type to
> 	int *.  Set is_tbnz to either 1 or 0.
> 	(aarch64_analyze_prologue): Change type of is_link to int.  Add
> 	new variables is_cbnz and is_tbnz.  Adjust call to
> 	aarch64_decode_cb and aarch64_decode_tb.

Patch looks good to me.
  

On 30/07/15 12:24, Yao Qi wrote:
> Pierre Langlois <pierre.langlois@arm.com> writes:
> 
>> 	* aarch64-tdep.c (decode_b): Rename link argument to is_bl.
>> 	Change its type to int *.
>> 	(decode_br): Rename link argument to is_blr.  Change its type to
>> 	int *.
>> 	(decode_cb): Rename op argument to is_cbnz.  Change its type to
>> 	int *.
>> 	(decode_tb): Rename op argument to is_tbnz.  Change its type to
>> 	int *.  Set is_tbnz to either 1 or 0.
>> 	(aarch64_analyze_prologue): Change type of is_link to int.  Add
>> 	new variables is_cbnz and is_tbnz.  Adjust call to
>> 	aarch64_decode_cb and aarch64_decode_tb.
> 
> Patch looks good to me.
> 

Thanks Yao, I've pushed this in.

Thanks,
Pierre