Re: [Qemu-devel] [PATCH 1/2] target-arm: Use sextract32() in branch decode

[Peter Maydell]

On 23 August 2013 19:09, Richard Henderson <address@hidden> wrote:
> On 08/23/2013 09:12 AM, Peter Maydell wrote:
>> -                offset = (((int32_t)insn << 8) >> 8);
>> -                val += (offset << 2) + 4;
>> +                offset = sextract32(insn << 2, 0, 26);
>> +                val += offset + 4;
>
> I read this incorrectly at first, considering the shift of insn, and
> I wonder if it's really the best way to write this because of that.
>
> What about just changing the one line to sextract(insn, 0, 24)?
>
> The second line by itself ought not trigger a warning from clang,
> because the << 2 never changes the sign bit.  If it still does,
> perhaps just multiply by 4 instead...

No, left shift of a negative value is undefined: "If E1 has a signed type
and nonnegative value, and E1 × 2E2 is representable in the result
type, then that is the resulting value; otherwise, the behavior is
undefined."

Also the ARM ARM pseudocode defines this operation as "first
append two zero bits and then sign extend" so I prefer it if we
actually implement it that way round.

> It's a stupid warning.  When was the last ones-compliment machine built?

The stupidity is that the C standard hasn't mandated 2s-complement.

-- PMM