| Message ID | 1377274359-8707-2-git-send-email-peter.maydell@linaro.org |
|---|---|
| State | Superseded |
| Headers | show |
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... It's a stupid warning. When was the last ones-compliment machine built? r~
On 23 August 2013 19:09, Richard Henderson <rth@twiddle.net> 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
diff --git a/target-arm/translate.c b/target-arm/translate.c index d1e8538..ebf5d4f 100644 --- a/target-arm/translate.c +++ b/target-arm/translate.c @@ -28,6 +28,7 @@ #include "disas/disas.h" #include "tcg-op.h" #include "qemu/log.h" +#include "qemu/bitops.h" #include "helper.h" #define GEN_HELPER 1 @@ -7956,8 +7957,8 @@ static void disas_arm_insn(CPUARMState * env, DisasContext *s) tcg_gen_movi_i32(tmp, val); store_reg(s, 14, tmp); } - offset = (((int32_t)insn << 8) >> 8); - val += (offset << 2) + 4; + offset = sextract32(insn << 2, 0, 26); + val += offset + 4; gen_jmp(s, val); } break;
In the decode of ARM B and BL insns, swap the order of the "append 2 implicit zeros to imm24" and the sign extend, and use the new sextract32() utility function to do the latter. This avoids a direct dependency on the undefined C behaviour of shifting into the sign bit of an integer. Signed-off-by: Peter Maydell <peter.maydell@linaro.org> --- target-arm/translate.c | 5 +++-- 1 file changed, 3 insertions(+), 2 deletions(-)