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

[Alex Bennée]

Richard Henderson <address@hidden> writes:

> This does require an extra two checks within the slow paths
> to replace the assert that we're moving.
>
> Signed-off-by: Richard Henderson <address@hidden>
> ---
>  tcg/aarch64/tcg-target.inc.c | 35 ++++++++++++++++++++---------------
>  1 file changed, 20 insertions(+), 15 deletions(-)
>
> diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> index 16f08c59c4..77f0ca4d5e 100644
> --- a/tcg/aarch64/tcg-target.inc.c
> +++ b/tcg/aarch64/tcg-target.inc.c
> @@ -78,20 +78,26 @@ static const int tcg_target_call_oarg_regs[1] = {
>  #define TCG_REG_GUEST_BASE TCG_REG_X28
>  #endif
>
> -static inline void reloc_pc26(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
> +static inline bool reloc_pc26(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
>  {
>      ptrdiff_t offset = target - code_ptr;
> -    tcg_debug_assert(offset == sextract64(offset, 0, 26));
> -    /* read instruction, mask away previous PC_REL26 parameter contents,
> -       set the proper offset, then write back the instruction. */
> -    *code_ptr = deposit32(*code_ptr, 0, 26, offset);
> +    if (offset == sextract64(offset, 0, 26)) {
> +        /* read instruction, mask away previous PC_REL26 parameter contents,
> +           set the proper offset, then write back the instruction. */
> +        *code_ptr = deposit32(*code_ptr, 0, 26, offset);
> +        return true;
> +    }
> +    return false;
>  }
>
> -static inline void reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
> +static inline bool reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
>  {
>      ptrdiff_t offset = target - code_ptr;
> -    tcg_debug_assert(offset == sextract64(offset, 0, 19));
> -    *code_ptr = deposit32(*code_ptr, 5, 19, offset);
> +    if (offset == sextract64(offset, 0, 19)) {
> +        *code_ptr = deposit32(*code_ptr, 5, 19, offset);
> +        return true;
> +    }
> +    return false;
>  }
>
>  static inline bool patch_reloc(tcg_insn_unit *code_ptr, int type,
> @@ -101,15 +107,12 @@ static inline bool patch_reloc(tcg_insn_unit *code_ptr, 
> int type,
>      switch (type) {
>      case R_AARCH64_JUMP26:
>      case R_AARCH64_CALL26:
> -        reloc_pc26(code_ptr, (tcg_insn_unit *)value);
> -        break;
> +        return reloc_pc26(code_ptr, (tcg_insn_unit *)value);
>      case R_AARCH64_CONDBR19:
> -        reloc_pc19(code_ptr, (tcg_insn_unit *)value);
> -        break;
> +        return reloc_pc19(code_ptr, (tcg_insn_unit *)value);
>      default:
>          tcg_abort();
>      }
> -    return true;

nit: the default leg could return false for the same effect

Otherwise:

Reviewed-by: Alex Bennée <address@hidden>


--
Alex Bennée