[1/3] arm-tdep.c: Refactor arm_process_displaced_insn

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

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.