Re: [Qemu-devel] [PATCH v3 16/16] tcg/s390x: Return false on failure from patch_reloc

["Alex Bennée" <alex.bennee@linaro.org>]

Richard Henderson <richard.henderson@linaro.org> writes:

> This does require an extra two checks within the slow paths
> to replace the assert that we're moving.  Also add two checks
> within existing functions that lacked any kind of assert for
> out of range branch.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>

> ---
>  tcg/s390/tcg-target.inc.c | 34 +++++++++++++++++++++++-----------
>  1 file changed, 23 insertions(+), 11 deletions(-)
>
> diff --git a/tcg/s390/tcg-target.inc.c b/tcg/s390/tcg-target.inc.c
> index 68a4c60394..39ecf609a1 100644
> --- a/tcg/s390/tcg-target.inc.c
> +++ b/tcg/s390/tcg-target.inc.c
> @@ -377,23 +377,29 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>
>      switch (type) {
>      case R_390_PC16DBL:
> -        assert(pcrel2 == (int16_t)pcrel2);
> -        tcg_patch16(code_ptr, pcrel2);
> +        if (pcrel2 == (int16_t)pcrel2) {
> +            tcg_patch16(code_ptr, pcrel2);
> +            return true;
> +        }
>          break;
>      case R_390_PC32DBL:
> -        assert(pcrel2 == (int32_t)pcrel2);
> -        tcg_patch32(code_ptr, pcrel2);
> +        if (pcrel2 == (int32_t)pcrel2) {
> +            tcg_patch32(code_ptr, pcrel2);
> +            return true;
> +        }
>          break;
>      case R_390_20:
> -        assert(value == sextract64(value, 0, 20));
> -        old = *(uint32_t *)code_ptr & 0xf00000ff;
> -        old |= ((value & 0xfff) << 16) | ((value & 0xff000) >> 4);
> -        tcg_patch32(code_ptr, old);
> +        if (value == sextract64(value, 0, 20)) {
> +            old = *(uint32_t *)code_ptr & 0xf00000ff;
> +            old |= ((value & 0xfff) << 16) | ((value & 0xff000) >> 4);
> +            tcg_patch32(code_ptr, old);
> +            return true;
> +        }
>          break;
>      default:
>          g_assert_not_reached();
>      }
> -    return true;
> +    return false;
>  }
>
>  /* parse target specific constraints */
> @@ -1334,6 +1340,7 @@ static void tgen_compare_branch(TCGContext *s, S390Opcode opc, int cc,
>
>      if (l->has_value) {
>          off = l->u.value_ptr - s->code_ptr;
> +        tcg_debug_assert(off == (int16_t)off);
>      } else {
>          tcg_out_reloc(s, s->code_ptr + 1, R_390_PC16DBL, l, 2);
>      }
> @@ -1350,6 +1357,7 @@ static void tgen_compare_imm_branch(TCGContext *s, S390Opcode opc, int cc,
>
>      if (l->has_value) {
>          off = l->u.value_ptr - s->code_ptr;
> +        tcg_debug_assert(off == (int16_t)off);
>      } else {
>          tcg_out_reloc(s, s->code_ptr + 1, R_390_PC16DBL, l, 2);
>      }
> @@ -1615,7 +1623,9 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
>      TCGMemOpIdx oi = lb->oi;
>      TCGMemOp opc = get_memop(oi);
>
> -    patch_reloc(lb->label_ptr[0], R_390_PC16DBL, (intptr_t)s->code_ptr, 2);
> +    bool ok = patch_reloc(lb->label_ptr[0], R_390_PC16DBL,
> +                          (intptr_t)s->code_ptr, 2);
> +    tcg_debug_assert(ok);
>
>      tcg_out_mov(s, TCG_TYPE_PTR, TCG_REG_R2, TCG_AREG0);
>      if (TARGET_LONG_BITS == 64) {
> @@ -1636,7 +1646,9 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
>      TCGMemOpIdx oi = lb->oi;
>      TCGMemOp opc = get_memop(oi);
>
> -    patch_reloc(lb->label_ptr[0], R_390_PC16DBL, (intptr_t)s->code_ptr, 2);
> +    bool ok = patch_reloc(lb->label_ptr[0], R_390_PC16DBL,
> +                          (intptr_t)s->code_ptr, 2);
> +    tcg_debug_assert(ok);
>
>      tcg_out_mov(s, TCG_TYPE_PTR, TCG_REG_R2, TCG_AREG0);
>      if (TARGET_LONG_BITS == 64) {


--
Alex Bennée