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

Pierre Langlois
2015-07-29T10:44:26Z

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