diff mbox series

[v1,4/8] aarch64: improve debuggability on array of enum

Message ID 20241023104816.501176-5-matthieu.longo@arm.com
State Accepted
Headers
Series small refactorings before posting PAuth_LR patch series |

Checks

Context Check Description
linaro-tcwg-bot/tcwg_binutils_build--master-arm success Build passed
linaro-tcwg-bot/tcwg_binutils_build--master-aarch64 success Build passed
linaro-tcwg-bot/tcwg_binutils_check--master-aarch64 success Test passed
linaro-tcwg-bot/tcwg_binutils_check--master-arm success Test passed

Commit Message

Matthieu Longo Oct. 23, 2024, 10:48 a.m. UTC 
  The current space optmization on enum aarch64_opn_qualifier forced its
encoding using an unsigned char. This "hard-coded" optimization has the
bad consequence of making the array of such enums being completely
unreadable when debugging with GDB because the enum type is lost along
the way.
Keeping this space optimization, and the enum type as well, is possible
when the declaration of the enum is tagged with attribute((packed)).
attribute((packed)) is a GNU extension, and is wrapped in the macro
ATTRIBUTE_PACKED (defined in ansidecl.h), and should be used instead.
---
 include/opcode/aarch64.h | 6 +++---
 opcodes/aarch64-opc.c    | 6 +++---
 2 files changed, 6 insertions(+), 6 deletions(-)

Comments

Richard Earnshaw (lists) Nov. 5, 2024, 5:29 p.m. UTC | #1 
On 23/10/2024 11:48, Matthieu Longo wrote:
> The current space optmization on enum aarch64_opn_qualifier forced its
> encoding using an unsigned char. This "hard-coded" optimization has the
> bad consequence of making the array of such enums being completely
> unreadable when debugging with GDB because the enum type is lost along
> the way.
> Keeping this space optimization, and the enum type as well, is possible
> when the declaration of the enum is tagged with attribute((packed)).
> attribute((packed)) is a GNU extension, and is wrapped in the macro
> ATTRIBUTE_PACKED (defined in ansidecl.h), and should be used instead.
> ---
>  include/opcode/aarch64.h | 6 +++---
>  opcodes/aarch64-opc.c    | 6 +++---
>  2 files changed, 6 insertions(+), 6 deletions(-)
> 
> diff --git a/include/opcode/aarch64.h b/include/opcode/aarch64.h
> index c96bad61ca0..1955ca45c67 100644
> --- a/include/opcode/aarch64.h
> +++ b/include/opcode/aarch64.h
> @@ -985,7 +985,7 @@ enum aarch64_opnd_qualifier
>  
>    /* Special qualifier used for indicating error in qualifier retrieval.  */
>    AARCH64_OPND_QLF_ERR,
> -};
> +} ATTRIBUTE_PACKED;

This is one of those cases where C++ really does do this better: "enum aarch64_opnd_qualifier : uint8_t".

But while we are still using C, this is OK.

R.
diff mbox series

Patch

diff --git a/include/opcode/aarch64.h b/include/opcode/aarch64.h
index c96bad61ca0..1955ca45c67 100644
--- a/include/opcode/aarch64.h
+++ b/include/opcode/aarch64.h
@@ -985,7 +985,7 @@  enum aarch64_opnd_qualifier
 
   /* Special qualifier used for indicating error in qualifier retrieval.  */
   AARCH64_OPND_QLF_ERR,
-};
+} ATTRIBUTE_PACKED;
 
 /* Instruction class.  */
 
@@ -1236,8 +1236,8 @@  enum err_type
 #define AARCH64_MAX_OPND_NUM 7
 /* Maximum number of qualifier sequences an instruction can have.  */
 #define AARCH64_MAX_QLF_SEQ_NUM 10
-/* Operand qualifier typedef; optimized for the size.  */
-typedef unsigned char aarch64_opnd_qualifier_t;
+/* Operand qualifier typedef  */
+typedef enum aarch64_opnd_qualifier aarch64_opnd_qualifier_t;
 /* Operand qualifier sequence typedef.  */
 typedef aarch64_opnd_qualifier_t	\
 	  aarch64_opnd_qualifier_seq_t [AARCH64_MAX_OPND_NUM];
diff --git a/opcodes/aarch64-opc.c b/opcodes/aarch64-opc.c
index 05e1a248622..93ae8767dfe 100644
--- a/opcodes/aarch64-opc.c
+++ b/opcodes/aarch64-opc.c
@@ -768,9 +768,9 @@  aarch64_get_expected_qualifier (const aarch64_opnd_qualifier_seq_t *qseq_list,
      it can mean no qualifier for the operand, or the qualifer sequence is
      not in use (when all qualifiers in the sequence are NILs), we have to
      handle this special case here.  */
-  if (known_qlf == AARCH64_OPND_NIL)
+  if (((enum aarch64_opnd) known_qlf) == AARCH64_OPND_NIL)
     {
-      assert (qseq_list[0][known_idx] == AARCH64_OPND_NIL);
+      assert (((enum aarch64_opnd) qseq_list[0][known_idx]) == AARCH64_OPND_NIL);
       return qseq_list[0][idx];
     }
 
@@ -781,7 +781,7 @@  aarch64_get_expected_qualifier (const aarch64_opnd_qualifier_seq_t *qseq_list,
 	  if (saved_i != -1)
 	    /* More than one sequences are found to have KNOWN_QLF at
 	       KNOWN_IDX.  */
-	    return AARCH64_OPND_NIL;
+	    return AARCH64_OPND_QLF_NIL;
 	  saved_i = i;
 	}
     }