[PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding

[PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding

[Simon Marchi]

I am currently working on extracting the instruction decoding from the
displaced stepping support in arm-tdep.c, in order to share the functionality
with the upcoming fast tracepoint support.  I did a few refactors that helped
me correlate the code with the ARM Architecture Reference Manual.  I think the
change helps readability in general, and especially when you have the manual
open on the side.

The idea is to follow the the order of the manual, use the same names and do
the same "checks" (avoid using unnecessary shortcuts that make the code more
cryptic).

Simon Marchi (3):
  arm-tdep.c: Refactor arm_process_displaced_insn
  arm-tdep.c: Refactor arm_decode_dp_misc
  arm-tdep.c: Refactor arm_decode_media

 gdb/arm-tdep.c | 171 ++++++++++++++++++++++++++++++++++++++++-----------------
 1 file changed, 121 insertions(+), 50 deletions(-)

-- 
2.5.1

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

[Simon Marchi]

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

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index 6ac05f0..0a9c0f6 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -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)
-- 
2.5.1

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

[Yao Qi]

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.

-- 
Yao (齐尧)

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

[Simon Marchi]

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

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

[Yao Qi]

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.

-- 
Yao (齐尧)

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

[Simon Marchi]

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.

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);
-      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;
+    }
+  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;
 }
 
 static int
-- 
2.5.1

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

[Yao Qi]

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.

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

-- 
Yao (齐尧)

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

[Yao Qi]

On 11/02/16 11:52, Yao Qi wrote:
> 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.

s/ feel/ don't feel/

-- 
Yao (齐尧)

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

[Simon Marchi]

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

[PATCH 3/3] arm-tdep.c: Refactor arm_decode_media

[Simon Marchi]

Refactor arm_decode_media 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_decode_media): Refactor instruction decoding.
---
 gdb/arm-tdep.c | 30 ++++++++++++++++++++++--------
 1 file changed, 22 insertions(+), 8 deletions(-)

diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
index e17a1a4..bf5bc49 100644
--- a/gdb/arm-tdep.c
+++ b/gdb/arm-tdep.c
@@ -6625,49 +6625,63 @@ static int
 arm_decode_media (struct gdbarch *gdbarch, uint32_t insn,
 		  struct displaced_step_closure *dsc)
 {
-  switch (bits (insn, 20, 24))
+  uint8_t op1 = bits (insn, 20, 24);
+  uint8_t rd = bits (insn, 12, 15);
+  uint8_t op2 = bits (insn, 5, 7);
+  uint8_t rn = bits (insn, 0, 3);
+
+  switch (op1)
     {
     case 0x00: case 0x01: case 0x02: case 0x03:
+      /* Parallel addition and subtraction, signed  */
       return arm_copy_unmodified (gdbarch, insn, "parallel add/sub signed", dsc);
 
     case 0x04: case 0x05: case 0x06: case 0x07:
+      /* Parallel addition and subtraction, unsigned  */
       return arm_copy_unmodified (gdbarch, insn, "parallel add/sub unsigned", dsc);
 
     case 0x08: case 0x09: case 0x0a: case 0x0b:
     case 0x0c: case 0x0d: case 0x0e: case 0x0f:
+      /* Packing, unpacking, saturation, and reversal  */
       return arm_copy_unmodified (gdbarch, insn,
 			      "decode/pack/unpack/saturate/reverse", dsc);
 
     case 0x18:
-      if (bits (insn, 5, 7) == 0)  /* op2.  */
+      if (op2 == 0)
 	 {
-	  if (bits (insn, 12, 15) == 0xf)
+	  if (rd == 0xf)
+	    /* USAD8  */
 	    return arm_copy_unmodified (gdbarch, insn, "usad8", dsc);
 	  else
+	    /* USADA8  */
 	    return arm_copy_unmodified (gdbarch, insn, "usada8", dsc);
 	}
       else
-	 return arm_copy_undef (gdbarch, insn, dsc);
+	return arm_copy_undef (gdbarch, insn, dsc);
 
     case 0x1a: case 0x1b:
-      if (bits (insn, 5, 6) == 0x2)  /* op2[1:0].  */
+      if ((op2 & 0x3) == 0x2)
+	/* SBFX  */
 	return arm_copy_unmodified (gdbarch, insn, "sbfx", dsc);
       else
 	return arm_copy_undef (gdbarch, insn, dsc);
 
     case 0x1c: case 0x1d:
-      if (bits (insn, 5, 6) == 0x0)  /* op2[1:0].  */
+      if ((op2 & 0x3) == 0x0)
 	 {
-	  if (bits (insn, 0, 3) == 0xf)
+	  if (rn == 0xf)
+	    /* BFC  */
 	    return arm_copy_unmodified (gdbarch, insn, "bfc", dsc);
 	  else
+	    /* BFI  */
 	    return arm_copy_unmodified (gdbarch, insn, "bfi", dsc);
 	}
       else
 	return arm_copy_undef (gdbarch, insn, dsc);
 
     case 0x1e: case 0x1f:
-      if (bits (insn, 5, 6) == 0x2)  /* op2[1:0].  */
+      if ((op2 & 0x3) == 0x2)
+	/* UBFX  */
 	return arm_copy_unmodified (gdbarch, insn, "ubfx", dsc);
       else
 	return arm_copy_undef (gdbarch, insn, dsc);
-- 
2.5.1

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

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

> -  switch (bits (insn, 20, 24))
> +  uint8_t op1 = bits (insn, 20, 24);
> +  uint8_t rd = bits (insn, 12, 15);
> +  uint8_t op2 = bits (insn, 5, 7);
> +  uint8_t rn = bits (insn, 0, 3);
> +
> +  switch (op1)

op1 is only used once, I prefer using bits (insn, 20, 24) rather than
defining a new variable.

Other variables, like rd and rn, can be defined where they are used.

-- 
Yao (齐尧)

Re: [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding

[Yao Qi]

Simon Marchi <simon.marchi@ericsson.com> writes:

> I am currently working on extracting the instruction decoding from the
> displaced stepping support in arm-tdep.c, in order to share the functionality
> with the upcoming fast tracepoint support.  I did a few refactors that helped
> me correlate the code with the ARM Architecture Reference Manual.  I think the
> change helps readability in general, and especially when you have the manual
> open on the side.
>
> The idea is to follow the the order of the manual, use the same names and do
> the same "checks" (avoid using unnecessary shortcuts that make the code more
> cryptic).

It is good to match the code to the manual.  The instruction encoding
can't change, but the order or the category of instruction _may_ change in
the manual in the future.  I am not the manual writer, but writers may
want to refactor the doc in the future too :)

Although your change is code refactor, better to run tests.

-- 
Yao (齐尧)

Re: [PATCH 0/3] Minor refactorings in arm-tdep.c instruction decoding

[Simon Marchi]

On 16-02-11 06:17 AM, Yao Qi wrote:
> Simon Marchi <simon.marchi@ericsson.com> writes:
> 
>> I am currently working on extracting the instruction decoding from the
>> displaced stepping support in arm-tdep.c, in order to share the functionality
>> with the upcoming fast tracepoint support.  I did a few refactors that helped
>> me correlate the code with the ARM Architecture Reference Manual.  I think the
>> change helps readability in general, and especially when you have the manual
>> open on the side.
>>
>> The idea is to follow the the order of the manual, use the same names and do
>> the same "checks" (avoid using unnecessary shortcuts that make the code more
>> cryptic).
> 
> It is good to match the code to the manual.  The instruction encoding
> can't change, but the order or the category of instruction _may_ change in
> the manual in the future.  I am not the manual writer, but writers may
> want to refactor the doc in the future too :)
> 
> Although your change is code refactor, better to run tests.

Yes, that is the part that is worrying me.  Since we don't have unit tests,
I am too afraid to break things.

I'll forget this patch set for the time being.  When the code is extracted to
its own file and decoupled from the core of GDB, maybe there will be some way
to write some kind of unit tests that ensure that the refactor is safe.

Thanks for reviewing anyway!