| Message ID | 20190409060435.22801-1-sw@weilnetz.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | tci: Fix some unaligned memory accesses | expand |
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
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:
Signed-off-by: Stefan Weil <sw@weilnetz.de> --- tcg/tci.c | 18 +++++++++++------- 1 file changed, 11 insertions(+), 7 deletions(-)