[Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Peter Maydell]

This patchset fixes a collection of warnings emitted by the clang
undefined behaviour sanitizer in the course of booting an AArch64
Linux guest to a shell prompt. These are all various kinds of bad
shift (shifting into the sign bit, left shifting negative values,
shifting by more than the size of the data type).

There remains one set of warnings which I'm not sure how to
approach. We have an enum for the valid syndrome register EC
field values:

enum arm_exception_class {
    EC_UNCATEGORIZED          = 0x00,
    [...]
    EC_VECTORCATCH            = 0x3a,
    EC_AA64_BKPT              = 0x3c,
};

The EC field is a 6 bit field at the top of the 32 bit syndrome
register, so we define

#define ARM_EL_EC_SHIFT 26

and have utility functions that assemble syndrome values like:

static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
{
    return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
}

Unfortunately this is UB, because C says that enums can be
signed types, and if the enum is signed then "EC_AA64_BKPT << 26"
is shifting into the sign bit of a signed type.

I can't see any nice way of avoiding this. Possible nasty ways:

(1) Drop the enum and just use old-fashioned
#define EC_AA64_BKPT 0x3cU
since then we can force them to be unsigned
(2) Cast the constant to unsigned at point of use
(3) Keep the enum and add defines wrapping actual use
#define EC_AA64_BKPT ((unsigned)EC_AA64_BKPT)
I guess there's also
(4) Ignore the problem and trust that the compiler developers will
never decide to use this bit of undefined behaviour.

My preference is (1) I suppose (we don't actually use the
enum for anything except to define the values, so if it's
not providing helpful definitions we should drop it).

Opinions?

Peter Maydell (4):
  target-arm: A64: Fix shifts into sign bit
  target-arm: A64: Fix handling of rotate in logic_imm_decode_wmask
  target-arm: A64: Avoid left shifting negative integers in
    disas_pc_rel_addr
  target-arm: A64: Avoid signed shifts in disas_ldst_pair()

 target-arm/translate-a64.c | 18 +++++++++++-------
 1 file changed, 11 insertions(+), 7 deletions(-)

-- 
1.9.1

[Qemu-devel] [PATCH 1/4] target-arm: A64: Fix shifts into sign bit

[Peter Maydell]

Fix attempts to shift into the sign bit of an int, which is undefined
behaviour in C and warned about by the clang sanitizer.

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

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index acf4b16..d3801c5 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1077,7 +1077,7 @@ static void disas_uncond_b_imm(DisasContext *s, uint32_t insn)
 {
     uint64_t addr = s->pc + sextract32(insn, 0, 26) * 4 - 4;
 
-    if (insn & (1 << 31)) {
+    if (insn & (1U << 31)) {
         /* C5.6.26 BL Branch with link */
         tcg_gen_movi_i64(cpu_reg(s, 30), s->pc);
     }
@@ -1271,7 +1271,7 @@ static void gen_get_nzcv(TCGv_i64 tcg_rt)
     TCGv_i32 nzcv = tcg_temp_new_i32();
 
     /* build bit 31, N */
-    tcg_gen_andi_i32(nzcv, cpu_NF, (1 << 31));
+    tcg_gen_andi_i32(nzcv, cpu_NF, (1U << 31));
     /* build bit 30, Z */
     tcg_gen_setcondi_i32(TCG_COND_EQ, tmp, cpu_ZF, 0);
     tcg_gen_deposit_i32(nzcv, nzcv, tmp, 30, 1);
@@ -1296,7 +1296,7 @@ static void gen_set_nzcv(TCGv_i64 tcg_rt)
     tcg_gen_trunc_i64_i32(nzcv, tcg_rt);
 
     /* bit 31, N */
-    tcg_gen_andi_i32(cpu_NF, nzcv, (1 << 31));
+    tcg_gen_andi_i32(cpu_NF, nzcv, (1U << 31));
     /* bit 30, Z */
     tcg_gen_andi_i32(cpu_ZF, nzcv, (1 << 30));
     tcg_gen_setcondi_i32(TCG_COND_EQ, cpu_ZF, cpu_ZF, 0);
-- 
1.9.1

[Qemu-devel] [PATCH 2/4] target-arm: A64: Fix handling of rotate in logic_imm_decode_wmask

[Peter Maydell]

The code in logic_imm_decode_wmask attempts to rotate a mask
value within the bottom 'e' bits of the value with
    mask = (mask >> r) | (mask << (e - r));
This has two issues:
 * if the element size is 64 then a rotate by zero results
   in a shift left by 64, which is undefined behaviour
 * if the element size is smaller than 64 then this will
   leave junk in the value at bit 'e' and above, which is
   not valid input to bitfield_replicate(). As it happens,
   the bits at bit 'e' to '2e - r' are exactly the ones
   which bitfield_replicate is going to copy in there,
   so this isn't a "wrong code generated" bug, but it's
   confusing and if we ever put an assert in
   bitfield_replicate it would fire on valid guest code.

Fix the former by not doing anything if r is zero, and
the latter by masking with bitmask64(e).

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate-a64.c | 5 ++++-
 1 file changed, 4 insertions(+), 1 deletion(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index d3801c5..94b3bf4 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -2820,7 +2820,10 @@ static bool logic_imm_decode_wmask(uint64_t *result, unsigned int immn,
      * by r within the element (which is e bits wide)...
      */
     mask = bitmask64(s + 1);
-    mask = (mask >> r) | (mask << (e - r));
+    if (r) {
+        mask = (mask >> r) | (mask << (e - r));
+        mask &= bitmask64(e);
+    }
     /* ...then replicate the element over the whole 64 bit value */
     mask = bitfield_replicate(mask, e);
     *result = mask;
-- 
1.9.1

[Qemu-devel] [PATCH 3/4] target-arm: A64: Avoid left shifting negative integers in disas_pc_rel_addr

[Peter Maydell]

Shifting a negative integer left is undefined behaviour in C.
Avoid it by assembling and shifting the offset fields as
unsigned values and then sign extending as the final action.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate-a64.c | 5 +++--
 1 file changed, 3 insertions(+), 2 deletions(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 94b3bf4..68c5b23 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -2662,11 +2662,12 @@ static void disas_pc_rel_adr(DisasContext *s, uint32_t insn)
 {
     unsigned int page, rd;
     uint64_t base;
-    int64_t offset;
+    uint64_t offset;
 
     page = extract32(insn, 31, 1);
     /* SignExtend(immhi:immlo) -> offset */
-    offset = ((int64_t)sextract32(insn, 5, 19) << 2) | extract32(insn, 29, 2);
+    offset = sextract64(insn, 5, 19);
+    offset = offset << 2 | extract32(insn, 29, 2);
     rd = extract32(insn, 0, 5);
     base = s->pc - 4;
 
-- 
1.9.1

[Qemu-devel] [PATCH 4/4] target-arm: A64: Avoid signed shifts in disas_ldst_pair()

[Peter Maydell]

Avoid shifting potentially negative signed offset values in
disas_ldst_pair() by keeping the offset in a uint64_t rather
than an int64_t.

Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
---
 target-arm/translate-a64.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
index 68c5b23..9f54501 100644
--- a/target-arm/translate-a64.c
+++ b/target-arm/translate-a64.c
@@ -1917,7 +1917,7 @@ static void disas_ldst_pair(DisasContext *s, uint32_t insn)
     int rt = extract32(insn, 0, 5);
     int rn = extract32(insn, 5, 5);
     int rt2 = extract32(insn, 10, 5);
-    int64_t offset = sextract32(insn, 15, 7);
+    uint64_t offset = sextract64(insn, 15, 7);
     int index = extract32(insn, 23, 2);
     bool is_vector = extract32(insn, 26, 1);
     bool is_load = extract32(insn, 22, 1);
-- 
1.9.1

Re: [Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Greg Bellows]

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

On Fri, Feb 6, 2015 at 8:34 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> This patchset fixes a collection of warnings emitted by the clang
> undefined behaviour sanitizer in the course of booting an AArch64
> Linux guest to a shell prompt. These are all various kinds of bad
> shift (shifting into the sign bit, left shifting negative values,
> shifting by more than the size of the data type).
>
> There remains one set of warnings which I'm not sure how to
> approach. We have an enum for the valid syndrome register EC
> field values:
>
> enum arm_exception_class {
>     EC_UNCATEGORIZED          = 0x00,
>     [...]
>     EC_VECTORCATCH            = 0x3a,
>     EC_AA64_BKPT              = 0x3c,
> };
>
> The EC field is a 6 bit field at the top of the 32 bit syndrome
> register, so we define
>
> #define ARM_EL_EC_SHIFT 26
>
> and have utility functions that assemble syndrome values like:
>
> static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
> {
>     return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 &
> 0xffff);
> }
>
> Unfortunately this is UB, because C says that enums can be
> signed types, and if the enum is signed then "EC_AA64_BKPT << 26"
> is shifting into the sign bit of a signed type.
>
> I can't see any nice way of avoiding this. Possible nasty ways:
>
> (1) Drop the enum and just use old-fashioned
> #define EC_AA64_BKPT 0x3cU
> since then we can force them to be unsigned
> (2) Cast the constant to unsigned at point of use
> (3) Keep the enum and add defines wrapping actual use
> #define EC_AA64_BKPT ((unsigned)EC_AA64_BKPT)
> I guess there's also
> (4) Ignore the problem and trust that the compiler developers will
> never decide to use this bit of undefined behaviour.
>
> My preference is (1) I suppose (we don't actually use the
> enum for anything except to define the values, so if it's
> not providing helpful definitions we should drop it).
>
> Opinions?
>

​Similar to the #define approach, but possibly with the benefits of the
enum.  What if you declare each enum separately as const unsigned int?
​


>
> Peter Maydell (4):
>   target-arm: A64: Fix shifts into sign bit
>   target-arm: A64: Fix handling of rotate in logic_imm_decode_wmask
>   target-arm: A64: Avoid left shifting negative integers in
>     disas_pc_rel_addr
>   target-arm: A64: Avoid signed shifts in disas_ldst_pair()
>
>  target-arm/translate-a64.c | 18 +++++++++++-------
>  1 file changed, 11 insertions(+), 7 deletions(-)
>
> --
> 1.9.1
>
>
>

[-- Attachment #2: Type: text/html, Size: 3560 bytes --]

Re: [Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Richard Henderson]

On 02/06/2015 07:57 AM, Greg Bellows wrote:
> Similar to the #define approach, but possibly with the benefits of the enum. 
> What if you declare each enum separately as const unsigned int?

This is not c++.  That won't do what you want.


r~

Re: [Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Richard Henderson]

On 02/06/2015 06:34 AM, Peter Maydell wrote:
> I can't see any nice way of avoiding this. Possible nasty ways:
> 
> (1) Drop the enum and just use old-fashioned
> #define EC_AA64_BKPT 0x3cU
> since then we can force them to be unsigned
> (2) Cast the constant to unsigned at point of use
> (3) Keep the enum and add defines wrapping actual use
> #define EC_AA64_BKPT ((unsigned)EC_AA64_BKPT)
> I guess there's also
> (4) Ignore the problem and trust that the compiler developers will
> never decide to use this bit of undefined behaviour.

It should be enough to simply add the unsigned suffix to the integers as they
are, forcing the underlying type to be unsigned.

At least that's suggested by the simple test case

   extern void link_error(void);
   enum X { bot = 1u } x = bot;
   int main() { if (-x < 0) link_error(); return 0; }

$ clang -O z.c
z.c:3:21: warning: comparison of unsigned expression < 0 is always false
      [-Wtautological-compare]
int main() { if (-x < 0) link_error(); return 0; }
                 ~~ ^ ~
$ gcc -O -Wall z.c
$


r~

Re: [Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Peter Maydell]

On 6 February 2015 at 16:20, Richard Henderson <rth@twiddle.net> wrote:
> It should be enough to simply add the unsigned suffix to the integers as they
> are, forcing the underlying type to be unsigned.

I can't see anything in the C99 spec that justifies this
as a fix... In fact, 6.7.2.2 para 3 says
"The identifiers in an enumerator list are declared as
constants that have type int", which sounds to me like it
means "if you have enum { foo = ..., }; then 'foo' must
have (signed) integer type even if the representation
of the enum type is unsigned".

And indeed:
e104462:trusty:qemu$ cat /tmp/zz9.c
enum { foo = 1U };

int main(void)
{
    return foo << 31;
}
e104462:trusty:qemu$ clang -fsanitize=undefined -Wall /tmp/zz9.c -o /tmp/zz9
e104462:trusty:qemu$ /tmp/zz9
/tmp/zz9.c:5:16: runtime error: left shift of 1 by 31 places cannot be
represented in type 'int'

-- PMM

Re: [Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Richard Henderson]

On 02/06/2015 08:43 AM, Peter Maydell wrote:
> On 6 February 2015 at 16:20, Richard Henderson <rth@twiddle.net> wrote:
>> It should be enough to simply add the unsigned suffix to the integers as they
>> are, forcing the underlying type to be unsigned.
> 
> I can't see anything in the C99 spec that justifies this
> as a fix... In fact, 6.7.2.2 para 3 says
> "The identifiers in an enumerator list are declared as
> constants that have type int", which sounds to me like it
> means "if you have enum { foo = ..., }; then 'foo' must
> have (signed) integer type even if the representation
> of the enum type is unsigned".

Hum.  True, despite the talk in para 4 about the compatible type of the
enumeration.  Which is why my test worked with a variable of enum type, but
doesn't work with one of the enum members.

Oh well.


r~

Re: [Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Eric Blake]

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

On 02/06/2015 07:34 AM, Peter Maydell wrote:
> This patchset fixes a collection of warnings emitted by the clang
> undefined behaviour sanitizer in the course of booting an AArch64
> Linux guest to a shell prompt. These are all various kinds of bad
> shift (shifting into the sign bit, left shifting negative values,
> shifting by more than the size of the data type).
> 
> There remains one set of warnings which I'm not sure how to
> approach. We have an enum for the valid syndrome register EC
> field values:
> 
> enum arm_exception_class {
>     EC_UNCATEGORIZED          = 0x00,
>     [...]
>     EC_VECTORCATCH            = 0x3a,
>     EC_AA64_BKPT              = 0x3c,
> };
> 
> The EC field is a 6 bit field at the top of the 32 bit syndrome
> register, so we define
> 
> #define ARM_EL_EC_SHIFT 26
> 
> and have utility functions that assemble syndrome values like:
> 
> static inline uint32_t syn_aa64_bkpt(uint32_t imm16)
> {
>     return (EC_AA64_BKPT << ARM_EL_EC_SHIFT) | ARM_EL_IL | (imm16 & 0xffff);
> }
> 
> Unfortunately this is UB, because C says that enums can be
> signed types, and if the enum is signed then "EC_AA64_BKPT << 26"
> is shifting into the sign bit of a signed type.
> 
> I can't see any nice way of avoiding this. Possible nasty ways:
> 
> (1) Drop the enum and just use old-fashioned
> #define EC_AA64_BKPT 0x3cU
> since then we can force them to be unsigned
> (2) Cast the constant to unsigned at point of use
> (3) Keep the enum and add defines wrapping actual use
> #define EC_AA64_BKPT ((unsigned)EC_AA64_BKPT)

There's also:

(5) change the usage of the shifting macro:
#define ARM_EL_EC_SHIFT(x) (((x)+0U) << 26)
...
    return ARM_EL_EC_SHIFT(EC_AA64_BKPT) | ARM_EL_IL | (imm16 & 0xffff);

> I guess there's also
> (4) Ignore the problem and trust that the compiler developers will
> never decide to use this bit of undefined behaviour.

HACKING already implies we assume sane 2's complement behavior of shifts
(maybe it's worth another line for this particular case of shifting into
the signed bit of a signed result, and figuring out how to shut up clang):

>> The C language specification defines regions of undefined behavior and
>> implementation defined behavior (to give compiler authors enough leeway to
>> produce better code).  In general, code in QEMU should follow the language
>> specification and avoid both undefined and implementation defined
>> constructs. ("It works fine on the gcc I tested it with" is not a valid
>> argument...) However there are a few areas where we allow ourselves to
>> assume certain behaviors because in practice all the platforms we care about
>> behave in the same way and writing strictly conformant code would be
>> painful. These are:
>>  * you may assume that integers are 2s complement representation
>>  * you may assume that right shift of a signed integer duplicates
>>    the sign bit (ie it is an arithmetic shift, not a logical shift)


-- 
Eric Blake   eblake redhat com    +1-919-301-3266
Libvirt virtualization library http://libvirt.org


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

Re: [Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Peter Maydell]

On 6 February 2015 at 17:37, Eric Blake <eblake@redhat.com> wrote:
> On 02/06/2015 07:34 AM, Peter Maydell wrote:
> HACKING already implies we assume sane 2's complement behavior of shifts
> (maybe it's worth another line for this particular case of shifting into
> the signed bit of a signed result, and figuring out how to shut up clang):
>
>>> The C language specification defines regions of undefined behavior and
>>> implementation defined behavior (to give compiler authors enough leeway to
>>> produce better code).  In general, code in QEMU should follow the language
>>> specification and avoid both undefined and implementation defined
>>> constructs. ("It works fine on the gcc I tested it with" is not a valid
>>> argument...) However there are a few areas where we allow ourselves to
>>> assume certain behaviors because in practice all the platforms we care about
>>> behave in the same way and writing strictly conformant code would be
>>> painful. These are:
>>>  * you may assume that integers are 2s complement representation
>>>  * you may assume that right shift of a signed integer duplicates
>>>    the sign bit (ie it is an arithmetic shift, not a logical shift)

The difference is that these two are implementation-defined
behaviour, whereas shifting into the sign bit is undefined
behaviour.

I would much rather we had a well-defined and supported "friendly C"
dialect which actually did the things programmers expect C to do:
http://blog.regehr.org/archives/1180

In the absence of that we pretty much have to assume adversarial
optimization on the part of the compiler, because 0.5% improvements
in SPEC benchmark scores justify breaking previously working code...

-- PMM

Re: [Qemu-devel] [PATCH 0/4] target-arm: fix various clang UB sanitizer warnings

[Markus Armbruster]

Peter Maydell <peter.maydell@linaro.org> writes:

> On 6 February 2015 at 17:37, Eric Blake <eblake@redhat.com> wrote:
>> On 02/06/2015 07:34 AM, Peter Maydell wrote:
>> HACKING already implies we assume sane 2's complement behavior of shifts
>> (maybe it's worth another line for this particular case of shifting into
>> the signed bit of a signed result, and figuring out how to shut up clang):
>>
>>>> The C language specification defines regions of undefined behavior and
>>>> implementation defined behavior (to give compiler authors enough leeway to
>>>> produce better code).  In general, code in QEMU should follow the language
>>>> specification and avoid both undefined and implementation defined
>>>> constructs. ("It works fine on the gcc I tested it with" is not a valid
>>>> argument...) However there are a few areas where we allow ourselves to
>>>> assume certain behaviors because in practice all the platforms we care about
>>>> behave in the same way and writing strictly conformant code would be
>>>> painful. These are:
>>>>  * you may assume that integers are 2s complement representation
>>>>  * you may assume that right shift of a signed integer duplicates
>>>>    the sign bit (ie it is an arithmetic shift, not a logical shift)
>
> The difference is that these two are implementation-defined
> behaviour, whereas shifting into the sign bit is undefined
> behaviour.
>
> I would much rather we had a well-defined and supported "friendly C"
> dialect which actually did the things programmers expect C to do:
> http://blog.regehr.org/archives/1180
>
> In the absence of that we pretty much have to assume adversarial
> optimization on the part of the compiler, because 0.5% improvements
> in SPEC benchmark scores justify breaking previously working code...

Incredibly bad tradeoff.  But what's really, truly inexcusable is
compilers smart enough to "optimize" working code into faster,
non-working code, yet not smart enough to tell us when they use their
undefined behavior license.  clang at least provides tools to give us a
fighting chance.

That said, I'm lukewarm about complicating working, straightforward code
to comply with the fine print of the standard unless there's evidence of
optimizers breaking it, without workable ways to suppress the
problematic "optimizations".  For what it's worth, the kernel uses
-fno-strict-aliasing -fno-struct-overflow.

Your option (1) "Drop the enum and just use old-fashioned #define
EC_AA64_BKPT 0x3cU" isn't too bad, though.

If Eric's option (5) "change the usage of the shifting macro" fully
solves the problem, I'd prefer it.

I've always wanted to be able to say things like "unsigned long enum".

Re: [Qemu-devel] [PATCH 1/4] target-arm: A64: Fix shifts into sign bit

[Greg Bellows]

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

On Fri, Feb 6, 2015 at 8:34 AM, Peter Maydell <peter.maydell@linaro.org>
wrote:

> Fix attempts to shift into the sign bit of an int, which is undefined
> behaviour in C and warned about by the clang sanitizer.
>
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>
> ---
>  target-arm/translate-a64.c | 6 +++---
>  1 file changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/target-arm/translate-a64.c b/target-arm/translate-a64.c
> index acf4b16..d3801c5 100644
> --- a/target-arm/translate-a64.c
> +++ b/target-arm/translate-a64.c
> @@ -1077,7 +1077,7 @@ static void disas_uncond_b_imm(DisasContext *s,
> uint32_t insn)
>  {
>      uint64_t addr = s->pc + sextract32(insn, 0, 26) * 4 - 4;
>
> -    if (insn & (1 << 31)) {
> +    if (insn & (1U << 31)) {
>          /* C5.6.26 BL Branch with link */
>          tcg_gen_movi_i64(cpu_reg(s, 30), s->pc);
>      }
> @@ -1271,7 +1271,7 @@ static void gen_get_nzcv(TCGv_i64 tcg_rt)
>      TCGv_i32 nzcv = tcg_temp_new_i32();
>
>      /* build bit 31, N */
> -    tcg_gen_andi_i32(nzcv, cpu_NF, (1 << 31));
> +    tcg_gen_andi_i32(nzcv, cpu_NF, (1U << 31));
>      /* build bit 30, Z */
>      tcg_gen_setcondi_i32(TCG_COND_EQ, tmp, cpu_ZF, 0);
>      tcg_gen_deposit_i32(nzcv, nzcv, tmp, 30, 1);
> @@ -1296,7 +1296,7 @@ static void gen_set_nzcv(TCGv_i64 tcg_rt)
>      tcg_gen_trunc_i64_i32(nzcv, tcg_rt);
>
>      /* bit 31, N */
> -    tcg_gen_andi_i32(cpu_NF, nzcv, (1 << 31));
> +    tcg_gen_andi_i32(cpu_NF, nzcv, (1U << 31));
>      /* bit 30, Z */
>      tcg_gen_andi_i32(cpu_ZF, nzcv, (1 << 30));
>      tcg_gen_setcondi_i32(TCG_COND_EQ, cpu_ZF, cpu_ZF, 0);
> --
> 1.9.1
>
>
>
​Reviewed-by: Greg Bellows <greg.bellows@linaro.org>​

[-- Attachment #2: Type: text/html, Size: 2608 bytes --]