Re: [PATCH 2/3] arm-tdep.c: Refactor arm_decode_dp_misc

[Simon Marchi <simon dot marchi at ericsson dot com>]

On 16-02-11 06:52 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> Refactor arm_decode_dp_misc 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.
> 
> As I mentioned in the reply to the patch cover letter, the manual may
> adjust the layout in the future.  For example, the manual lists
> instructions whose OP is 0 first, but it may change to list instructions
> whose OP is 1 first in the future.  IMO, we don't have to 100% match the
> code to the manual.

Indeed, but I think there is very little chance that they change the order
just for fun.  And in the mean time, I think it can help people who want
to read the code to learn or verify it.

>>
>> gdb/ChangeLog:
>>
>> 	* arm-tdep.c (arm_decode_dp_misc): Refactor instruction decoding.
>> ---
>>  gdb/arm-tdep.c | 73 +++++++++++++++++++++++++++++++++++++++-------------------
>>  1 file changed, 49 insertions(+), 24 deletions(-)
>>
>> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
>> index 0a9c0f6..e17a1a4 100644
>> --- a/gdb/arm-tdep.c
>> +++ b/gdb/arm-tdep.c
>> @@ -6517,45 +6517,70 @@ arm_decode_dp_misc (struct gdbarch *gdbarch, uint32_t insn,
>>  		    struct regcache *regs,
>>  		    struct displaced_step_closure *dsc)
>>  {
>> -  if (bit (insn, 25))
>> -    switch (bits (insn, 20, 24))
>> -      {
>> -      case 0x10:
>> -	return arm_copy_unmodified (gdbarch, insn, "movw", dsc);
>> -
>> -      case 0x14:
>> -	return arm_copy_unmodified (gdbarch, insn, "movt", dsc);
>> +  uint8_t op = bit (insn, 25);
>> +  uint8_t op1 = bits (insn, 20, 24);
>> +  uint8_t op2 = bits (insn, 4, 7);
>>  
>> -      case 0x12: case 0x16:
>> -	return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc);
>> -
>> -      default:
>> -	return arm_copy_alu_imm (gdbarch, insn, regs, dsc);
>> -      }
>> -  else
>> +  if (op == 0)
>>      {
>> -      uint32_t op1 = bits (insn, 20, 24), op2 = bits (insn, 4, 7);
>> -
>>        if ((op1 & 0x19) != 0x10 && (op2 & 0x1) == 0x0)
>> +	/* Data-processing (register)  */
>>  	return arm_copy_alu_reg (gdbarch, insn, regs, dsc);
>>        else if ((op1 & 0x19) != 0x10 && (op2 & 0x9) == 0x1)
>> +	/* Data-processing (register-shifted register)  */
>>  	return arm_copy_alu_shifted_reg (gdbarch, insn, regs, dsc);
>>        else if ((op1 & 0x19) == 0x10 && (op2 & 0x8) == 0x0)
>> +	/* Miscellaneous instructions  */
>>  	return arm_decode_miscellaneous (gdbarch, insn, regs, dsc);
>>        else if ((op1 & 0x19) == 0x10 && (op2 & 0x9) == 0x8)
>> +	/* Halfword multiply and multiply accumulate  */
>>  	return arm_copy_unmodified (gdbarch, insn, "halfword mul/mla", dsc);
>>        else if ((op1 & 0x10) == 0x00 && op2 == 0x9)
>> +	/* Multiply and multiply accumulate  */
>>  	return arm_copy_unmodified (gdbarch, insn, "mul/mla", dsc);
>>        else if ((op1 & 0x10) == 0x10 && op2 == 0x9)
>> +	/* Synchronization primitives  */
>>  	return arm_copy_unmodified (gdbarch, insn, "synch", dsc);
> 
> These added comments are helpful.
> 
>> -      else if (op2 == 0xb || (op2 & 0xd) == 0xd)
>> -	/* 2nd arg means "unprivileged".  */
>> -	return arm_copy_extra_ld_st (gdbarch, insn, (op1 & 0x12) == 0x02, regs,
>> -				     dsc);
>> +      else if ((op1 & 0x12) != 0x2 && op2 == 0xb)
>> +	/* Extra load/store instructions  */
>> +	return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc);
>> +      else if ((op1 & 0x12) != 0x2 && (op2 & 0xd) == 0xd)
>> +	/* Extra load/store instructions  */
>> +	return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc);
>> +      else if ((op1 & 0x13) == 0x2 && (op2 & 0xd) == 0xd)
>> +	/* Extra load/store instructions  */
>> +	return arm_copy_extra_ld_st (gdbarch, insn, 0, regs, dsc);
>> +      else if ((op1 & 0x12) == 0x2 && op2 == 0xd)
>> +	/* Extra load/store instructions, unprivileged  */
>> +	return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc);
>> +      else if ((op1 & 0x13) == 0x3 && (op2 & 0xd) == 0xd)
>> +	/* Extra load/store instructions, unprivileged  */
>> +	return arm_copy_extra_ld_st (gdbarch, insn, 1, regs, dsc);
>> +      else
>> +	return 1;
> 
> However, I don't see how helpful or useful the changes above are.
>
>> +    }
>> +  else
>> +    {
>> +      switch (op1)
>> +        {
>> +        default:
>> +          /* Data-processing (immediate)  */
>> +	  return arm_copy_alu_imm (gdbarch, insn, regs, dsc);
>> +
>> +        case 0x10:
>> +          /* 16-bit immediate load, MOV (immediate)  */
>> +          return arm_copy_unmodified (gdbarch, insn, "movw", dsc);
>> +
>> +        case 0x14:
>> +          /* High halfword 16-bit immediate load, MOVT  */
>> +	  return arm_copy_unmodified (gdbarch, insn, "movt", dsc);
>> +
>> +	case 0x12:
>> +	case 0x16:
>> +	  /* MSR (immediate), and hints  */
>> +	  return arm_copy_unmodified (gdbarch, insn, "msr imm", dsc);
>> +        }
>>      }
>> -
>> -  /* Should be unreachable.  */
>> -  return 1;
>>  }
> 
> In short, I don't see how this patch improve the readability of the
> code, and I feel hard mapping the code to the manual.

Just to be sure that we are referring to the same manual, I am using this:

http://nova.polymtl.ca/~simark/ss/fileWKxYqt.png
Section A5.2

If we take the current code for when op == 0:

      uint32_t op1 = bits (insn, 20, 24), op2 = bits (insn, 4, 7);

      if ((op1 & 0x19) != 0x10 && (op2 & 0x1) == 0x0)
	return arm_copy_alu_reg (gdbarch, insn, regs, dsc);
      else if ((op1 & 0x19) != 0x10 && (op2 & 0x9) == 0x1)
	return arm_copy_alu_shifted_reg (gdbarch, insn, regs, dsc);
      else if ((op1 & 0x19) == 0x10 && (op2 & 0x8) == 0x0)
	return arm_decode_miscellaneous (gdbarch, insn, regs, dsc);
      else if ((op1 & 0x19) == 0x10 && (op2 & 0x9) == 0x8)
	return arm_copy_unmodified (gdbarch, insn, "halfword mul/mla", dsc);
      else if ((op1 & 0x10) == 0x00 && op2 == 0x9)
	return arm_copy_unmodified (gdbarch, insn, "mul/mla", dsc);
      else if ((op1 & 0x10) == 0x10 && op2 == 0x9)
	return arm_copy_unmodified (gdbarch, insn, "synch", dsc);
      else if (op2 == 0xb || (op2 & 0xd) == 0xd)
	/* 2nd arg means "unpriveleged".  */
	return arm_copy_extra_ld_st (gdbarch, insn, (op1 & 0x12) == 0x02, regs,
				     dsc);

Maybe it's just me being a bit slow with bitwise operations, but it's not obvious
at all that "(op2 == 0xb || (op2 & 0xd) == 0xd)" matches exactly (not less and no
more) the possibles values listed for "Extra load/store instructions".  It's also
not obvious that "(op1 & 0x12) == 0x02" manage to differentiate between the op1
values for privileged vs unprivileged.  I mean, I could spend time doing those
computations on paper and would probably realize that it's equivalent.  But
if each condition in the code matches exactly each contidion in the manual, it's
easy to see follow.

For example, I think it's relatively easy to verify that

  else if ((op1 & 0x13) == 0x3 && (op2 & 0xd) == 0xd)

matches the condition from the manual

  op1     op2    Instruction
  0xx11   11x1   Extra load/store instructions, unprivileged