Re: [PATCH v4 34/43] tcg/riscv: Fix branch range checks

[Alistair Francis]

On Mon, Dec 14, 2020 at 6:18 AM Richard Henderson
<richard.henderson@linaro.org> wrote:
>
> The offset even checks were folded into the range check incorrectly.
> By offsetting by 1, and not decrementing the width, we silently
> allowed out of range branches.
>
> Assert that the offset is always even instead.  Move tcg_out_goto
> down into the CONFIG_SOFTMMU block so that it is not unused.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

Reviewed-by: Alistair Francis <alistair.francis@wdc.com>

Alistair

> ---
>  tcg/riscv/tcg-target.c.inc | 28 +++++++++++++++-------------
>  1 file changed, 15 insertions(+), 13 deletions(-)
>
> diff --git a/tcg/riscv/tcg-target.c.inc b/tcg/riscv/tcg-target.c.inc
> index 025e3cd0bb..195c3eff03 100644
> --- a/tcg/riscv/tcg-target.c.inc
> +++ b/tcg/riscv/tcg-target.c.inc
> @@ -429,7 +429,8 @@ static bool reloc_sbimm12(tcg_insn_unit *code_ptr, 
> tcg_insn_unit *target)
>  {
>      intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
>
> -    if (offset == sextreg(offset, 1, 12) << 1) {
> +    tcg_debug_assert((offset & 1) == 0);
> +    if (offset == sextreg(offset, 0, 12)) {
>          code_ptr[0] |= encode_sbimm12(offset);
>          return true;
>      }
> @@ -441,7 +442,8 @@ static bool reloc_jimm20(tcg_insn_unit *code_ptr, 
> tcg_insn_unit *target)
>  {
>      intptr_t offset = (intptr_t)target - (intptr_t)code_ptr;
>
> -    if (offset == sextreg(offset, 1, 20) << 1) {
> +    tcg_debug_assert((offset & 1) == 0);
> +    if (offset == sextreg(offset, 0, 20)) {
>          code_ptr[0] |= encode_ujimm20(offset);
>          return true;
>      }
> @@ -854,28 +856,21 @@ static void tcg_out_setcond2(TCGContext *s, TCGCond 
> cond, TCGReg ret,
>      g_assert_not_reached();
>  }
>
> -static inline void tcg_out_goto(TCGContext *s, tcg_insn_unit *target)
> -{
> -    ptrdiff_t offset = tcg_pcrel_diff(s, target);
> -    tcg_debug_assert(offset == sextreg(offset, 1, 20) << 1);
> -    tcg_out_opc_jump(s, OPC_JAL, TCG_REG_ZERO, offset);
> -}
> -
>  static void tcg_out_call_int(TCGContext *s, const tcg_insn_unit *arg, bool 
> tail)
>  {
>      TCGReg link = tail ? TCG_REG_ZERO : TCG_REG_RA;
>      ptrdiff_t offset = tcg_pcrel_diff(s, arg);
>      int ret;
>
> -    if (offset == sextreg(offset, 1, 20) << 1) {
> +    tcg_debug_assert((offset & 1) == 0);
> +    if (offset == sextreg(offset, 0, 20)) {
>          /* short jump: -2097150 to 2097152 */
>          tcg_out_opc_jump(s, OPC_JAL, link, offset);
> -    } else if (TCG_TARGET_REG_BITS == 32 ||
> -        offset == sextreg(offset, 1, 31) << 1) {
> +    } else if (TCG_TARGET_REG_BITS == 32 || offset == (int32_t)offset) {
>          /* long jump: -2147483646 to 2147483648 */
>          tcg_out_opc_upper(s, OPC_AUIPC, TCG_REG_TMP0, 0);
>          tcg_out_opc_imm(s, OPC_JALR, link, TCG_REG_TMP0, 0);
> -        ret = reloc_call(s->code_ptr - 2, arg);\
> +        ret = reloc_call(s->code_ptr - 2, arg);
>          tcg_debug_assert(ret == true);
>      } else if (TCG_TARGET_REG_BITS == 64) {
>          /* far jump: 64-bit */
> @@ -962,6 +957,13 @@ QEMU_BUILD_BUG_ON(TCG_TARGET_REG_BITS < 
> TARGET_LONG_BITS);
>  QEMU_BUILD_BUG_ON(TLB_MASK_TABLE_OFS(0) > 0);
>  QEMU_BUILD_BUG_ON(TLB_MASK_TABLE_OFS(0) < -(1 << 11));
>
> +static void tcg_out_goto(TCGContext *s, tcg_insn_unit *target)
> +{
> +    tcg_out_opc_jump(s, OPC_JAL, TCG_REG_ZERO, 0);
> +    bool ok = reloc_jimm20(s->code_ptr - 1, target);
> +    tcg_debug_assert(ok);
> +}
> +
>  static void tcg_out_tlb_load(TCGContext *s, TCGReg addrl,
>                               TCGReg addrh, TCGMemOpIdx oi,
>                               tcg_insn_unit **label_ptr, bool is_load)
> --
> 2.25.1
>
>