[Qemu-devel] [RFC 0/3] tcg: Avoid undefined behaviour on unaligned stores

[Qemu-devel] [RFC 0/3] tcg: Avoid undefined behaviour on unaligned stores

[Peter Maydell]

These patches fix various cases in the x86 backend and the runtime
TB-jump-patching code where we cast an unaligned pointer to a uint32_t*
and store into it. Unaligned accesses are OK on x86 hardware, of
course, but this is still undefined behaviour in C, and the clang
sanitizer complains.

Sent out as an RFC to get a feel for whether we want to go
down some road like this. Personally I think it is worthwhile
for two reasons:
 (1) it's easier to see sanitizer warnings that actually matter if
they're not hidden in among a lot of warnings that don't.
 (2) it's increasingly clear that it's a bad idea to trust
compiler engineers, who will happily throw real applications
under the bus for a 0.05% improvement in Dhrystone scores;
so if we can reasonably avoid undefined behaviour we should.

It would probably be better to split the tcg_out functions into
separate ones for "I know this pointer is aligned" and "may be
unaligned", to avoid overhead on hosts which don't have cheap
unaligned stores. Also I haven't tried to extend the tcg_patch*
usage to the other backends.


Peter Maydell (3):
  exec-all.h: Use stl_p to avoid undefined behaviour patching x86 jumps
  tcg: Avoid stores to unaligned addresses
  tcg: Avoid undefined behaviour patching code at unaligned addresses

 include/exec/exec-all.h |  2 +-
 tcg/i386/tcg-target.c   | 12 ++++++------
 tcg/tcg.c               | 26 +++++++++++++++++++++++---
 3 files changed, 30 insertions(+), 10 deletions(-)

-- 
1.9.0

[Qemu-devel] [RFC 1/3] exec-all.h: Use stl_p to avoid undefined behaviour patching x86 jumps

[Peter Maydell]

The code which patches x86 jump instructions assumes it can do an
unaligned write of a uint32_t. This is actually safe on x86, but it's
still undefined behaviour. We have infrastructure for doing efficient
unaligned accesses which doesn't engage in undefined behaviour, so
use it.

This is technically fractionally less efficient, at least with gcc 4.6;
instead of one instruction:
 7b2:   89 3e                   mov    %edi,(%rsi)
we get an extra spurious store to the stack slot:
 7b2:   89 7c 24 64             mov    %edi,0x64(%rsp)
 7b6:   89 3e                   mov    %edi,(%rsi)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 include/exec/exec-all.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/include/exec/exec-all.h b/include/exec/exec-all.h
index f9ac332..1c49a21 100644
--- a/include/exec/exec-all.h
+++ b/include/exec/exec-all.h
@@ -229,7 +229,7 @@ void ppc_tb_set_jmp_target(unsigned long jmp_addr, unsigned long addr);
 static inline void tb_set_jmp_target1(uintptr_t jmp_addr, uintptr_t addr)
 {
     /* patch the branch destination */
-    *(uint32_t *)jmp_addr = addr - (jmp_addr + 4);
+    stl_p((void*)jmp_addr, addr - (jmp_addr + 4));
     /* no need to flush icache explicitly */
 }
 #elif defined(__aarch64__)
-- 
1.9.0

[Qemu-devel] [RFC 2/3] tcg: Avoid stores to unaligned addresses

[Peter Maydell]

Avoid stores to unaligned addresses in TCG code generation, by using the
usual memcpy() approach. (Using bswap.h would drag a lot of QEMU baggage
into TCG, so it's simpler just to do direct memcpy() here.)

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tcg/tcg.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index f1e0763..60f06c5 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -125,21 +125,21 @@ static inline void tcg_out8(TCGContext *s, uint8_t v)
 static inline void tcg_out16(TCGContext *s, uint16_t v)
 {
     uint8_t *p = s->code_ptr;
-    *(uint16_t *)p = v;
+    memcpy(p, &v, sizeof(v));
     s->code_ptr = p + 2;
 }
 
 static inline void tcg_out32(TCGContext *s, uint32_t v)
 {
     uint8_t *p = s->code_ptr;
-    *(uint32_t *)p = v;
+    memcpy(p, &v, sizeof(v));
     s->code_ptr = p + 4;
 }
 
 static inline void tcg_out64(TCGContext *s, uint64_t v)
 {
     uint8_t *p = s->code_ptr;
-    *(uint64_t *)p = v;
+    memcpy(p, &v, sizeof(v));
     s->code_ptr = p + 8;
 }
 
-- 
1.9.0

Re: [Qemu-devel] [RFC 2/3] tcg: Avoid stores to unaligned addresses

[Andreas Färber]

Am 28.03.2014 16:29, schrieb Peter Maydell:
> Avoid stores to unaligned addresses in TCG code generation, by using the
> usual memcpy() approach. (Using bswap.h would drag a lot of QEMU baggage
> into TCG, so it's simpler just to do direct memcpy() here.)
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

Reviewed-by: Andreas Färber <afaerber@suse.de>

Andreas

-- 
SUSE LINUX Products GmbH, Maxfeldstr. 5, 90409 Nürnberg, Germany
GF: Jeff Hawn, Jennifer Guild, Felix Imendörffer; HRB 16746 AG Nürnberg

[Qemu-devel] [RFC 3/3] tcg: Avoid undefined behaviour patching code at unaligned addresses

[Peter Maydell]

To avoid C undefined behaviour when patching generated code,
provide wrappers tcg_patch8/16/32/64 which use the usual memcpy
trick, and use them in the i386 backend.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 tcg/i386/tcg-target.c | 12 ++++++------
 tcg/tcg.c             | 20 ++++++++++++++++++++
 2 files changed, 26 insertions(+), 6 deletions(-)

diff --git a/tcg/i386/tcg-target.c b/tcg/i386/tcg-target.c
index f832282..a0f9dff 100644
--- a/tcg/i386/tcg-target.c
+++ b/tcg/i386/tcg-target.c
@@ -151,14 +151,14 @@ static void patch_reloc(uint8_t *code_ptr, int type,
         if (value != (int32_t)value) {
             tcg_abort();
         }
-        *(uint32_t *)code_ptr = value;
+        tcg_patch32(code_ptr, value);
         break;
     case R_386_PC8:
         value -= (uintptr_t)code_ptr;
         if (value != (int8_t)value) {
             tcg_abort();
         }
-        *(uint8_t *)code_ptr = value;
+        tcg_patch8(code_ptr, value);
         break;
     default:
         tcg_abort();
@@ -1276,9 +1276,9 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *l)
     uint8_t **label_ptr = &l->label_ptr[0];
 
     /* resolve label address */
-    *(uint32_t *)label_ptr[0] = (uint32_t)(s->code_ptr - label_ptr[0] - 4);
+    tcg_patch32(label_ptr[0], s->code_ptr - label_ptr[0] - 4);
     if (TARGET_LONG_BITS > TCG_TARGET_REG_BITS) {
-        *(uint32_t *)label_ptr[1] = (uint32_t)(s->code_ptr - label_ptr[1] - 4);
+        tcg_patch32(label_ptr[1], s->code_ptr - label_ptr[1] - 4);
     }
 
     if (TCG_TARGET_REG_BITS == 32) {
@@ -1360,9 +1360,9 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *l)
     TCGReg retaddr;
 
     /* resolve label address */
-    *(uint32_t *)label_ptr[0] = (uint32_t)(s->code_ptr - label_ptr[0] - 4);
+    tcg_patch32(label_ptr[0], s->code_ptr - label_ptr[0] - 4);
     if (TARGET_LONG_BITS > TCG_TARGET_REG_BITS) {
-        *(uint32_t *)label_ptr[1] = (uint32_t)(s->code_ptr - label_ptr[1] - 4);
+        tcg_patch32(label_ptr[1], s->code_ptr - label_ptr[1] - 4);
     }
 
     if (TCG_TARGET_REG_BITS == 32) {
diff --git a/tcg/tcg.c b/tcg/tcg.c
index 60f06c5..727112d 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -143,6 +143,26 @@ static inline void tcg_out64(TCGContext *s, uint64_t v)
     s->code_ptr = p + 8;
 }
 
+static inline void tcg_patch8(uint8_t *p, uint8_t v)
+{
+    memcpy(p, &v, sizeof(v));
+}
+
+static inline void tcg_patch16(uint8_t *p, uint16_t v)
+{
+    memcpy(p, &v, sizeof(v));
+}
+
+static inline void tcg_patch32(uint8_t *p, uint32_t v)
+{
+    memcpy(p, &v, sizeof(v));
+}
+
+static inline void tcg_patch64(uint8_t *p, uint64_t v)
+{
+    memcpy(p, &v, sizeof(v));
+}
+
 /* label relocation processing */
 
 static void tcg_out_reloc(TCGContext *s, uint8_t *code_ptr, int type,
-- 
1.9.0

Re: [Qemu-devel] [RFC 0/3] tcg: Avoid undefined behaviour on unaligned stores

[Richard Henderson]

On 03/28/2014 08:29 AM, Peter Maydell wrote:
> Peter Maydell (3):
>   exec-all.h: Use stl_p to avoid undefined behaviour patching x86 jumps
>   tcg: Avoid stores to unaligned addresses
>   tcg: Avoid undefined behaviour patching code at unaligned addresses

Reviewed-by: Richard Henderson <rth@twiddle.net>


r~

Re: [Qemu-devel] [RFC 0/3] tcg: Avoid undefined behaviour on unaligned stores

[Peter Maydell]

On 28 March 2014 18:06, Richard Henderson <rth@twiddle.net> wrote:
> On 03/28/2014 08:29 AM, Peter Maydell wrote:
>> Peter Maydell (3):
>>   exec-all.h: Use stl_p to avoid undefined behaviour patching x86 jumps
>>   tcg: Avoid stores to unaligned addresses
>>   tcg: Avoid undefined behaviour patching code at unaligned addresses
>
> Reviewed-by: Richard Henderson <rth@twiddle.net>

I was expecting more argument :-)

I'll do a respin with separate tcg_out*_unaligned and tcg_out*,
and update all the backends.

thanks
-- PMM

Re: [Qemu-devel] [RFC 0/3] tcg: Avoid undefined behaviour on unaligned stores

[Richard Henderson]

On 03/28/2014 11:18 AM, Peter Maydell wrote:
> On 28 March 2014 18:06, Richard Henderson <rth@twiddle.net> wrote:
>> On 03/28/2014 08:29 AM, Peter Maydell wrote:
>>> Peter Maydell (3):
>>>   exec-all.h: Use stl_p to avoid undefined behaviour patching x86 jumps
>>>   tcg: Avoid stores to unaligned addresses
>>>   tcg: Avoid undefined behaviour patching code at unaligned addresses
>>
>> Reviewed-by: Richard Henderson <rth@twiddle.net>
> 
> I was expecting more argument :-)
> 
> I'll do a respin with separate tcg_out*_unaligned and tcg_out*,
> and update all the backends.

Please don't.  I'd rather make different changes there, making s->code_ptr be
more appropriate to the elemental type of the native insn.

Since x86 wouldn't change its elemental type, I think your change is the best
of the possible solutions there.


r~