[PATCH] aarch64 sim big-endian support

[PATCH] aarch64 sim big-endian support

[Jim Wilson]

[-- Attachment #1: Type: text/plain, Size: 714 bytes --]

On aarch64, code is always little-endian, even when compiling
big-endian, so we need to force little-endian when reading
instructions.

Running the gcc C language testsuite, I get for an aarch64-elf target

# of expected passes            35433
# of unexpected failures        254
# of unsupported tests          131

and for an aarch64_be-elf target with the attached patch I get

# of expected passes            35200
# of unexpected failures        487
# of unsupported tests          131

so this simple patch gets us most of the way there.  I haven't tried
looking at the other problems yet.

I also have a dejagnu patch I wrote to make this work, which I will be
submitting to the dejagnu team shortly.

Jim

[-- Attachment #2: gdb-sim-aarch64.patch --]
[-- Type: text/x-patch, Size: 977 bytes --]

2016-06-02  Jim Wilson  <jim.wilson@linaro.org>

	sim/aarch64/
	* simulator.c (aarch64_step): New var saved_target_byte_order.  Force
	byte order to BFD_ENDIAN_LITTLE before pc read, then restore saved
	value.

diff --git a/sim/aarch64/simulator.c b/sim/aarch64/simulator.c
index 88cb03d..5a1814c 100644
--- a/sim/aarch64/simulator.c
+++ b/sim/aarch64/simulator.c
@@ -14078,12 +14078,18 @@ static bfd_boolean
 aarch64_step (sim_cpu *cpu)
 {
   uint64_t pc = aarch64_get_PC (cpu);
+  enum bfd_endian saved_target_byte_order;
 
   if (pc == TOP_LEVEL_RETURN_PC)
     return FALSE;
 
   aarch64_set_next_PC (cpu, pc + 4);
+
+  /* Code is always little-endian.  */
+  saved_target_byte_order = current_target_byte_order;
+  current_target_byte_order = BFD_ENDIAN_LITTLE;
   aarch64_get_instr (cpu) = aarch64_get_mem_u32 (cpu, pc);
+  current_target_byte_order = saved_target_byte_order;
 
   TRACE_INSN (cpu, " pc = %" PRIx64 " instr = %08x", pc,
 	      aarch64_get_instr (cpu));

Re: [PATCH] aarch64 sim big-endian support

[Jim Wilson]

ping

for the attachment, see
https://sourceware.org/ml/gdb-patches/2016-06/msg00046.html

On Thu, Jun 2, 2016 at 7:36 PM, Jim Wilson <jim.wilson@linaro.org> wrote:
> On aarch64, code is always little-endian, even when compiling
> big-endian, so we need to force little-endian when reading
> instructions.
>
> Running the gcc C language testsuite, I get for an aarch64-elf target
>
> # of expected passes            35433
> # of unexpected failures        254
> # of unsupported tests          131
>
> and for an aarch64_be-elf target with the attached patch I get
>
> # of expected passes            35200
> # of unexpected failures        487
> # of unsupported tests          131
>
> so this simple patch gets us most of the way there.  I haven't tried
> looking at the other problems yet.
>
> I also have a dejagnu patch I wrote to make this work, which I will be
> submitting to the dejagnu team shortly.
>
> Jim

Re: [PATCH] aarch64 sim big-endian support

[Mike Frysinger]

[-- Attachment #1: Type: text/plain, Size: 703 bytes --]

On 02 Jun 2016 19:36, Jim Wilson wrote:
>  aarch64_step (sim_cpu *cpu)
>  {
>    uint64_t pc = aarch64_get_PC (cpu);
> +  enum bfd_endian saved_target_byte_order;
>  
>    if (pc == TOP_LEVEL_RETURN_PC)
>      return FALSE;
>  
>    aarch64_set_next_PC (cpu, pc + 4);
> +
> +  /* Code is always little-endian.  */
> +  saved_target_byte_order = current_target_byte_order;
> +  current_target_byte_order = BFD_ENDIAN_LITTLE;
>    aarch64_get_instr (cpu) = aarch64_get_mem_u32 (cpu, pc);
> +  current_target_byte_order = saved_target_byte_order;

i don't think you should be messing with global state.  the sim core
has functions for reading raw opcodes if that's what you need.
-mike

[-- Attachment #2: Digital signature --]
[-- Type: application/pgp-signature, Size: 819 bytes --]

Re: [PATCH] aarch64 sim big-endian support

[Nick Clifton]

[-- Attachment #1: Type: text/plain, Size: 467 bytes --]

Hi Jim,

> ping

oops - sorry.

> for the attachment, see
> https://sourceware.org/ml/gdb-patches/2016-06/msg00046.html

Did you see Mike Frysinger's comment about not changing global
variables ?

I think that I agree with this comment, although I could not find
the raw opcode reading functions to which he was referring, (unless
he meant sim_core_read_buffer), so would you mind trying out this
variation of your patch to see if it works instead ?

Cheers
  Nick



[-- Attachment #2: aarch64-sim.patch --]
[-- Type: text/x-patch, Size: 1910 bytes --]

diff --git a/sim/aarch64/memory.c b/sim/aarch64/memory.c
index 50f4837..f7eea22 100644
--- a/sim/aarch64/memory.c
+++ b/sim/aarch64/memory.c
@@ -78,6 +78,20 @@ FETCH_FUNC32 (int32_t,   int16_t, s16, 2)
 FETCH_FUNC32 (uint32_t,  uint8_t, u8, 1)
 FETCH_FUNC32 (int32_t,    int8_t, s8, 1)
 
+/* Specialized version of aarch64_get_mem_u32 for fetching instructions
+   which are always held in little endian order even on big-endian
+   configured targets.  */
+
+uint32_t
+aarch64_get_instruction (sim_cpu *cpu, uint64_t address)
+{
+  uint32_t val = aarch64_get_mem_u32 (cpu, address);
+
+  if (CURRENT_TARGET_BYTE_ORDER == BFD_ENDIAN_BIG)
+    val = _SWAP_4 (val);
+  return val;
+}
+
 void
 aarch64_get_mem_long_double (sim_cpu *cpu, uint64_t address, FRegister *a)
 {
diff --git a/sim/aarch64/memory.h b/sim/aarch64/memory.h
index 3f63973..3559245 100644
--- a/sim/aarch64/memory.h
+++ b/sim/aarch64/memory.h
@@ -37,6 +37,7 @@ extern uint32_t     aarch64_get_mem_u8  (sim_cpu *, uint64_t);
 extern int32_t      aarch64_get_mem_s8  (sim_cpu *, uint64_t);
 extern void         aarch64_get_mem_blk (sim_cpu *, uint64_t, char *, unsigned);
 extern const char * aarch64_get_mem_ptr (sim_cpu *, uint64_t);
+extern uint32_t     aarch64_get_instruction (sim_cpu *, uint64_t);
 
 extern void         aarch64_set_mem_long_double (sim_cpu *, uint64_t, FRegister);
 extern void         aarch64_set_mem_u64 (sim_cpu *, uint64_t, uint64_t);
diff --git a/sim/aarch64/simulator.c b/sim/aarch64/simulator.c
index 88cb03d..c70fd96 100644
--- a/sim/aarch64/simulator.c
+++ b/sim/aarch64/simulator.c
@@ -14083,7 +14083,7 @@ aarch64_step (sim_cpu *cpu)
     return FALSE;
 
   aarch64_set_next_PC (cpu, pc + 4);
-  aarch64_get_instr (cpu) = aarch64_get_mem_u32 (cpu, pc);
+  aarch64_get_instr (cpu) = aarch64_get_instruction (cpu, pc);
 
   TRACE_INSN (cpu, " pc = %" PRIx64 " instr = %08x", pc,
 	      aarch64_get_instr (cpu));

Re: [PATCH] aarch64 sim big-endian support

[Jim Wilson]

[-- Attachment #1: Type: text/plain, Size: 3473 bytes --]

On Mon, Jun 13, 2016 at 5:38 AM, Nick Clifton <nickc@redhat.com> wrote:
> I think that I agree with this comment, although I could not find
> the raw opcode reading functions to which he was referring, (unless
> he meant sim_core_read_buffer), so would you mind trying out this
> variation of your patch to see if it works instead ?

I finally got back to this.  I don't see any raw read function other
than sim_core_read_buffer either.  A raw read is not quite what I
want, as I need a little-endian to host translation, but I can call
endian_le2h_4 to do the swap after the raw read.  The interface is a
little awkward, as sim_core_read_buffer stores into a buffer instead
of returning a pointer, so I need to store the instruction, and then
read it back out again, swap it, and store it back again.

An alternative solution might be to make a copy of sim-n-core.h, call
it sim-n-core-le.h, and then change all of the T2H_M/H2T_M calls into
LE2H_M/H2LE_M calls, along with a few other minor changes to complete
the conversion.  We can then call sim_core_read_le_aligned_N instead
of sim_core_read_aligned_N for the instruction loads.  Note that
big-endian aarch64 is not the only target with this problem.
big-endian ARMv7-A works the same way, and if we had an IA-64
simulator, it would work the same way too.  So there are other
potential users of these functions.  This is maybe a little overkill
though for now, as we don't need the unaligned and misaligned read
functions for aarch64/armv7-a/ia-64 instruction loads, and we don't
need the write functions either.  We only need the aligned read
functions.

I tried testing this for all four combinations of big/little endian
host/target with a hello world program, and discovered that the
big-endian host support is broken.  The problem is with the
GRegisterValue untion.  You have
typedef union GRegisterValue
{
  int8_t   s8;
...
  int64_t s64;
} GRegister;
On a little-endian host, the s8 member will match the low-byte of the
s64 member, which is what we want.  However, on a big-endian host, the
s8 member will match the high-byte of the u64 member, and the
simulator fails.  I can fix this by using an anonymous struct for the
big-endian case
typedef union GRegisterValue
{
  struct { int64_t :56; int8_t s8; };
...l
  sint64_t s64;
} GRegister;
There are other ways to fix this, but this just seemed to me like the
quickest and smallest patch that would make it work.  There may also
be other issues here, as I only tested an integer hello world program.

Fixing the problem this way means that we require either an ISO C 2011
compiler, or a compiler that supports GCC extensions to ISO C 1990 or
1999.  Otherwise, you may get an error for the anonymous structs.  Or
alternatively, it requires using a C++ compiler, as C++ added
anonymous structs long before C did.  I'm not sure how much of a
problem this will be.  If this is a serious problem, it could be fixed
by giving names to the structs, adding the structs to the little
endian side also with the field order switched, and then fixing all
users to use the new names for the fields.  That will be a bigger
patch.

With both changes, a hello world program works on all four
combinations of big/little host/target.

if you aren't happy with the cpustate.h change, it would be nice to
get an approval for just the simulator.c change, as that is the part I
care more about.  We can worry about how to fix the big-endian host
cpustate.h support later.

Jim

[-- Attachment #2: gdb-sim-aarch64.patch --]
[-- Type: text/x-patch, Size: 1895 bytes --]

2016-06-29  Jim Wilson  <jim.wilson@linaro.org>

	sim/aarch64/
	* cpustate.h: Include config.h.
	(union GRegisterValue): Add WORDS_BIGENDIAN check.  For big endian code
	use anonymous structs to align members.
	* simulator.c (aarch64_step): Use sim_core_read_buffer and
	endian_le2h_4 to read instruction from pc.

diff --git a/sim/aarch64/cpustate.h b/sim/aarch64/cpustate.h
index 07446a2..2754f7c 100644
--- a/sim/aarch64/cpustate.h
+++ b/sim/aarch64/cpustate.h
@@ -22,6 +22,7 @@
 #ifndef _CPU_STATE_H
 #define _CPU_STATE_H
 
+#include "config.h"
 #include <sys/types.h>
 #include <stdint.h>
 #include <inttypes.h>
@@ -133,6 +134,7 @@ typedef enum VReg
    an explicit extend.  */
 typedef union GRegisterValue
 {
+#if !WORDS_BIGENDIAN
   int8_t   s8;
   int16_t  s16;
   int32_t  s32;
@@ -141,6 +143,16 @@ typedef union GRegisterValue
   uint16_t u16;
   uint32_t u32;
   uint64_t u64;
+#else
+  struct { int64_t :56; int8_t s8; };
+  struct { int64_t :48; int16_t s16; };
+  struct { int64_t :32; int32_t s32; };
+  int64_t s64;
+  struct { uint64_t :56; uint8_t u8; };
+  struct { uint64_t :48; uint16_t u16; };
+  struct { uint64_t :32; uint32_t u32; };
+  uint64_t u64;
+#endif
 } GRegister;
 
 /* Float registers provide for storage of a single, double or quad
diff --git a/sim/aarch64/simulator.c b/sim/aarch64/simulator.c
index 88cb03d..8eb582a 100644
--- a/sim/aarch64/simulator.c
+++ b/sim/aarch64/simulator.c
@@ -14083,7 +14083,11 @@ aarch64_step (sim_cpu *cpu)
     return FALSE;
 
   aarch64_set_next_PC (cpu, pc + 4);
-  aarch64_get_instr (cpu) = aarch64_get_mem_u32 (cpu, pc);
+
+  /* Code is always little-endian.  */
+  sim_core_read_buffer (CPU_STATE (cpu), cpu, read_map,
+			&aarch64_get_instr (cpu), pc, 4);
+  aarch64_get_instr (cpu) = endian_le2h_4 (aarch64_get_instr (cpu));
 
   TRACE_INSN (cpu, " pc = %" PRIx64 " instr = %08x", pc,
 	      aarch64_get_instr (cpu));

Re: [PATCH] aarch64 sim big-endian support

[Nick Clifton]

Hi Jim,

> The problem is with the
> GRegisterValue untion.  You have
> typedef union GRegisterValue
> {
>   int8_t   s8;
> ...
>   int64_t s64;
> } GRegister;
> On a little-endian host, the s8 member will match the low-byte of the
> s64 member, which is what we want.  However, on a big-endian host, the
> s8 member will match the high-byte of the u64 member, and the
> simulator fails.

Ah, yes.  I had not considered this.

> Fixing the problem this way means that we require either an ISO C 2011
> compiler, or a compiler that supports GCC extensions to ISO C 1990 or
> 1999. 

I have no problems with this requirement.

I have now checked in your patch (to save time).  Thanks very much for
persuing this matter.

Cheers
  Nick