[1/3] arm-tdep.c: Refactor arm_process_displaced_insn
Commit Message
Refactor arm_process_displaced_insn to make it more readable. The
new layout matches very closely the description in the ARM Architecture
Reference Manual. It uses the same order and same nomenclature.
gdb/ChangeLog:
* arm-tdep.c (arm_process_displaced_insn): Refactor instruction
decoding.
---
gdb/arm-tdep.c | 68 ++++++++++++++++++++++++++++++++++++++++++----------------
1 file changed, 50 insertions(+), 18 deletions(-)
Comments
Simon Marchi <simon.marchi@ericsson.com> writes:
> - if ((insn & 0xf0000000) == 0xf0000000)
> - err = arm_decode_unconditional (gdbarch, insn, regs, dsc);
> - else switch (((insn & 0x10) >> 4) | ((insn & 0xe000000) >> 24))
> + cond = bits (insn, 28, 31);
Variable 'cond' is only used once, so don't need to define it. This is
my personal flavour.
> + op1 = bits (insn, 25, 27);
> + op = bit (insn, 4);
> +
> + if (cond != 0xf)
if (bits (insn, 28, 31) != INST_NV)
this is consistent with other places in arm-tdep.c
> {
> - case 0x0: case 0x1: case 0x2: case 0x3:
> - err = arm_decode_dp_misc (gdbarch, insn, regs, dsc);
> - break;
> + switch (op1)
> + {
> + case 0x0:
> + case 0x1:
> + /* Data-processing and miscellaneous instructions */
> + err = arm_decode_dp_misc (gdbarch, insn, regs, dsc);
> + break;
>
> - case 0x4: case 0x5: case 0x6:
> - err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc);
> - break;
> + case 0x2:
> + /* Load/store word and unsigned byte */
> + err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc);
> + break;
>
> - case 0x7:
> - err = arm_decode_media (gdbarch, insn, dsc);
> - break;
> + case 0x3:
> + if (op == 0)
'op' is only used here, let us define it in this block, or use
'bit (insn, 4)' instead.
On 16-02-11 06:21 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
>
>> - if ((insn & 0xf0000000) == 0xf0000000)
>> - err = arm_decode_unconditional (gdbarch, insn, regs, dsc);
>> - else switch (((insn & 0x10) >> 4) | ((insn & 0xe000000) >> 24))
>> + cond = bits (insn, 28, 31);
>
> Variable 'cond' is only used once, so don't need to define it. This is
> my personal flavour.
Well, my goal was to use variables with names that refer to these tables:
http://nova.polymtl.ca/~simark/ss/fileJVxJNx.png
(ARM Architecture Reference Manual, section A5.1)
If you only use the bits (insn, 28, 31) notation, I think you lose readability,
because the you have to do one more indirection in the doc, to go see what those
bits mean.
>> + op1 = bits (insn, 25, 27);
>> + op = bit (insn, 4);
>> +
>> + if (cond != 0xf)
>
> if (bits (insn, 28, 31) != INST_NV)
>
> this is consistent with other places in arm-tdep.c
I agree, if there is a define for that it should be used. What does _NV stand
for though?
>> {
>> - case 0x0: case 0x1: case 0x2: case 0x3:
>> - err = arm_decode_dp_misc (gdbarch, insn, regs, dsc);
>> - break;
>> + switch (op1)
>> + {
>> + case 0x0:
>> + case 0x1:
>> + -/* Data-processing and miscellaneous instructions */
>> + err = arm_decode_dp_misc (gdbarch, insn, regs, dsc);
>> + break;
>>
>> - case 0x4: case 0x5: case 0x6:
>> - err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc);
>> - break;
>> + case 0x2:
>> + /* Load/store word and unsigned byte */
>> + err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc);
>> + break;
>>
>> - case 0x7:
>> - err = arm_decode_media (gdbarch, insn, dsc);
>> - break;
>> + case 0x3:
>> + if (op == 0)
>
> 'op' is only used here, let us define it in this block, or use
> 'bit (insn, 4)' instead.
Ok for moving it, but I would suggest keeping the variable op, for
the same reason as cond mentioned above.
Thanks,
Simon
Simon Marchi <simon.marchi@ericsson.com> writes:
> Well, my goal was to use variables with names that refer to these tables:
>
> http://nova.polymtl.ca/~simark/ss/fileJVxJNx.png
> (ARM Architecture Reference Manual, section A5.1)
Yes, I clearly understand your goal, but I don't think the change is
necessary. However, I can't see anything harmful or negative in this
patch, and looks the patch is useful in terms of helping you reference
the doc easily, I am OK.
>
> If you only use the bits (insn, 28, 31) notation, I think you lose readability,
> because the you have to do one more indirection in the doc, to go see what those
> bits mean.
but if you write code like "if (bits (insn, 28, 31) != INST_NV)", people
do understand what those bits mean.
>>> + op1 = bits (insn, 25, 27);
>>> + op = bit (insn, 4);
>>> +
>>> + if (cond != 0xf)
>>
>> if (bits (insn, 28, 31) != INST_NV)
>>
>> this is consistent with other places in arm-tdep.c
>
> I agree, if there is a define for that it should be used. What does _NV stand
> for though?
NV means Never.
>>
>> 'op' is only used here, let us define it in this block, or use
>> 'bit (insn, 4)' instead.
>
> Ok for moving it, but I would suggest keeping the variable op, for
> the same reason as cond mentioned above.
OK, that is fine, since this is the personal flavour of writing code.
@@ -7495,6 +7495,7 @@ arm_process_displaced_insn (struct gdbarch *gdbarch, CORE_ADDR from,
int err = 0;
enum bfd_endian byte_order_for_code = gdbarch_byte_order_for_code (gdbarch);
uint32_t insn;
+ uint8_t cond, op, op1;
/* Most displaced instructions use a 1-instruction scratch space, so set this
here and override below if/when necessary. */
@@ -7515,29 +7516,60 @@ arm_process_displaced_insn (struct gdbarch *gdbarch, CORE_ADDR from,
"at %.8lx\n", (unsigned long) insn,
(unsigned long) from);
- if ((insn & 0xf0000000) == 0xf0000000)
- err = arm_decode_unconditional (gdbarch, insn, regs, dsc);
- else switch (((insn & 0x10) >> 4) | ((insn & 0xe000000) >> 24))
+ cond = bits (insn, 28, 31);
+ op1 = bits (insn, 25, 27);
+ op = bit (insn, 4);
+
+ if (cond != 0xf)
{
- case 0x0: case 0x1: case 0x2: case 0x3:
- err = arm_decode_dp_misc (gdbarch, insn, regs, dsc);
- break;
+ switch (op1)
+ {
+ case 0x0:
+ case 0x1:
+ /* Data-processing and miscellaneous instructions */
+ err = arm_decode_dp_misc (gdbarch, insn, regs, dsc);
+ break;
- case 0x4: case 0x5: case 0x6:
- err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc);
- break;
+ case 0x2:
+ /* Load/store word and unsigned byte */
+ err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc);
+ break;
- case 0x7:
- err = arm_decode_media (gdbarch, insn, dsc);
- break;
+ case 0x3:
+ if (op == 0)
+ {
+ /* Load/store word and unsigned byte */
+ err = arm_decode_ld_st_word_ubyte (gdbarch, insn, regs, dsc);
+ }
+ else
+ {
+ /* Media instructions */
+ err = arm_decode_media (gdbarch, insn, dsc);
+ }
+ break;
- case 0x8: case 0x9: case 0xa: case 0xb:
- err = arm_decode_b_bl_ldmstm (gdbarch, insn, regs, dsc);
- break;
+ case 0x4:
+ case 0x5:
+ /* Branch, branch with link, and block data transfer */
+ err = arm_decode_b_bl_ldmstm (gdbarch, insn, regs, dsc);
+ break;
- case 0xc: case 0xd: case 0xe: case 0xf:
- err = arm_decode_svc_copro (gdbarch, insn, to, regs, dsc);
- break;
+ case 0x6:
+ case 0x7:
+ /* Coprocessor instructions, and Supervisor Call */
+ err = arm_decode_svc_copro (gdbarch, insn, to, regs, dsc);
+ break;
+
+ default:
+ internal_error (__FILE__, __LINE__,
+ _("arm_process_displaced_insn: Missing case"));
+ break;
+ }
+ }
+ else
+ {
+ /* Unconditional instructions */
+ err = arm_decode_unconditional (gdbarch, insn, regs, dsc);
}
if (err)