Re: [PATCH] [AArch64] Rename boolean arguments in decoding functions

[pinskia@gmail.com]

> 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
>