[Qemu-devel] [PATCH 0/1] Fix unaligned reads in the tcg/tci.c

[Qemu-devel] [PATCH 0/1] Fix unaligned reads in the tcg/tci.c

[Anatoly Trosinenko]

The code in tcg/tci.c reads some data from TCI bytecode through
pointer dereferencing. As far as I know unaligned reads in such a way are
undefined behavior and compiling with -fsanitize=undefined enumerated
them as such at run-time.

I have replaced such reads with invocations of ld{l,q}_he_p.
A comment in include/qemu/bswap.h:310 suggests they should be properly
translated by the compiler. I didn't added signed/unsigned casts
since bswap.h does contain separate signed/unsigned versions
for 16-bit integers but does not for 32- and 64-bit ones, so I supposed
the developers of the bswap.h already arranged everything so
integer promotions don't mess things up. I can add casts in case I'm
not right about it.

Anatoly Trosinenko (1):
  tci: eliminate UB due to unaligned reads

 tcg/tci.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

-- 
2.14.1

[Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

Use ldl_he_p / ldq_he_p functions instead of a plain memory access
through pointer.

Signed-off-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
---
 tcg/tci.c | 16 +++++++++++-----
 1 file changed, 11 insertions(+), 5 deletions(-)

diff --git a/tcg/tci.c b/tcg/tci.c
index 33edca1903..410817cf6a 100644
--- a/tcg/tci.c
+++ b/tcg/tci.c
@@ -159,7 +159,13 @@ 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);
+#if TCG_TARGET_REG_BITS == 32
+    tcg_target_ulong value = ldl_he_p(*tb_ptr);
+#elif TCG_TARGET_REG_BITS == 64
+    tcg_target_ulong value = ldq_he_p(*tb_ptr);
+#else
+#error unsupported
+#endif
     *tb_ptr += sizeof(value);
     return value;
 }
@@ -167,7 +173,7 @@ 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 = ldl_he_p(*tb_ptr);
     *tb_ptr += sizeof(value);
     return value;
 }
@@ -175,7 +181,7 @@ 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 = ldl_he_p(*tb_ptr);
     *tb_ptr += sizeof(value);
     return value;
 }
@@ -184,7 +190,7 @@ 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 = ldq_he_p(*tb_ptr);
     *tb_ptr += sizeof(value);
     return value;
 }
@@ -1094,7 +1100,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;
+            ret = ldq_he_p(tb_ptr);
             goto exit;
             break;
         case INDEX_op_goto_tb:
-- 
2.14.1

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Stefan Weil]

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

Am 27.01.2018 um 14:49 schrieb Anatoly Trosinenko:
> Use ldl_he_p / ldq_he_p functions instead of a plain memory access
> through pointer.
> 
> Signed-off-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
> ---
>  tcg/tci.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)

A better alternative might be aligning the relevant data when generating
the bytecode. See also my comment on alignment in tcg/tci/README.

Stefan


[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

My patch is kind of trivial quick fix that just eliminates these unaligned
reads and doesn't seem to require complicated testing supposing my code
properly handles integer promotion (and hope it will not slow the
interpreter down).

Aligning everything, on the other hand, can not only remove the UB but also
speed things up, but if I get it right, requires O(opcode count) manual
work and subsequent less trivial testing that every opcode's argument
layout match on generation and interpretation side (errors should be
significantly localized due to present assertion on operation size,
though). So my patch may be considered as temporary solution.

In fact, I had to similarly make these unaligned reads explicit when
porting QEMU to JavaScript because Emscripten hugely relies on absence of
some kinds of UB such as "implicit" unaligned accesses, and such fix seemed
to resolve this issue for me on "host with special alignment requirement".


2018-01-27 19:38 GMT+03:00 Stefan Weil <sw@weilnetz.de>:

> Am 27.01.2018 um 14:49 schrieb Anatoly Trosinenko:
> > Use ldl_he_p / ldq_he_p functions instead of a plain memory access
> > through pointer.
> >
> > Signed-off-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
> > ---
> >  tcg/tci.c | 16 +++++++++++-----
> >  1 file changed, 11 insertions(+), 5 deletions(-)
>
> A better alternative might be aligning the relevant data when generating
> the bytecode. See also my comment on alignment in tcg/tci/README.
>
> Stefan
>
>


-- 
С уважением,
Анатолий Тросиненко
e-mail: anatoly.trosinenko@gmail.com

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

Ping.
Patchwork link: http://patchwork.ozlabs.org/patch/866732/
Patchew link: http://patchew.org/QEMU/20180127134908.24095-1-
anatoly.trosinenko@gmail.com/

Original cover letter:
The code in tcg/tci.c reads some data from TCI bytecode through
pointer dereferencing. As far as I know unaligned reads in such a way are
undefined behavior and compiling with -fsanitize=undefined enumerated
them as such at run-time.

I have replaced such reads with invocations of ld{l,q}_he_p.
A comment in include/qemu/bswap.h:310 suggests they should be properly
translated by the compiler. I didn't added signed/unsigned casts
since bswap.h does contain separate signed/unsigned versions
for 16-bit integers but does not for 32- and 64-bit ones, so I supposed
the developers of the bswap.h already arranged everything so
integer promotions don't mess things up. I can add casts in case I'm
not right about it.
http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg06516.html


2018-01-28 9:42 GMT+03:00 Anatoly Trosinenko <anatoly.trosinenko@gmail.com>:

> My patch is kind of trivial quick fix that just eliminates these unaligned
> reads and doesn't seem to require complicated testing supposing my code
> properly handles integer promotion (and hope it will not slow the
> interpreter down).
>
> Aligning everything, on the other hand, can not only remove the UB but
> also speed things up, but if I get it right, requires O(opcode count)
> manual work and subsequent less trivial testing that every opcode's
> argument layout match on generation and interpretation side (errors should
> be significantly localized due to present assertion on operation size,
> though). So my patch may be considered as temporary solution.
>
> In fact, I had to similarly make these unaligned reads explicit when
> porting QEMU to JavaScript because Emscripten hugely relies on absence of
> some kinds of UB such as "implicit" unaligned accesses, and such fix seemed
> to resolve this issue for me on "host with special alignment requirement".
>
>
> 2018-01-27 19:38 GMT+03:00 Stefan Weil <sw@weilnetz.de>:
>
>> Am 27.01.2018 um 14:49 schrieb Anatoly Trosinenko:
>> > Use ldl_he_p / ldq_he_p functions instead of a plain memory access
>> > through pointer.
>> >
>> > Signed-off-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
>> > ---
>> >  tcg/tci.c | 16 +++++++++++-----
>> >  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> A better alternative might be aligning the relevant data when generating
>> the bytecode. See also my comment on alignment in tcg/tci/README.
>>
>> Stefan
>>
>>

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

Ping.
Patchwork link: http://patchwork.ozlabs.org/patch/866732/
Patchew link: http://patchew.org/QEMU/20180127134908.24095-1-anatoly.
trosinenko@gmail.com/

The code in tcg/tci.c reads some data from TCI bytecode through
pointer dereferencing. As far as I know unaligned reads in such a way are
undefined behavior and compiling with -fsanitize=undefined enumerated
them as such at run-time.

I have replaced such reads with invocations of ld{l,q}_he_p.
A comment in include/qemu/bswap.h:310 suggests they should be properly
translated by the compiler. I didn't added signed/unsigned casts
since bswap.h does contain separate signed/unsigned versions
for 16-bit integers but does not for 32- and 64-bit ones, so I supposed
the developers of the bswap.h already arranged everything so
integer promotions don't mess things up. I can add casts in case I'm
not right about it.
http://lists.nongnu.org/archive/html/qemu-devel/2018-01/msg06516.html

2018-01-28 9:42 GMT+03:00 Anatoly Trosinenko <anatoly.trosinenko@gmail.com>:

> My patch is kind of trivial quick fix that just eliminates these unaligned
> reads and doesn't seem to require complicated testing supposing my code
> properly handles integer promotion (and hope it will not slow the
> interpreter down).
>
> Aligning everything, on the other hand, can not only remove the UB but
> also speed things up, but if I get it right, requires O(opcode count)
> manual work and subsequent less trivial testing that every opcode's
> argument layout match on generation and interpretation side (errors should
> be significantly localized due to present assertion on operation size,
> though). So my patch may be considered as temporary solution.
>
> In fact, I had to similarly make these unaligned reads explicit when
> porting QEMU to JavaScript because Emscripten hugely relies on absence of
> some kinds of UB such as "implicit" unaligned accesses, and such fix seemed
> to resolve this issue for me on "host with special alignment requirement".
>
>
> 2018-01-27 19:38 GMT+03:00 Stefan Weil <sw@weilnetz.de>:
>
>> Am 27.01.2018 um 14:49 schrieb Anatoly Trosinenko:
>> > Use ldl_he_p / ldq_he_p functions instead of a plain memory access
>> > through pointer.
>> >
>> > Signed-off-by: Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
>> > ---
>> >  tcg/tci.c | 16 +++++++++++-----
>> >  1 file changed, 11 insertions(+), 5 deletions(-)
>>
>> A better alternative might be aligning the relevant data when generating
>> the bytecode. See also my comment on alignment in tcg/tci/README.
>>
>> Stefan
>>
>>
>
>
> --
> С уважением,
> Анатолий Тросиненко
> e-mail: anatoly.trosinenko@gmail.com
>



-- 
С уважением,
Анатолий Тросиненко
e-mail: anatoly.trosinenko@gmail.com

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Richard Henderson]

On 03/03/2018 12:54 AM, Anatoly Trosinenko wrote:
> Ping.
> Patchwork link: http://patchwork.ozlabs.org/patch/866732/
> <http://patchwork.ozlabs.org/patch/866732/>
> Patchew link:
> http://patchew.org/QEMU/20180127134908.24095-1-anatoly.trosinenko@gmail.com/
> <http://patchew.org/QEMU/20180127134908.24095-1-anatoly.trosinenko@gmail.com/>
> 
> The code in tcg/tci.c reads some data from TCI bytecode through
> pointer dereferencing. As far as I know unaligned reads in such a way are
> undefined behavior and compiling with -fsanitize=undefined enumerated
> them as such at run-time.

This is exactly one of the reasons why I have urged for TCI to be abandoned.

While your patch works, it is *enormously* inefficient for hosts that require it.


r~

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

Can rewriting TCI in such a way that every operation is aligned at 4- or
even 8-byte boundary fix the situation or are there some more serious
problems?

2018-03-03 16:57 GMT+03:00 Richard Henderson <rth@twiddle.net>:

> On 03/03/2018 12:54 AM, Anatoly Trosinenko wrote:
> > Ping.
> > Patchwork link: http://patchwork.ozlabs.org/patch/866732/
> > <http://patchwork.ozlabs.org/patch/866732/>
> > Patchew link:
> > http://patchew.org/QEMU/20180127134908.24095-1-
> anatoly.trosinenko@gmail.com/
> > <http://patchew.org/QEMU/20180127134908.24095-1-
> anatoly.trosinenko@gmail.com/>
> >
> > The code in tcg/tci.c reads some data from TCI bytecode through
> > pointer dereferencing. As far as I know unaligned reads in such a way are
> > undefined behavior and compiling with -fsanitize=undefined enumerated
> > them as such at run-time.
>
> This is exactly one of the reasons why I have urged for TCI to be
> abandoned.
>
> While your patch works, it is *enormously* inefficient for hosts that
> require it.
>
>
> r~
>



-- 
С уважением,
Анатолий Тросиненко
e-mail: anatoly.trosinenko@gmail.com

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Richard Henderson]

On 03/03/2018 06:07 AM, Anatoly Trosinenko wrote:
> Can rewriting TCI in such a way that every operation is aligned at 4- or even
> 8-byte boundary fix the situation or are there some more serious problems?

With the current TCI, there are also problems with calls to helper functions.
The only portable way to do this is to use a library such as libffi.

I once rewrote TCI completely in order to address both problems, but that only
brought questions as to why TCI is useful at all.

So.  Why do you want to use TCI instead of a native TCG backend?


r~

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

> So.  Why do you want to use TCI instead of a native TCG backend?

Frankly speaking, personally I just have a strange experiment on porting
QEMU to JavaScript. :) I used the TCI bytecode as some intermediate
patchable form for rarely executing BBs and for (re)generating asm.js from
it when required. I used a Python script to generate wrappers with exactly
10 arguments around helper functions. In fact, it may be worth for me to
create WebAssembly TCG backend and interpret **that** bytecode if required.

TCI may still be useful for someone else running QEMU on unsupported host,
though.

2018-03-03 17:13 GMT+03:00 Richard Henderson <rth@twiddle.net>:

> On 03/03/2018 06:07 AM, Anatoly Trosinenko wrote:
> > Can rewriting TCI in such a way that every operation is aligned at 4- or
> even
> > 8-byte boundary fix the situation or are there some more serious
> problems?
>
> With the current TCI, there are also problems with calls to helper
> functions.
> The only portable way to do this is to use a library such as libffi.
>
> I once rewrote TCI completely in order to address both problems, but that
> only
> brought questions as to why TCI is useful at all.
>
> So.  Why do you want to use TCI instead of a native TCG backend?
>
>
> r~
>

--
Best regards,
Anatoly

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Stefan Weil]

Am 03.03.2018 um 15:07 schrieb Anatoly Trosinenko:
> Can rewriting TCI in such a way that every operation is aligned at 4- or
> even 8-byte boundary fix the situation or are there some more serious
> problems?

That's my preferred solution. Are there cases which would require 8-byte
alignment?

Richard, the discussion about non-portable calls to helper functions is
not new, but I still have no test case which fails. Do you think of a
special case (architecture, helper function)?

Stefan

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Anatoly Trosinenko]

2018-03-03 18:41 GMT+03:00 Stefan Weil <sw@weilnetz.de>:

> Am 03.03.2018 um 15:07 schrieb Anatoly Trosinenko:
> > Can rewriting TCI in such a way that every operation is aligned at 4- or
> > even 8-byte boundary fix the situation or are there some more serious
> > problems?
>
> That's my preferred solution. Are there cases which would require 8-byte
> alignment?
>

And what if create some function like

uint8_t *align_and_increment(uint8_t **ptr, int pow2) {
  size_t size = 1 << pow2;
  uint8_t *result = (uint8_t*)((((uintptr)*ptr) + size - 1) & ~(size - 1));
  *ptr = result + size;
  return result;
}

and rewrite get / put functions like this:

static uint32_t tci_read_i32(uint8_t **tb_ptr)
{
    uint32_t value = *(uint32_t *)align_and_increment(tb_ptr, 2);
    return value;
}

On one hand, it involves some slightly obscure pointer calculations
(just in one place), on the other hand, no modifications will probably
be required for TCI TCG backend or interpreter loop code (they can
still be useful for **optimizations** of bytecode size, but it should
just work as is).

--
Best regards,
Anatoly

Re: [Qemu-devel] [PATCH 1/1] tci: eliminate UB due to unaligned reads

[Richard Henderson]

On 03/03/2018 05:41 PM, Stefan Weil wrote:
> Richard, the discussion about non-portable calls to helper functions is
> not new, but I still have no test case which fails. Do you think of a
> special case (architecture, helper function)?

The two that come to mind immediately are:

i386 with a compiler defaulting to stdcall (which I admit is unlikely; it also
would not work for TCG without modification).

m68k (pointers passed in address registers; integrals passed in data registers).


r~

Re: [Qemu-devel] [PATCH 0/1] Fix unaligned reads in the tcg/tci.c

[Anatoly Trosinenko]

Ping.
Patchwork link: http://patchwork.ozlabs.org/patch/866732/
Patchew link:
http://patchew.org/QEMU/20180127134908.24095-1-anatoly.trosinenko@gmail.com/

(Initially forgot to add Richard Henderson to CC.)

2018-01-27 16:49 GMT+03:00 Anatoly Trosinenko <anatoly.trosinenko@gmail.com>
:

> The code in tcg/tci.c reads some data from TCI bytecode through
> pointer dereferencing. As far as I know unaligned reads in such a way are
> undefined behavior and compiling with -fsanitize=undefined enumerated
> them as such at run-time.
>
> I have replaced such reads with invocations of ld{l,q}_he_p.
> A comment in include/qemu/bswap.h:310 suggests they should be properly
> translated by the compiler. I didn't added signed/unsigned casts
> since bswap.h does contain separate signed/unsigned versions
> for 16-bit integers but does not for 32- and 64-bit ones, so I supposed
> the developers of the bswap.h already arranged everything so
> integer promotions don't mess things up. I can add casts in case I'm
> not right about it.
>
> Anatoly Trosinenko (1):
>   tci: eliminate UB due to unaligned reads
>
>  tcg/tci.c | 16 +++++++++++-----
>  1 file changed, 11 insertions(+), 5 deletions(-)
>
> --
> 2.14.1
>
>


-- 
Best regards,
Anatoly Trosinenko