Signed-off-by: Stefan Weil <sw@weilnetz.de>
---
 tcg/tci.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

diff --git a/tcg/tci.c b/tcg/tci.c
index 33edca1903..20b0715b6e 100644
--- a/tcg/tci.c
+++ b/tcg/tci.c
@@ -1,7 +1,7 @@
 /*
  * Tiny Code Interpreter for QEMU
  *
- * Copyright (c) 2009, 2011, 2016 Stefan Weil
+ * Copyright (c) 2009, 2011, 2016, 2019 Stefan Weil
  *
  * This program is free software: you can redistribute it and/or modify
  * it under the terms of the GNU General Public License as published by
@@ -159,7 +159,8 @@ static uint64_t tci_uint64(uint32_t high, uint32_t low)
 /* Read constant (native size) from bytecode. */
 static tcg_target_ulong tci_read_i(uint8_t **tb_ptr)
 {
-    tcg_target_ulong value = *(tcg_target_ulong *)(*tb_ptr);
+    tcg_target_ulong value;
+    memcpy(&value, *tb_ptr, sizeof(value));
     *tb_ptr += sizeof(value);
     return value;
 }
@@ -167,7 +168,8 @@ static tcg_target_ulong tci_read_i(uint8_t **tb_ptr)
 /* Read unsigned constant (32 bit) from bytecode. */
 static uint32_t tci_read_i32(uint8_t **tb_ptr)
 {
-    uint32_t value = *(uint32_t *)(*tb_ptr);
+    uint32_t value;
+    memcpy(&value, *tb_ptr, sizeof(value));
     *tb_ptr += sizeof(value);
     return value;
 }
@@ -175,7 +177,8 @@ static uint32_t tci_read_i32(uint8_t **tb_ptr)
 /* Read signed constant (32 bit) from bytecode. */
 static int32_t tci_read_s32(uint8_t **tb_ptr)
 {
-    int32_t value = *(int32_t *)(*tb_ptr);
+    int32_t value;
+    memcpy(&value, *tb_ptr, sizeof(value));
     *tb_ptr += sizeof(value);
     return value;
 }
@@ -184,7 +187,8 @@ static int32_t tci_read_s32(uint8_t **tb_ptr)
 /* Read constant (64 bit) from bytecode. */
 static uint64_t tci_read_i64(uint8_t **tb_ptr)
 {
-    uint64_t value = *(uint64_t *)(*tb_ptr);
+    uint64_t value;
+    memcpy(&value, *tb_ptr, sizeof(value));
     *tb_ptr += sizeof(value);
     return value;
 }
@@ -474,7 +478,7 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
     tcg_target_ulong regs[TCG_TARGET_NB_REGS];
     long tcg_temps[CPU_TEMP_BUF_NLONGS];
     uintptr_t sp_value = (uintptr_t)(tcg_temps + CPU_TEMP_BUF_NLONGS);
-    uintptr_t ret = 0;
+    uint64_t ret = 0;
 
     regs[TCG_AREG0] = (tcg_target_ulong)env;
     regs[TCG_REG_CALL_STACK] = sp_value;
@@ -1094,7 +1098,7 @@ uintptr_t tcg_qemu_tb_exec(CPUArchState *env, uint8_t *tb_ptr)
             /* QEMU specific operations. */
 
         case INDEX_op_exit_tb:
-            ret = *(uint64_t *)tb_ptr;
+            memcpy(&ret, tb_ptr, sizeof(uint64_t));
             goto exit;
             break;
         case INDEX_op_goto_tb:
-- 
2.11.0

On 4/8/19 8:04 PM, Stefan Weil wrote:
>  static tcg_target_ulong tci_read_i(uint8_t **tb_ptr)
>  {
> -    tcg_target_ulong value = *(tcg_target_ulong *)(*tb_ptr);
> +    tcg_target_ulong value;

Ideally these would use the helpers from "qemu/bswap.h", ldl_he_p(), etc.


r~

On 09.04.19 08:58, Richard Henderson wrote:
> On 4/8/19 8:04 PM, Stefan Weil wrote:
>>  static tcg_target_ulong tci_read_i(uint8_t **tb_ptr)
>>  {
>> -    tcg_target_ulong value = *(tcg_target_ulong *)(*tb_ptr);
>> +    tcg_target_ulong value;
> 
> Ideally these would use the helpers from "qemu/bswap.h", ldl_he_p(), etc.
> 
> r~



That would require adding a helper for tcg_target_ulong to qemu/bswap.h.
Or tci.c would need conditional code for reading a tcg_target_ulong.

Those helpers in qemu/bswap.h are also a little bit strange:

- Why does lduw_he_p return an int instead of an uint16_t?
- Why does ldsw_he_p return an int instead of an int16_t?
- Why does ldl_he_p return an int instead of an int32_t?
- Should ldl_he_p be renamed into ldsl_he_p?
  And why is ldul_he_p missing?
- Should ldq_he_p be renamed into lduq_he_p?
  And why is ldsq_he_p missing?

Using the helpers might require nasty type casts to avoid compiler
warnings because of signed / unsigned and size mismatches.

Aren't the few memcpy statements in the TCI helpers much more direct and
understandable?

Regards
Stefan

On Tue, 9 Apr 2019 at 18:04, Stefan Weil <sw@weilnetz.de> wrote:
>
> On 09.04.19 08:58, Richard Henderson wrote:
> > On 4/8/19 8:04 PM, Stefan Weil wrote:
> >>  static tcg_target_ulong tci_read_i(uint8_t **tb_ptr)
> >>  {
> >> -    tcg_target_ulong value = *(tcg_target_ulong *)(*tb_ptr);
> >> +    tcg_target_ulong value;
> >
> > Ideally these would use the helpers from "qemu/bswap.h", ldl_he_p(), etc.

> That would require adding a helper for tcg_target_ulong to qemu/bswap.h.
> Or tci.c would need conditional code for reading a tcg_target_ulong.
>
> Those helpers in qemu/bswap.h are also a little bit strange:
>
> - Why does lduw_he_p return an int instead of an uint16_t?
> - Why does ldsw_he_p return an int instead of an int16_t?
> - Why does ldl_he_p return an int instead of an int32_t?

Some of these are for historical reasons (ie their API was
designed in a world where the callers were typically throwing
values around in "int"s rather than carefully using exact
right-sized types). If you want to assign the results to a
uint16_t/int16_t/int32_t this will work fine (and no cast is
required).

> - Should ldl_he_p be renamed into ldsl_he_p?
>   And why is ldul_he_p missing?

The assumption is that for 32-bit accesses there is no need
to care about the signedness of the load because the result
is going into a 32-bit variable anyway and so there is no
extension of any kind to be done.

> - Should ldq_he_p be renamed into lduq_he_p?
>   And why is ldsq_he_p missing?

Similarly here the value is being returned as a 64-bit
quantity, so there is no question of whether it should
be sign or zero extended as it isn't being extended at all.

> Using the helpers might require nasty type casts to avoid compiler
> warnings because of signed / unsigned and size mismatches.
>
> Aren't the few memcpy statements in the TCI helpers much more direct and
> understandable?

In general, no. "memcpy is a way to do an efficient possibly-unaligned
access" is an obscure bit of knowledge. We deliberately abstract this
away into these helper functions which provide APIs which are more
clearly "we need to perform a possibly-unaligned load of a specified
endianness". (It also means that if it turns out that we would
rather use __builtin_memcpy() rather than memcpy(), as we've just
discovered we need to, then there's only one file to change rather
than many.)

thanks
-- PMM