Re: [PATCH] [Arm] Fix endianness handling for arm record self tests

[Andrew Burgess via Gdb-patches <gdb-patches@sourceware.org>]

Luis Machado via Gdb-patches <gdb-patches@sourceware.org> writes:

> The arm record tests handle 16-bit and 32-bit thumb instructions, but the
> code is laid out in a way that handles the 32-bit thumb instructions as
> two 16-bit parts.
>
> This is fine, but it is prone to host-endianness issues given how the two
> 16-bit parts are stored and how they are accessed later on. Arm is
> little-endian by default, so running this test with a GDB built with
> --enable-targets=all and on a big endian host will run into the following:
>
> Running selftest arm-record.
> Process record and replay target doesn't support syscall number -2036195
> Process record does not support instruction 0x7f70ee1d at address 0x0.
> Self test failed: self-test failed at ../../binutils-gdb/gdb/arm-tdep.c:14482
>
> Investigating this a bit further, there seems to be a chance to do a simple
> fix through a type template, using uint16_t for 16-bit thumb instructions
> and uint32_t for 32-bit thumb instructions.
>
> This patch implements this.
>
> Bug: https://sourceware.org/bugzilla/show_bug.cgi?id=29432
> ---
>  gdb/arm-tdep.c | 32 ++++++++++++++++++--------------
>  1 file changed, 18 insertions(+), 14 deletions(-)
>
> diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c
> index cf8b610a381..57b865a0819 100644
> --- a/gdb/arm-tdep.c
> +++ b/gdb/arm-tdep.c
> @@ -14387,14 +14387,18 @@ decode_insn (abstract_memory_reader &reader,
>  #if GDB_SELF_TEST
>  namespace selftests {
>  
> -/* Provide both 16-bit and 32-bit thumb instructions.  */
> +/* Provide both 16-bit and 32-bit thumb instructions.
>  
> +   For 16-bit Thumb instructions, an array of uint16_t should be used.
> +   For 32-bit Thumb instructions, an array of uint32_t should be used.  */
> +
> +template<typename T>
>  class instruction_reader_thumb : public abstract_memory_reader
>  {
>  public:
>    template<size_t SIZE>
>    instruction_reader_thumb (enum bfd_endian endian,
> -			    const uint16_t (&insns)[SIZE])
> +			    const T (&insns)[SIZE])
>      : m_endian (endian), m_insns (insns), m_insns_size (SIZE)
>    {}
>  
> @@ -14404,18 +14408,14 @@ class instruction_reader_thumb : public abstract_memory_reader
>      SELF_CHECK (memaddr % 2 == 0);
>      SELF_CHECK ((memaddr / 2) < m_insns_size);

I was expecting this '/ 2' to need updating here.  If memaddr is an octet
address, then the '/ 2' converts to a 16-bit chunk address, which is
fine if T is uint16_t, but surely is wrong when T is uint32_t...

>  
> -    store_unsigned_integer (buf, 2, m_endian, m_insns[memaddr / 2]);
> -    if (len == 4)
> -      {
> -	store_unsigned_integer (&buf[2], 2, m_endian,
> -				m_insns[memaddr / 2 + 1]);
> -      }
> +    store_unsigned_integer (buf, sizeof (T), m_endian, m_insns[memaddr / 2]);

And the same here.

> +
>      return true;
>    }
>  
>  private:
>    enum bfd_endian m_endian;
> -  const uint16_t *m_insns;
> +  const T *m_insns;
>    size_t m_insns_size;
>  };
>  
> @@ -14436,6 +14436,8 @@ arm_record_test (void)
>      memset (&arm_record, 0, sizeof (arm_insn_decode_record));
>      arm_record.gdbarch = gdbarch;
>  
> +    /* Use the endian-free representation of the instructions here.  The test
> +       will handle endianness conversions.  */
>      static const uint16_t insns[] = {
>        /* db b2	uxtb	r3, r3 */
>        0xb2db,
> @@ -14444,7 +14446,7 @@ arm_record_test (void)
>      };
>  
>      enum bfd_endian endian = gdbarch_byte_order_for_code (arm_record.gdbarch);
> -    instruction_reader_thumb reader (endian, insns);
> +    instruction_reader_thumb<uint16_t> reader (endian, insns);

I wonder if there's an alternative fix here?
gdbarch_byte_order_for_code returns a value such that READER will
correctly read instructions from arm instruction memory, right?  Which
happens to be little-endian.

However, we're not reading from arm instruction memory, but we are
instead reading from host memory.

On many targets, host memory also happens to be little-endian, thus
gdbarch_byte_order_for_code is still correct.

But, could we not instead pass in a value here that represents the host
memory order instead, then maybe READER will just do the right thing?

Thanks,
Andrew

>      int ret = decode_insn (reader, &arm_record, THUMB_RECORD,
>  			   THUMB_INSN_SIZE_BYTES);
>  
> @@ -14470,13 +14472,15 @@ arm_record_test (void)
>      memset (&arm_record, 0, sizeof (arm_insn_decode_record));
>      arm_record.gdbarch = gdbarch;
>  
> -    static const uint16_t insns[] = {
> -      /* 1d ee 70 7f	 mrc	15, 0, r7, cr13, cr0, {3} */
> -      0xee1d, 0x7f70,
> +    /* Use the endian-free representation of the instruction here.  The test
> +       will handle endianness conversions.  */
> +    static const uint32_t insns[] = {
> +      /* mrc	15, 0, r7, cr13, cr0, {3} */
> +      0x7f70ee1d,
>      };
>  
>      enum bfd_endian endian = gdbarch_byte_order_for_code (arm_record.gdbarch);
> -    instruction_reader_thumb reader (endian, insns);
> +    instruction_reader_thumb<uint32_t> reader (endian, insns);
>      int ret = decode_insn (reader, &arm_record, THUMB2_RECORD,
>  			   THUMB2_INSN_SIZE_BYTES);
>  
> -- 
> 2.25.1