[Qemu-devel] [PATCH v3 00/16] tcg: Assorted cleanups

[Qemu-devel] [PATCH v3 00/16] tcg: Assorted cleanups

[Richard Henderson]

In review of v2, Alex asked for patch 3 to be broken apart.
Here, patches 3-16 are that breakup.  I've omitted the rest
of the v2 patchset for now; I have yet to make substantive
changes to those.


r~


Richard Henderson (16):
  tcg/i386: Always use %ebp for TCG_AREG0
  tcg/i386: Move TCG_REG_CALL_STACK from define to enum
  tcg/aarch64: Remove reloc_pc26_atomic
  tcg/aarch64: Fold away "noaddr" branch routines
  tcg/arm: Remove reloc_pc24_atomic
  tcg/arm: Fold away "noaddr" branch routines
  tcg/ppc: Fold away "noaddr" branch routines
  tcg/s390: Remove retranslation code
  tcg/sparc: Remove retranslation code
  tcg/mips: Remove retranslation code
  tcg: Return success from patch_reloc
  tcg/i386: Return false on failure from patch_reloc
  tcg/aarch64: Return false on failure from patch_reloc
  tcg/arm: Return false on failure from patch_reloc
  tcg/ppc: Return false on failure from patch_reloc
  tcg/s390x: Return false on failure from patch_reloc

 tcg/i386/tcg-target.h        | 10 ++----
 tcg/aarch64/tcg-target.inc.c | 69 ++++++++++++------------------------
 tcg/arm/tcg-target.inc.c     | 55 +++++++++++-----------------
 tcg/i386/tcg-target.inc.c    |  7 ++--
 tcg/mips/tcg-target.inc.c    | 10 ++----
 tcg/ppc/tcg-target.inc.c     | 60 +++++++++++++++----------------
 tcg/s390/tcg-target.inc.c    | 45 +++++++++++++----------
 tcg/sparc/tcg-target.inc.c   | 13 +++----
 tcg/tcg.c                    |  8 +++--
 tcg/tci/tcg-target.inc.c     |  3 +-
 10 files changed, 122 insertions(+), 158 deletions(-)

-- 
2.17.2

[Qemu-devel] [PATCH v3 01/16] tcg/i386: Always use %ebp for TCG_AREG0

[Richard Henderson]

For x86_64, this can remove a REX prefix resulting in smaller code
when manipulating globals of type i32, as we move them between backing
store via cpu_env, aka TCG_AREG0.

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/i386/tcg-target.h | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
index 9fdf37f23c..7488c3d869 100644
--- a/tcg/i386/tcg-target.h
+++ b/tcg/i386/tcg-target.h
@@ -84,6 +84,8 @@ typedef enum {
     TCG_REG_RBP = TCG_REG_EBP,
     TCG_REG_RSI = TCG_REG_ESI,
     TCG_REG_RDI = TCG_REG_EDI,
+
+    TCG_AREG0 = TCG_REG_EBP,
 } TCGReg;
 
 /* used for function call generation */
@@ -194,12 +196,6 @@ extern bool have_avx2;
 #define TCG_TARGET_extract_i64_valid(ofs, len) \
     (((ofs) == 8 && (len) == 8) || ((ofs) + (len)) == 32)
 
-#if TCG_TARGET_REG_BITS == 64
-# define TCG_AREG0 TCG_REG_R14
-#else
-# define TCG_AREG0 TCG_REG_EBP
-#endif
-
 static inline void flush_icache_range(uintptr_t start, uintptr_t stop)
 {
 }
-- 
2.17.2

[Qemu-devel] [PATCH v3 02/16] tcg/i386: Move TCG_REG_CALL_STACK from define to enum

[Richard Henderson]

Reviewed-by: Alex Bennée <alex.bennee@linaro.org>
Reviewed-by: Emilio G. Cota <cota@braap.org>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/i386/tcg-target.h | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/tcg/i386/tcg-target.h b/tcg/i386/tcg-target.h
index 7488c3d869..2441658865 100644
--- a/tcg/i386/tcg-target.h
+++ b/tcg/i386/tcg-target.h
@@ -86,10 +86,10 @@ typedef enum {
     TCG_REG_RDI = TCG_REG_EDI,
 
     TCG_AREG0 = TCG_REG_EBP,
+    TCG_REG_CALL_STACK = TCG_REG_ESP
 } TCGReg;
 
 /* used for function call generation */
-#define TCG_REG_CALL_STACK TCG_REG_ESP 
 #define TCG_TARGET_STACK_ALIGN 16
 #if defined(_WIN64)
 #define TCG_TARGET_CALL_STACK_OFFSET 32
-- 
2.17.2

[Qemu-devel] [PATCH v3 03/16] tcg/aarch64: Remove reloc_pc26_atomic

[Richard Henderson]

It is unused since b68686bd4bfeb70040b4099df993dfa0b4f37b03.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/aarch64/tcg-target.inc.c | 12 ------------
 1 file changed, 12 deletions(-)

diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
index 083592a4d7..a41b633960 100644
--- a/tcg/aarch64/tcg-target.inc.c
+++ b/tcg/aarch64/tcg-target.inc.c
@@ -87,18 +87,6 @@ static inline void reloc_pc26(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
     *code_ptr = deposit32(*code_ptr, 0, 26, offset);
 }
 
-static inline void reloc_pc26_atomic(tcg_insn_unit *code_ptr,
-                                     tcg_insn_unit *target)
-{
-    ptrdiff_t offset = target - code_ptr;
-    tcg_insn_unit insn;
-    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. */
-    insn = atomic_read(code_ptr);
-    atomic_set(code_ptr, deposit32(insn, 0, 26, offset));
-}
-
 static inline void reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
 {
     ptrdiff_t offset = target - code_ptr;
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 03/16] tcg/aarch64: Remove reloc_pc26_atomic

[Alex Bennée]

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

> It is unused since b68686bd4bfeb70040b4099df993dfa0b4f37b03.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/aarch64/tcg-target.inc.c | 12 ------------
>  1 file changed, 12 deletions(-)
>
> diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> index 083592a4d7..a41b633960 100644
> --- a/tcg/aarch64/tcg-target.inc.c
> +++ b/tcg/aarch64/tcg-target.inc.c
> @@ -87,18 +87,6 @@ static inline void reloc_pc26(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
>      *code_ptr = deposit32(*code_ptr, 0, 26, offset);
>  }
>
> -static inline void reloc_pc26_atomic(tcg_insn_unit *code_ptr,
> -                                     tcg_insn_unit *target)
> -{
> -    ptrdiff_t offset = target - code_ptr;
> -    tcg_insn_unit insn;
> -    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. */
> -    insn = atomic_read(code_ptr);
> -    atomic_set(code_ptr, deposit32(insn, 0, 26, offset));
> -}
> -
>  static inline void reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
>  {
>      ptrdiff_t offset = target - code_ptr;


--
Alex Bennée

[Qemu-devel] [PATCH v3 04/16] tcg/aarch64: Fold away "noaddr" branch routines

[Richard Henderson]

There are one use apiece for these.  There is no longer a need for
preserving branch offset operands, as we no longer re-translate.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/aarch64/tcg-target.inc.c | 21 ++-------------------
 1 file changed, 2 insertions(+), 19 deletions(-)

diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
index a41b633960..28de0226fb 100644
--- a/tcg/aarch64/tcg-target.inc.c
+++ b/tcg/aarch64/tcg-target.inc.c
@@ -1129,23 +1129,6 @@ static inline void tcg_out_goto_long(TCGContext *s, tcg_insn_unit *target)
     }
 }
 
-static inline void tcg_out_goto_noaddr(TCGContext *s)
-{
-    /* We pay attention here to not modify the branch target by reading from
-       the buffer. This ensure that caches and memory are kept coherent during
-       retranslation.  Mask away possible garbage in the high bits for the
-       first translation, while keeping the offset bits for retranslation. */
-    uint32_t old = tcg_in32(s);
-    tcg_out_insn(s, 3206, B, old);
-}
-
-static inline void tcg_out_goto_cond_noaddr(TCGContext *s, TCGCond c)
-{
-    /* See comments in tcg_out_goto_noaddr.  */
-    uint32_t old = tcg_in32(s) >> 5;
-    tcg_out_insn(s, 3202, B_C, c, old);
-}
-
 static inline void tcg_out_callr(TCGContext *s, TCGReg reg)
 {
     tcg_out_insn(s, 3207, BLR, reg);
@@ -1192,7 +1175,7 @@ static inline void tcg_out_goto_label(TCGContext *s, TCGLabel *l)
 {
     if (!l->has_value) {
         tcg_out_reloc(s, s->code_ptr, R_AARCH64_JUMP26, l, 0);
-        tcg_out_goto_noaddr(s);
+        tcg_out_insn(s, 3206, B, 0);
     } else {
         tcg_out_goto(s, l->u.value_ptr);
     }
@@ -1523,7 +1506,7 @@ static void tcg_out_tlb_read(TCGContext *s, TCGReg addr_reg, TCGMemOp opc,
 
     /* If not equal, we jump to the slow path. */
     *label_ptr = s->code_ptr;
-    tcg_out_goto_cond_noaddr(s, TCG_COND_NE);
+    tcg_out_insn(s, 3202, B_C, TCG_COND_NE, 0);
 }
 
 #endif /* CONFIG_SOFTMMU */
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 04/16] tcg/aarch64: Fold away "noaddr" branch routines

[Alex Bennée]

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

> There are one use apiece for these.  There is no longer a need for
> preserving branch offset operands, as we no longer re-translate.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/aarch64/tcg-target.inc.c | 21 ++-------------------
>  1 file changed, 2 insertions(+), 19 deletions(-)
>
> diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> index a41b633960..28de0226fb 100644
> --- a/tcg/aarch64/tcg-target.inc.c
> +++ b/tcg/aarch64/tcg-target.inc.c
> @@ -1129,23 +1129,6 @@ static inline void tcg_out_goto_long(TCGContext *s, tcg_insn_unit *target)
>      }
>  }
>
> -static inline void tcg_out_goto_noaddr(TCGContext *s)
> -{
> -    /* We pay attention here to not modify the branch target by reading from
> -       the buffer. This ensure that caches and memory are kept coherent during
> -       retranslation.  Mask away possible garbage in the high bits for the
> -       first translation, while keeping the offset bits for retranslation. */
> -    uint32_t old = tcg_in32(s);
> -    tcg_out_insn(s, 3206, B, old);
> -}
> -
> -static inline void tcg_out_goto_cond_noaddr(TCGContext *s, TCGCond c)
> -{
> -    /* See comments in tcg_out_goto_noaddr.  */
> -    uint32_t old = tcg_in32(s) >> 5;
> -    tcg_out_insn(s, 3202, B_C, c, old);
> -}
> -
>  static inline void tcg_out_callr(TCGContext *s, TCGReg reg)
>  {
>      tcg_out_insn(s, 3207, BLR, reg);
> @@ -1192,7 +1175,7 @@ static inline void tcg_out_goto_label(TCGContext *s, TCGLabel *l)
>  {
>      if (!l->has_value) {
>          tcg_out_reloc(s, s->code_ptr, R_AARCH64_JUMP26, l, 0);
> -        tcg_out_goto_noaddr(s);
> +        tcg_out_insn(s, 3206, B, 0);
>      } else {
>          tcg_out_goto(s, l->u.value_ptr);
>      }
> @@ -1523,7 +1506,7 @@ static void tcg_out_tlb_read(TCGContext *s, TCGReg addr_reg, TCGMemOp opc,
>
>      /* If not equal, we jump to the slow path. */
>      *label_ptr = s->code_ptr;
> -    tcg_out_goto_cond_noaddr(s, TCG_COND_NE);
> +    tcg_out_insn(s, 3202, B_C, TCG_COND_NE, 0);
>  }
>
>  #endif /* CONFIG_SOFTMMU */


--
Alex Bennée

[Qemu-devel] [PATCH v3 05/16] tcg/arm: Remove reloc_pc24_atomic

[Richard Henderson]

It is unused since 3fb53fb4d12f2e7833bd1659e6013237b130ef20.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/arm/tcg-target.inc.c | 8 --------
 1 file changed, 8 deletions(-)

diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
index e1fbf465cb..1142eb13ad 100644
--- a/tcg/arm/tcg-target.inc.c
+++ b/tcg/arm/tcg-target.inc.c
@@ -193,14 +193,6 @@ static inline void reloc_pc24(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
     *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff);
 }
 
-static inline void reloc_pc24_atomic(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
-{
-    ptrdiff_t offset = (tcg_ptr_byte_diff(target, code_ptr) - 8) >> 2;
-    tcg_insn_unit insn = atomic_read(code_ptr);
-    tcg_debug_assert(offset == sextract32(offset, 0, 24));
-    atomic_set(code_ptr, deposit32(insn, 0, 24, offset));
-}
-
 static void patch_reloc(tcg_insn_unit *code_ptr, int type,
                         intptr_t value, intptr_t addend)
 {
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 05/16] tcg/arm: Remove reloc_pc24_atomic

[Alex Bennée]

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

> It is unused since 3fb53fb4d12f2e7833bd1659e6013237b130ef20.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/arm/tcg-target.inc.c | 8 --------
>  1 file changed, 8 deletions(-)
>
> diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
> index e1fbf465cb..1142eb13ad 100644
> --- a/tcg/arm/tcg-target.inc.c
> +++ b/tcg/arm/tcg-target.inc.c
> @@ -193,14 +193,6 @@ static inline void reloc_pc24(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
>      *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff);
>  }
>
> -static inline void reloc_pc24_atomic(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
> -{
> -    ptrdiff_t offset = (tcg_ptr_byte_diff(target, code_ptr) - 8) >> 2;
> -    tcg_insn_unit insn = atomic_read(code_ptr);
> -    tcg_debug_assert(offset == sextract32(offset, 0, 24));
> -    atomic_set(code_ptr, deposit32(insn, 0, 24, offset));
> -}
> -
>  static void patch_reloc(tcg_insn_unit *code_ptr, int type,
>                          intptr_t value, intptr_t addend)
>  {


--
Alex Bennée

[Qemu-devel] [PATCH v3 06/16] tcg/arm: Fold away "noaddr" branch routines

[Richard Henderson]

There are one use apiece for these.  There is no longer a need for
preserving branch offset operands, as we no longer re-translate.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/arm/tcg-target.inc.c | 22 +++-------------------
 1 file changed, 3 insertions(+), 19 deletions(-)

diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
index 1142eb13ad..1651f00281 100644
--- a/tcg/arm/tcg-target.inc.c
+++ b/tcg/arm/tcg-target.inc.c
@@ -366,22 +366,6 @@ static inline void tcg_out_b(TCGContext *s, int cond, int32_t offset)
                     (((offset - 8) >> 2) & 0x00ffffff));
 }
 
-static inline void tcg_out_b_noaddr(TCGContext *s, int cond)
-{
-    /* We pay attention here to not modify the branch target by masking
-       the corresponding bytes.  This ensure that caches and memory are
-       kept coherent during retranslation. */
-    tcg_out32(s, deposit32(*s->code_ptr, 24, 8, (cond << 4) | 0x0a));
-}
-
-static inline void tcg_out_bl_noaddr(TCGContext *s, int cond)
-{
-    /* We pay attention here to not modify the branch target by masking
-       the corresponding bytes.  This ensure that caches and memory are
-       kept coherent during retranslation. */
-    tcg_out32(s, deposit32(*s->code_ptr, 24, 8, (cond << 4) | 0x0b));
-}
-
 static inline void tcg_out_bl(TCGContext *s, int cond, int32_t offset)
 {
     tcg_out32(s, (cond << 28) | 0x0b000000 |
@@ -1082,7 +1066,7 @@ static inline void tcg_out_goto_label(TCGContext *s, int cond, TCGLabel *l)
         tcg_out_goto(s, cond, l->u.value_ptr);
     } else {
         tcg_out_reloc(s, s->code_ptr, R_ARM_PC24, l, 0);
-        tcg_out_b_noaddr(s, cond);
+        tcg_out_b(s, cond, 0);
     }
 }
 
@@ -1628,7 +1612,7 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is64)
     /* This a conditional BL only to load a pointer within this opcode into LR
        for the slow path.  We will not be using the value for a tail call.  */
     label_ptr = s->code_ptr;
-    tcg_out_bl_noaddr(s, COND_NE);
+    tcg_out_bl(s, COND_NE, 0);
 
     tcg_out_qemu_ld_index(s, opc, datalo, datahi, addrlo, addend);
 
@@ -1760,7 +1744,7 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is64)
 
     /* The conditional call must come last, as we're going to return here.  */
     label_ptr = s->code_ptr;
-    tcg_out_bl_noaddr(s, COND_NE);
+    tcg_out_bl(s, COND_NE, 0);
 
     add_qemu_ldst_label(s, false, oi, datalo, datahi, addrlo, addrhi,
                         s->code_ptr, label_ptr);
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 06/16] tcg/arm: Fold away "noaddr" branch routines

[Alex Bennée]

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

> There are one use apiece for these.  There is no longer a need for
> preserving branch offset operands, as we no longer re-translate.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/arm/tcg-target.inc.c | 22 +++-------------------
>  1 file changed, 3 insertions(+), 19 deletions(-)
>
> diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
> index 1142eb13ad..1651f00281 100644
> --- a/tcg/arm/tcg-target.inc.c
> +++ b/tcg/arm/tcg-target.inc.c
> @@ -366,22 +366,6 @@ static inline void tcg_out_b(TCGContext *s, int cond, int32_t offset)
>                      (((offset - 8) >> 2) & 0x00ffffff));
>  }
>
> -static inline void tcg_out_b_noaddr(TCGContext *s, int cond)
> -{
> -    /* We pay attention here to not modify the branch target by masking
> -       the corresponding bytes.  This ensure that caches and memory are
> -       kept coherent during retranslation. */
> -    tcg_out32(s, deposit32(*s->code_ptr, 24, 8, (cond << 4) | 0x0a));
> -}
> -
> -static inline void tcg_out_bl_noaddr(TCGContext *s, int cond)
> -{
> -    /* We pay attention here to not modify the branch target by masking
> -       the corresponding bytes.  This ensure that caches and memory are
> -       kept coherent during retranslation. */
> -    tcg_out32(s, deposit32(*s->code_ptr, 24, 8, (cond << 4) | 0x0b));
> -}
> -
>  static inline void tcg_out_bl(TCGContext *s, int cond, int32_t offset)
>  {
>      tcg_out32(s, (cond << 28) | 0x0b000000 |
> @@ -1082,7 +1066,7 @@ static inline void tcg_out_goto_label(TCGContext *s, int cond, TCGLabel *l)
>          tcg_out_goto(s, cond, l->u.value_ptr);
>      } else {
>          tcg_out_reloc(s, s->code_ptr, R_ARM_PC24, l, 0);
> -        tcg_out_b_noaddr(s, cond);
> +        tcg_out_b(s, cond, 0);
>      }
>  }
>
> @@ -1628,7 +1612,7 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is64)
>      /* This a conditional BL only to load a pointer within this opcode into LR
>         for the slow path.  We will not be using the value for a tail call.  */
>      label_ptr = s->code_ptr;
> -    tcg_out_bl_noaddr(s, COND_NE);
> +    tcg_out_bl(s, COND_NE, 0);
>
>      tcg_out_qemu_ld_index(s, opc, datalo, datahi, addrlo, addend);
>
> @@ -1760,7 +1744,7 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is64)
>
>      /* The conditional call must come last, as we're going to return here.  */
>      label_ptr = s->code_ptr;
> -    tcg_out_bl_noaddr(s, COND_NE);
> +    tcg_out_bl(s, COND_NE, 0);
>
>      add_qemu_ldst_label(s, false, oi, datalo, datahi, addrlo, addrhi,
>                          s->code_ptr, label_ptr);


--
Alex Bennée

[Qemu-devel] [PATCH v3 07/16] tcg/ppc: Fold away "noaddr" branch routines

[Richard Henderson]

There is no longer a need for preserving branch offset operands,
as we no longer re-translate.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/ppc/tcg-target.inc.c | 25 +++++++------------------
 1 file changed, 7 insertions(+), 18 deletions(-)

diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index c2f729ee8f..2e2a22f579 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -210,18 +210,6 @@ static void reloc_pc14(tcg_insn_unit *pc, tcg_insn_unit *target)
     *pc = (*pc & ~0xfffc) | reloc_pc14_val(pc, target);
 }
 
-static inline void tcg_out_b_noaddr(TCGContext *s, int insn)
-{
-    unsigned retrans = *s->code_ptr & 0x3fffffc;
-    tcg_out32(s, insn | retrans);
-}
-
-static inline void tcg_out_bc_noaddr(TCGContext *s, int insn)
-{
-    unsigned retrans = *s->code_ptr & 0xfffc;
-    tcg_out32(s, insn | retrans);
-}
-
 /* parse target specific constraints */
 static const char *target_parse_constraint(TCGArgConstraint *ct,
                                            const char *ct_str, TCGType type)
@@ -1179,11 +1167,11 @@ static void tcg_out_setcond(TCGContext *s, TCGType type, TCGCond cond,
 static void tcg_out_bc(TCGContext *s, int bc, TCGLabel *l)
 {
     if (l->has_value) {
-        tcg_out32(s, bc | reloc_pc14_val(s->code_ptr, l->u.value_ptr));
+        bc |= reloc_pc14_val(s->code_ptr, l->u.value_ptr);
     } else {
         tcg_out_reloc(s, s->code_ptr, R_PPC_REL14, l, 0);
-        tcg_out_bc_noaddr(s, bc);
     }
+    tcg_out32(s, bc);
 }
 
 static void tcg_out_brcond(TCGContext *s, TCGCond cond,
@@ -1771,7 +1759,7 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is_64)
 
     /* Load a pointer into the current opcode w/conditional branch-link. */
     label_ptr = s->code_ptr;
-    tcg_out_bc_noaddr(s, BC | BI(7, CR_EQ) | BO_COND_FALSE | LK);
+    tcg_out32(s, BC | BI(7, CR_EQ) | BO_COND_FALSE | LK);
 
     rbase = TCG_REG_R3;
 #else  /* !CONFIG_SOFTMMU */
@@ -1846,7 +1834,7 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is_64)
 
     /* Load a pointer into the current opcode w/conditional branch-link. */
     label_ptr = s->code_ptr;
-    tcg_out_bc_noaddr(s, BC | BI(7, CR_EQ) | BO_COND_FALSE | LK);
+    tcg_out32(s, BC | BI(7, CR_EQ) | BO_COND_FALSE | LK);
 
     rbase = TCG_REG_R3;
 #else  /* !CONFIG_SOFTMMU */
@@ -2044,13 +2032,14 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
     case INDEX_op_br:
         {
             TCGLabel *l = arg_label(args[0]);
+            uint32_t insn = B;
 
             if (l->has_value) {
-                tcg_out_b(s, 0, l->u.value_ptr);
+                insn |= reloc_pc24_val(s->code_ptr, l->u.value_ptr);
             } else {
                 tcg_out_reloc(s, s->code_ptr, R_PPC_REL24, l, 0);
-                tcg_out_b_noaddr(s, B);
             }
+            tcg_out32(s, insn);
         }
         break;
     case INDEX_op_ld8u_i32:
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 07/16] tcg/ppc: Fold away "noaddr" branch routines

[Alex Bennée]

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

> There is no longer a need for preserving branch offset operands,
> as we no longer re-translate.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/ppc/tcg-target.inc.c | 25 +++++++------------------
>  1 file changed, 7 insertions(+), 18 deletions(-)
>
> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
> index c2f729ee8f..2e2a22f579 100644
> --- a/tcg/ppc/tcg-target.inc.c
> +++ b/tcg/ppc/tcg-target.inc.c
> @@ -210,18 +210,6 @@ static void reloc_pc14(tcg_insn_unit *pc, tcg_insn_unit *target)
>      *pc = (*pc & ~0xfffc) | reloc_pc14_val(pc, target);
>  }
>
> -static inline void tcg_out_b_noaddr(TCGContext *s, int insn)
> -{
> -    unsigned retrans = *s->code_ptr & 0x3fffffc;
> -    tcg_out32(s, insn | retrans);
> -}
> -
> -static inline void tcg_out_bc_noaddr(TCGContext *s, int insn)
> -{
> -    unsigned retrans = *s->code_ptr & 0xfffc;
> -    tcg_out32(s, insn | retrans);
> -}
> -
>  /* parse target specific constraints */
>  static const char *target_parse_constraint(TCGArgConstraint *ct,
>                                             const char *ct_str, TCGType type)
> @@ -1179,11 +1167,11 @@ static void tcg_out_setcond(TCGContext *s, TCGType type, TCGCond cond,
>  static void tcg_out_bc(TCGContext *s, int bc, TCGLabel *l)
>  {
>      if (l->has_value) {
> -        tcg_out32(s, bc | reloc_pc14_val(s->code_ptr, l->u.value_ptr));
> +        bc |= reloc_pc14_val(s->code_ptr, l->u.value_ptr);
>      } else {
>          tcg_out_reloc(s, s->code_ptr, R_PPC_REL14, l, 0);
> -        tcg_out_bc_noaddr(s, bc);
>      }
> +    tcg_out32(s, bc);
>  }
>
>  static void tcg_out_brcond(TCGContext *s, TCGCond cond,
> @@ -1771,7 +1759,7 @@ static void tcg_out_qemu_ld(TCGContext *s, const TCGArg *args, bool is_64)
>
>      /* Load a pointer into the current opcode w/conditional branch-link. */
>      label_ptr = s->code_ptr;
> -    tcg_out_bc_noaddr(s, BC | BI(7, CR_EQ) | BO_COND_FALSE | LK);
> +    tcg_out32(s, BC | BI(7, CR_EQ) | BO_COND_FALSE | LK);
>
>      rbase = TCG_REG_R3;
>  #else  /* !CONFIG_SOFTMMU */
> @@ -1846,7 +1834,7 @@ static void tcg_out_qemu_st(TCGContext *s, const TCGArg *args, bool is_64)
>
>      /* Load a pointer into the current opcode w/conditional branch-link. */
>      label_ptr = s->code_ptr;
> -    tcg_out_bc_noaddr(s, BC | BI(7, CR_EQ) | BO_COND_FALSE | LK);
> +    tcg_out32(s, BC | BI(7, CR_EQ) | BO_COND_FALSE | LK);
>
>      rbase = TCG_REG_R3;
>  #else  /* !CONFIG_SOFTMMU */
> @@ -2044,13 +2032,14 @@ static void tcg_out_op(TCGContext *s, TCGOpcode opc, const TCGArg *args,
>      case INDEX_op_br:
>          {
>              TCGLabel *l = arg_label(args[0]);
> +            uint32_t insn = B;
>
>              if (l->has_value) {
> -                tcg_out_b(s, 0, l->u.value_ptr);
> +                insn |= reloc_pc24_val(s->code_ptr, l->u.value_ptr);
>              } else {
>                  tcg_out_reloc(s, s->code_ptr, R_PPC_REL24, l, 0);
> -                tcg_out_b_noaddr(s, B);
>              }
> +            tcg_out32(s, insn);
>          }
>          break;
>      case INDEX_op_ld8u_i32:


--
Alex Bennée

[Qemu-devel] [PATCH v3 08/16] tcg/s390: Remove retranslation code

[Richard Henderson]

There is no longer a need for preserving branch offset operands,
as we no longer re-translate.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/s390/tcg-target.inc.c | 10 ++--------
 1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/tcg/s390/tcg-target.inc.c b/tcg/s390/tcg-target.inc.c
index 17c435ade5..96c344142e 100644
--- a/tcg/s390/tcg-target.inc.c
+++ b/tcg/s390/tcg-target.inc.c
@@ -1329,13 +1329,11 @@ static void tgen_branch(TCGContext *s, int cc, TCGLabel *l)
 static void tgen_compare_branch(TCGContext *s, S390Opcode opc, int cc,
                                 TCGReg r1, TCGReg r2, TCGLabel *l)
 {
-    intptr_t off;
+    intptr_t off = 0;
 
     if (l->has_value) {
         off = l->u.value_ptr - s->code_ptr;
     } else {
-        /* We need to keep the offset unchanged for retranslation.  */
-        off = s->code_ptr[1];
         tcg_out_reloc(s, s->code_ptr + 1, R_390_PC16DBL, l, 2);
     }
 
@@ -1347,13 +1345,11 @@ static void tgen_compare_branch(TCGContext *s, S390Opcode opc, int cc,
 static void tgen_compare_imm_branch(TCGContext *s, S390Opcode opc, int cc,
                                     TCGReg r1, int i2, TCGLabel *l)
 {
-    tcg_target_long off;
+    tcg_target_long off = 0;
 
     if (l->has_value) {
         off = l->u.value_ptr - s->code_ptr;
     } else {
-        /* We need to keep the offset unchanged for retranslation.  */
-        off = s->code_ptr[1];
         tcg_out_reloc(s, s->code_ptr + 1, R_390_PC16DBL, l, 2);
     }
 
@@ -1696,7 +1692,6 @@ static void tcg_out_qemu_ld(TCGContext* s, TCGReg data_reg, TCGReg addr_reg,
 
     base_reg = tcg_out_tlb_read(s, addr_reg, opc, mem_index, 1);
 
-    /* We need to keep the offset unchanged for retranslation.  */
     tcg_out16(s, RI_BRC | (S390_CC_NE << 4));
     label_ptr = s->code_ptr;
     s->code_ptr += 1;
@@ -1724,7 +1719,6 @@ static void tcg_out_qemu_st(TCGContext* s, TCGReg data_reg, TCGReg addr_reg,
 
     base_reg = tcg_out_tlb_read(s, addr_reg, opc, mem_index, 0);
 
-    /* We need to keep the offset unchanged for retranslation.  */
     tcg_out16(s, RI_BRC | (S390_CC_NE << 4));
     label_ptr = s->code_ptr;
     s->code_ptr += 1;
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 08/16] tcg/s390: Remove retranslation code

[Alex Bennée]

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

> There is no longer a need for preserving branch offset operands,
> as we no longer re-translate.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/s390/tcg-target.inc.c | 10 ++--------
>  1 file changed, 2 insertions(+), 8 deletions(-)
>
> diff --git a/tcg/s390/tcg-target.inc.c b/tcg/s390/tcg-target.inc.c
> index 17c435ade5..96c344142e 100644
> --- a/tcg/s390/tcg-target.inc.c
> +++ b/tcg/s390/tcg-target.inc.c
> @@ -1329,13 +1329,11 @@ static void tgen_branch(TCGContext *s, int cc, TCGLabel *l)
>  static void tgen_compare_branch(TCGContext *s, S390Opcode opc, int cc,
>                                  TCGReg r1, TCGReg r2, TCGLabel *l)
>  {
> -    intptr_t off;
> +    intptr_t off = 0;
>
>      if (l->has_value) {
>          off = l->u.value_ptr - s->code_ptr;
>      } else {
> -        /* We need to keep the offset unchanged for retranslation.  */
> -        off = s->code_ptr[1];
>          tcg_out_reloc(s, s->code_ptr + 1, R_390_PC16DBL, l, 2);
>      }
>
> @@ -1347,13 +1345,11 @@ static void tgen_compare_branch(TCGContext *s, S390Opcode opc, int cc,
>  static void tgen_compare_imm_branch(TCGContext *s, S390Opcode opc, int cc,
>                                      TCGReg r1, int i2, TCGLabel *l)
>  {
> -    tcg_target_long off;
> +    tcg_target_long off = 0;
>
>      if (l->has_value) {
>          off = l->u.value_ptr - s->code_ptr;
>      } else {
> -        /* We need to keep the offset unchanged for retranslation.  */
> -        off = s->code_ptr[1];
>          tcg_out_reloc(s, s->code_ptr + 1, R_390_PC16DBL, l, 2);
>      }
>
> @@ -1696,7 +1692,6 @@ static void tcg_out_qemu_ld(TCGContext* s, TCGReg data_reg, TCGReg addr_reg,
>
>      base_reg = tcg_out_tlb_read(s, addr_reg, opc, mem_index, 1);
>
> -    /* We need to keep the offset unchanged for retranslation.  */
>      tcg_out16(s, RI_BRC | (S390_CC_NE << 4));
>      label_ptr = s->code_ptr;
>      s->code_ptr += 1;
> @@ -1724,7 +1719,6 @@ static void tcg_out_qemu_st(TCGContext* s, TCGReg data_reg, TCGReg addr_reg,
>
>      base_reg = tcg_out_tlb_read(s, addr_reg, opc, mem_index, 0);
>
> -    /* We need to keep the offset unchanged for retranslation.  */
>      tcg_out16(s, RI_BRC | (S390_CC_NE << 4));
>      label_ptr = s->code_ptr;
>      s->code_ptr += 1;


--
Alex Bennée

[Qemu-devel] [PATCH v3 09/16] tcg/sparc: Remove retranslation code

[Richard Henderson]

There is no longer a need for preserving branch offset operands,
as we no longer re-translate.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/sparc/tcg-target.inc.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/tcg/sparc/tcg-target.inc.c b/tcg/sparc/tcg-target.inc.c
index 04bdc3df5e..671a04c54b 100644
--- a/tcg/sparc/tcg-target.inc.c
+++ b/tcg/sparc/tcg-target.inc.c
@@ -639,13 +639,11 @@ static void tcg_out_bpcc0(TCGContext *s, int scond, int flags, int off19)
 
 static void tcg_out_bpcc(TCGContext *s, int scond, int flags, TCGLabel *l)
 {
-    int off19;
+    int off19 = 0;
 
     if (l->has_value) {
         off19 = INSN_OFF19(tcg_pcrel_diff(s, l->u.value_ptr));
     } else {
-        /* Make sure to preserve destinations during retranslation.  */
-        off19 = *s->code_ptr & INSN_OFF19(-1);
         tcg_out_reloc(s, s->code_ptr, R_SPARC_WDISP19, l, 0);
     }
     tcg_out_bpcc0(s, scond, flags, off19);
@@ -685,13 +683,11 @@ static void tcg_out_brcond_i64(TCGContext *s, TCGCond cond, TCGReg arg1,
 {
     /* For 64-bit signed comparisons vs zero, we can avoid the compare.  */
     if (arg2 == 0 && !is_unsigned_cond(cond)) {
-        int off16;
+        int off16 = 0;
 
         if (l->has_value) {
             off16 = INSN_OFF16(tcg_pcrel_diff(s, l->u.value_ptr));
         } else {
-            /* Make sure to preserve destinations during retranslation.  */
-            off16 = *s->code_ptr & INSN_OFF16(-1);
             tcg_out_reloc(s, s->code_ptr, R_SPARC_WDISP16, l, 0);
         }
         tcg_out32(s, INSN_OP(0) | INSN_OP2(3) | BPR_PT | INSN_RS1(arg1)
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 09/16] tcg/sparc: Remove retranslation code

[Alex Bennée]

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

> There is no longer a need for preserving branch offset operands,
> as we no longer re-translate.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/sparc/tcg-target.inc.c | 8 ++------
>  1 file changed, 2 insertions(+), 6 deletions(-)
>
> diff --git a/tcg/sparc/tcg-target.inc.c b/tcg/sparc/tcg-target.inc.c
> index 04bdc3df5e..671a04c54b 100644
> --- a/tcg/sparc/tcg-target.inc.c
> +++ b/tcg/sparc/tcg-target.inc.c
> @@ -639,13 +639,11 @@ static void tcg_out_bpcc0(TCGContext *s, int scond, int flags, int off19)
>
>  static void tcg_out_bpcc(TCGContext *s, int scond, int flags, TCGLabel *l)
>  {
> -    int off19;
> +    int off19 = 0;
>
>      if (l->has_value) {
>          off19 = INSN_OFF19(tcg_pcrel_diff(s, l->u.value_ptr));
>      } else {
> -        /* Make sure to preserve destinations during retranslation.  */
> -        off19 = *s->code_ptr & INSN_OFF19(-1);
>          tcg_out_reloc(s, s->code_ptr, R_SPARC_WDISP19, l, 0);
>      }
>      tcg_out_bpcc0(s, scond, flags, off19);
> @@ -685,13 +683,11 @@ static void tcg_out_brcond_i64(TCGContext *s, TCGCond cond, TCGReg arg1,
>  {
>      /* For 64-bit signed comparisons vs zero, we can avoid the compare.  */
>      if (arg2 == 0 && !is_unsigned_cond(cond)) {
> -        int off16;
> +        int off16 = 0;
>
>          if (l->has_value) {
>              off16 = INSN_OFF16(tcg_pcrel_diff(s, l->u.value_ptr));
>          } else {
> -            /* Make sure to preserve destinations during retranslation.  */
> -            off16 = *s->code_ptr & INSN_OFF16(-1);
>              tcg_out_reloc(s, s->code_ptr, R_SPARC_WDISP16, l, 0);
>          }
>          tcg_out32(s, INSN_OP(0) | INSN_OP2(3) | BPR_PT | INSN_RS1(arg1)


--
Alex Bennée

[Qemu-devel] [PATCH v3 10/16] tcg/mips: Remove retranslation code

[Richard Henderson]

There is no longer a need for preserving branch offset operands,
as we no longer re-translate.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/mips/tcg-target.inc.c | 7 +------
 1 file changed, 1 insertion(+), 6 deletions(-)

diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
index cff525373b..e21cb1ae28 100644
--- a/tcg/mips/tcg-target.inc.c
+++ b/tcg/mips/tcg-target.inc.c
@@ -483,12 +483,7 @@ static inline void tcg_out_opc_bf64(TCGContext *s, MIPSInsn opc, MIPSInsn opm,
 static inline void tcg_out_opc_br(TCGContext *s, MIPSInsn opc,
                                   TCGReg rt, TCGReg rs)
 {
-    /* We pay attention here to not modify the branch target by reading
-       the existing value and using it again. This ensure that caches and
-       memory are kept coherent during retranslation. */
-    uint16_t offset = (uint16_t)*s->code_ptr;
-
-    tcg_out_opc_imm(s, opc, rt, rs, offset);
+    tcg_out_opc_imm(s, opc, rt, rs, 0);
 }
 
 /*
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 10/16] tcg/mips: Remove retranslation code

[Alex Bennée]

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

> There is no longer a need for preserving branch offset operands,
> as we no longer re-translate.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/mips/tcg-target.inc.c | 7 +------
>  1 file changed, 1 insertion(+), 6 deletions(-)
>
> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
> index cff525373b..e21cb1ae28 100644
> --- a/tcg/mips/tcg-target.inc.c
> +++ b/tcg/mips/tcg-target.inc.c
> @@ -483,12 +483,7 @@ static inline void tcg_out_opc_bf64(TCGContext *s, MIPSInsn opc, MIPSInsn opm,
>  static inline void tcg_out_opc_br(TCGContext *s, MIPSInsn opc,
>                                    TCGReg rt, TCGReg rs)
>  {
> -    /* We pay attention here to not modify the branch target by reading
> -       the existing value and using it again. This ensure that caches and
> -       memory are kept coherent during retranslation. */
> -    uint16_t offset = (uint16_t)*s->code_ptr;
> -
> -    tcg_out_opc_imm(s, opc, rt, rs, offset);
> +    tcg_out_opc_imm(s, opc, rt, rs, 0);
>  }
>
>  /*


--
Alex Bennée

[Qemu-devel] [PATCH v3 11/16] tcg: Return success from patch_reloc

[Richard Henderson]

This will move the assert for success from within (subroutines of)
patch_reloc into the callers.  It will also let new code do something
different when a relocation is out of range.

For the moment, all backends are trivially converted to return true.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/aarch64/tcg-target.inc.c | 3 ++-
 tcg/arm/tcg-target.inc.c     | 3 ++-
 tcg/i386/tcg-target.inc.c    | 3 ++-
 tcg/mips/tcg-target.inc.c    | 3 ++-
 tcg/ppc/tcg-target.inc.c     | 3 ++-
 tcg/s390/tcg-target.inc.c    | 3 ++-
 tcg/sparc/tcg-target.inc.c   | 5 +++--
 tcg/tcg.c                    | 8 +++++---
 tcg/tci/tcg-target.inc.c     | 3 ++-
 9 files changed, 22 insertions(+), 12 deletions(-)

diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
index 28de0226fb..16f08c59c4 100644
--- a/tcg/aarch64/tcg-target.inc.c
+++ b/tcg/aarch64/tcg-target.inc.c
@@ -94,7 +94,7 @@ static inline void reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
     *code_ptr = deposit32(*code_ptr, 5, 19, offset);
 }
 
-static inline void patch_reloc(tcg_insn_unit *code_ptr, int type,
+static inline bool patch_reloc(tcg_insn_unit *code_ptr, int type,
                                intptr_t value, intptr_t addend)
 {
     tcg_debug_assert(addend == 0);
@@ -109,6 +109,7 @@ static inline void patch_reloc(tcg_insn_unit *code_ptr, int type,
     default:
         tcg_abort();
     }
+    return true;
 }
 
 #define TCG_CT_CONST_AIMM 0x100
diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
index 1651f00281..deefa20fbf 100644
--- a/tcg/arm/tcg-target.inc.c
+++ b/tcg/arm/tcg-target.inc.c
@@ -193,7 +193,7 @@ static inline void reloc_pc24(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
     *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff);
 }
 
-static void patch_reloc(tcg_insn_unit *code_ptr, int type,
+static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
                         intptr_t value, intptr_t addend)
 {
     tcg_debug_assert(addend == 0);
@@ -229,6 +229,7 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
     } else {
         g_assert_not_reached();
     }
+    return true;
 }
 
 #define TCG_CT_CONST_ARM  0x100
diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
index 436195894b..5c88f1f36b 100644
--- a/tcg/i386/tcg-target.inc.c
+++ b/tcg/i386/tcg-target.inc.c
@@ -167,7 +167,7 @@ static bool have_lzcnt;
 
 static tcg_insn_unit *tb_ret_addr;
 
-static void patch_reloc(tcg_insn_unit *code_ptr, int type,
+static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
                         intptr_t value, intptr_t addend)
 {
     value += addend;
@@ -191,6 +191,7 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
     default:
         tcg_abort();
     }
+    return true;
 }
 
 #if TCG_TARGET_REG_BITS == 64
diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
index e21cb1ae28..a06ff257fa 100644
--- a/tcg/mips/tcg-target.inc.c
+++ b/tcg/mips/tcg-target.inc.c
@@ -168,12 +168,13 @@ static inline void reloc_26(tcg_insn_unit *pc, tcg_insn_unit *target)
     *pc = deposit32(*pc, 0, 26, reloc_26_val(pc, target));
 }
 
-static void patch_reloc(tcg_insn_unit *code_ptr, int type,
+static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
                         intptr_t value, intptr_t addend)
 {
     tcg_debug_assert(type == R_MIPS_PC16);
     tcg_debug_assert(addend == 0);
     reloc_pc16(code_ptr, (tcg_insn_unit *)value);
+    return true;
 }
 
 #define TCG_CT_CONST_ZERO 0x100
diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index 2e2a22f579..860b0d36e1 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -513,7 +513,7 @@ static const uint32_t tcg_to_isel[] = {
     [TCG_COND_GTU] = ISEL | BC_(7, CR_GT),
 };
 
-static void patch_reloc(tcg_insn_unit *code_ptr, int type,
+static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
                         intptr_t value, intptr_t addend)
 {
     tcg_insn_unit *target;
@@ -548,6 +548,7 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
     default:
         g_assert_not_reached();
     }
+    return true;
 }
 
 static void tcg_out_mem_long(TCGContext *s, int opi, int opx, TCGReg rt,
diff --git a/tcg/s390/tcg-target.inc.c b/tcg/s390/tcg-target.inc.c
index 96c344142e..68a4c60394 100644
--- a/tcg/s390/tcg-target.inc.c
+++ b/tcg/s390/tcg-target.inc.c
@@ -366,7 +366,7 @@ static void * const qemu_st_helpers[16] = {
 static tcg_insn_unit *tb_ret_addr;
 uint64_t s390_facilities;
 
-static void patch_reloc(tcg_insn_unit *code_ptr, int type,
+static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
                         intptr_t value, intptr_t addend)
 {
     intptr_t pcrel2;
@@ -393,6 +393,7 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
     default:
         g_assert_not_reached();
     }
+    return true;
 }
 
 /* parse target specific constraints */
diff --git a/tcg/sparc/tcg-target.inc.c b/tcg/sparc/tcg-target.inc.c
index 671a04c54b..cadda770c4 100644
--- a/tcg/sparc/tcg-target.inc.c
+++ b/tcg/sparc/tcg-target.inc.c
@@ -291,7 +291,7 @@ static inline int check_fit_i32(int32_t val, unsigned int bits)
 # define check_fit_ptr  check_fit_i32
 #endif
 
-static void patch_reloc(tcg_insn_unit *code_ptr, int type,
+static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
                         intptr_t value, intptr_t addend)
 {
     uint32_t insn = *code_ptr;
@@ -328,12 +328,13 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
         /* Note that we're abusing this reloc type for our own needs.  */
         code_ptr[0] = deposit32(code_ptr[0], 0, 22, value >> 10);
         code_ptr[1] = deposit32(code_ptr[1], 0, 10, value);
-        return;
+        return true;
     default:
         g_assert_not_reached();
     }
 
     *code_ptr = insn;
+    return true;
 }
 
 /* parse target specific constraints */
diff --git a/tcg/tcg.c b/tcg/tcg.c
index e85133ef05..54f1272187 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -66,7 +66,7 @@
 static void tcg_target_init(TCGContext *s);
 static const TCGTargetOpDef *tcg_target_op_def(TCGOpcode);
 static void tcg_target_qemu_prologue(TCGContext *s);
-static void patch_reloc(tcg_insn_unit *code_ptr, int type,
+static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
                         intptr_t value, intptr_t addend);
 
 /* The CIE and FDE header definitions will be common to all hosts.  */
@@ -268,7 +268,8 @@ static void tcg_out_reloc(TCGContext *s, tcg_insn_unit *code_ptr, int type,
         /* FIXME: This may break relocations on RISC targets that
            modify instruction fields in place.  The caller may not have 
            written the initial value.  */
-        patch_reloc(code_ptr, type, l->u.value, addend);
+        bool ok = patch_reloc(code_ptr, type, l->u.value, addend);
+        tcg_debug_assert(ok);
     } else {
         /* add a new relocation entry */
         r = tcg_malloc(sizeof(TCGRelocation));
@@ -288,7 +289,8 @@ static void tcg_out_label(TCGContext *s, TCGLabel *l, tcg_insn_unit *ptr)
     tcg_debug_assert(!l->has_value);
 
     for (r = l->u.first_reloc; r != NULL; r = r->next) {
-        patch_reloc(r->ptr, r->type, value, r->addend);
+        bool ok = patch_reloc(r->ptr, r->type, value, r->addend);
+        tcg_debug_assert(ok);
     }
 
     l->has_value = 1;
diff --git a/tcg/tci/tcg-target.inc.c b/tcg/tci/tcg-target.inc.c
index 62ed097254..0015a98485 100644
--- a/tcg/tci/tcg-target.inc.c
+++ b/tcg/tci/tcg-target.inc.c
@@ -369,7 +369,7 @@ static const char *const tcg_target_reg_names[TCG_TARGET_NB_REGS] = {
 };
 #endif
 
-static void patch_reloc(tcg_insn_unit *code_ptr, int type,
+static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
                         intptr_t value, intptr_t addend)
 {
     /* tcg_out_reloc always uses the same type, addend. */
@@ -381,6 +381,7 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
     } else {
         tcg_patch64(code_ptr, value);
     }
+    return true;
 }
 
 /* Parse target specific constraints. */
-- 
2.17.2

Re: [Qemu-devel] [PATCH v3 11/16] tcg: Return success from patch_reloc

[Alex Bennée]

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

> This will move the assert for success from within (subroutines of)
> patch_reloc into the callers.  It will also let new code do something
> different when a relocation is out of range.
>
> For the moment, all backends are trivially converted to return true.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/aarch64/tcg-target.inc.c | 3 ++-
>  tcg/arm/tcg-target.inc.c     | 3 ++-
>  tcg/i386/tcg-target.inc.c    | 3 ++-
>  tcg/mips/tcg-target.inc.c    | 3 ++-
>  tcg/ppc/tcg-target.inc.c     | 3 ++-
>  tcg/s390/tcg-target.inc.c    | 3 ++-
>  tcg/sparc/tcg-target.inc.c   | 5 +++--
>  tcg/tcg.c                    | 8 +++++---
>  tcg/tci/tcg-target.inc.c     | 3 ++-
>  9 files changed, 22 insertions(+), 12 deletions(-)
>
> diff --git a/tcg/aarch64/tcg-target.inc.c b/tcg/aarch64/tcg-target.inc.c
> index 28de0226fb..16f08c59c4 100644
> --- a/tcg/aarch64/tcg-target.inc.c
> +++ b/tcg/aarch64/tcg-target.inc.c
> @@ -94,7 +94,7 @@ static inline void reloc_pc19(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
>      *code_ptr = deposit32(*code_ptr, 5, 19, offset);
>  }
>
> -static inline void patch_reloc(tcg_insn_unit *code_ptr, int type,
> +static inline bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>                                 intptr_t value, intptr_t addend)
>  {
>      tcg_debug_assert(addend == 0);
> @@ -109,6 +109,7 @@ static inline void patch_reloc(tcg_insn_unit *code_ptr, int type,
>      default:
>          tcg_abort();
>      }
> +    return true;
>  }
>
>  #define TCG_CT_CONST_AIMM 0x100
> diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
> index 1651f00281..deefa20fbf 100644
> --- a/tcg/arm/tcg-target.inc.c
> +++ b/tcg/arm/tcg-target.inc.c
> @@ -193,7 +193,7 @@ static inline void reloc_pc24(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
>      *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff);
>  }
>
> -static void patch_reloc(tcg_insn_unit *code_ptr, int type,
> +static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>                          intptr_t value, intptr_t addend)
>  {
>      tcg_debug_assert(addend == 0);
> @@ -229,6 +229,7 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
>      } else {
>          g_assert_not_reached();
>      }
> +    return true;
>  }
>
>  #define TCG_CT_CONST_ARM  0x100
> diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
> index 436195894b..5c88f1f36b 100644
> --- a/tcg/i386/tcg-target.inc.c
> +++ b/tcg/i386/tcg-target.inc.c
> @@ -167,7 +167,7 @@ static bool have_lzcnt;
>
>  static tcg_insn_unit *tb_ret_addr;
>
> -static void patch_reloc(tcg_insn_unit *code_ptr, int type,
> +static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>                          intptr_t value, intptr_t addend)
>  {
>      value += addend;
> @@ -191,6 +191,7 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
>      default:
>          tcg_abort();
>      }
> +    return true;
>  }
>
>  #if TCG_TARGET_REG_BITS == 64
> diff --git a/tcg/mips/tcg-target.inc.c b/tcg/mips/tcg-target.inc.c
> index e21cb1ae28..a06ff257fa 100644
> --- a/tcg/mips/tcg-target.inc.c
> +++ b/tcg/mips/tcg-target.inc.c
> @@ -168,12 +168,13 @@ static inline void reloc_26(tcg_insn_unit *pc, tcg_insn_unit *target)
>      *pc = deposit32(*pc, 0, 26, reloc_26_val(pc, target));
>  }
>
> -static void patch_reloc(tcg_insn_unit *code_ptr, int type,
> +static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>                          intptr_t value, intptr_t addend)
>  {
>      tcg_debug_assert(type == R_MIPS_PC16);
>      tcg_debug_assert(addend == 0);
>      reloc_pc16(code_ptr, (tcg_insn_unit *)value);
> +    return true;
>  }
>
>  #define TCG_CT_CONST_ZERO 0x100
> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
> index 2e2a22f579..860b0d36e1 100644
> --- a/tcg/ppc/tcg-target.inc.c
> +++ b/tcg/ppc/tcg-target.inc.c
> @@ -513,7 +513,7 @@ static const uint32_t tcg_to_isel[] = {
>      [TCG_COND_GTU] = ISEL | BC_(7, CR_GT),
>  };
>
> -static void patch_reloc(tcg_insn_unit *code_ptr, int type,
> +static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>                          intptr_t value, intptr_t addend)
>  {
>      tcg_insn_unit *target;
> @@ -548,6 +548,7 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
>      default:
>          g_assert_not_reached();
>      }
> +    return true;
>  }
>
>  static void tcg_out_mem_long(TCGContext *s, int opi, int opx, TCGReg rt,
> diff --git a/tcg/s390/tcg-target.inc.c b/tcg/s390/tcg-target.inc.c
> index 96c344142e..68a4c60394 100644
> --- a/tcg/s390/tcg-target.inc.c
> +++ b/tcg/s390/tcg-target.inc.c
> @@ -366,7 +366,7 @@ static void * const qemu_st_helpers[16] = {
>  static tcg_insn_unit *tb_ret_addr;
>  uint64_t s390_facilities;
>
> -static void patch_reloc(tcg_insn_unit *code_ptr, int type,
> +static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>                          intptr_t value, intptr_t addend)
>  {
>      intptr_t pcrel2;
> @@ -393,6 +393,7 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
>      default:
>          g_assert_not_reached();
>      }
> +    return true;
>  }
>
>  /* parse target specific constraints */
> diff --git a/tcg/sparc/tcg-target.inc.c b/tcg/sparc/tcg-target.inc.c
> index 671a04c54b..cadda770c4 100644
> --- a/tcg/sparc/tcg-target.inc.c
> +++ b/tcg/sparc/tcg-target.inc.c
> @@ -291,7 +291,7 @@ static inline int check_fit_i32(int32_t val, unsigned int bits)
>  # define check_fit_ptr  check_fit_i32
>  #endif
>
> -static void patch_reloc(tcg_insn_unit *code_ptr, int type,
> +static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>                          intptr_t value, intptr_t addend)
>  {
>      uint32_t insn = *code_ptr;
> @@ -328,12 +328,13 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
>          /* Note that we're abusing this reloc type for our own needs.  */
>          code_ptr[0] = deposit32(code_ptr[0], 0, 22, value >> 10);
>          code_ptr[1] = deposit32(code_ptr[1], 0, 10, value);
> -        return;
> +        return true;
>      default:
>          g_assert_not_reached();
>      }
>
>      *code_ptr = insn;
> +    return true;
>  }
>
>  /* parse target specific constraints */
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index e85133ef05..54f1272187 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -66,7 +66,7 @@
>  static void tcg_target_init(TCGContext *s);
>  static const TCGTargetOpDef *tcg_target_op_def(TCGOpcode);
>  static void tcg_target_qemu_prologue(TCGContext *s);
> -static void patch_reloc(tcg_insn_unit *code_ptr, int type,
> +static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>                          intptr_t value, intptr_t addend);
>
>  /* The CIE and FDE header definitions will be common to all hosts.  */
> @@ -268,7 +268,8 @@ static void tcg_out_reloc(TCGContext *s, tcg_insn_unit *code_ptr, int type,
>          /* FIXME: This may break relocations on RISC targets that
>             modify instruction fields in place.  The caller may not have
>             written the initial value.  */
> -        patch_reloc(code_ptr, type, l->u.value, addend);
> +        bool ok = patch_reloc(code_ptr, type, l->u.value, addend);
> +        tcg_debug_assert(ok);
>      } else {
>          /* add a new relocation entry */
>          r = tcg_malloc(sizeof(TCGRelocation));
> @@ -288,7 +289,8 @@ static void tcg_out_label(TCGContext *s, TCGLabel *l, tcg_insn_unit *ptr)
>      tcg_debug_assert(!l->has_value);
>
>      for (r = l->u.first_reloc; r != NULL; r = r->next) {
> -        patch_reloc(r->ptr, r->type, value, r->addend);
> +        bool ok = patch_reloc(r->ptr, r->type, value, r->addend);
> +        tcg_debug_assert(ok);
>      }
>
>      l->has_value = 1;
> diff --git a/tcg/tci/tcg-target.inc.c b/tcg/tci/tcg-target.inc.c
> index 62ed097254..0015a98485 100644
> --- a/tcg/tci/tcg-target.inc.c
> +++ b/tcg/tci/tcg-target.inc.c
> @@ -369,7 +369,7 @@ static const char *const tcg_target_reg_names[TCG_TARGET_NB_REGS] = {
>  };
>  #endif
>
> -static void patch_reloc(tcg_insn_unit *code_ptr, int type,
> +static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>                          intptr_t value, intptr_t addend)
>  {
>      /* tcg_out_reloc always uses the same type, addend. */
> @@ -381,6 +381,7 @@ static void patch_reloc(tcg_insn_unit *code_ptr, int type,
>      } else {
>          tcg_patch64(code_ptr, value);
>      }
> +    return true;
>  }
>
>  /* Parse target specific constraints. */


--
Alex Bennée

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

[Richard Henderson]

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/i386/tcg-target.inc.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
index 5c88f1f36b..28192f4608 100644
--- a/tcg/i386/tcg-target.inc.c
+++ b/tcg/i386/tcg-target.inc.c
@@ -175,7 +175,7 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
     case R_386_PC32:
         value -= (uintptr_t)code_ptr;
         if (value != (int32_t)value) {
-            tcg_abort();
+            return false;
         }
         /* FALLTHRU */
     case R_386_32:
@@ -184,7 +184,7 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
     case R_386_PC8:
         value -= (uintptr_t)code_ptr;
         if (value != (int8_t)value) {
-            tcg_abort();
+            return false;
         }
         tcg_patch8(code_ptr, value);
         break;
-- 
2.17.2

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

[Alex Bennée]

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

> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/i386/tcg-target.inc.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/tcg/i386/tcg-target.inc.c b/tcg/i386/tcg-target.inc.c
> index 5c88f1f36b..28192f4608 100644
> --- a/tcg/i386/tcg-target.inc.c
> +++ b/tcg/i386/tcg-target.inc.c
> @@ -175,7 +175,7 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>      case R_386_PC32:
>          value -= (uintptr_t)code_ptr;
>          if (value != (int32_t)value) {
> -            tcg_abort();
> +            return false;
>          }
>          /* FALLTHRU */
>      case R_386_32:
> @@ -184,7 +184,7 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>      case R_386_PC8:
>          value -= (uintptr_t)code_ptr;
>          if (value != (int8_t)value) {
> -            tcg_abort();
> +            return false;
>          }
>          tcg_patch8(code_ptr, value);
>          break;


--
Alex Bennée

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

[Richard Henderson]

This does require an extra two checks within the slow paths
to replace the assert that we're moving.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 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;
 }
 
 #define TCG_CT_CONST_AIMM 0x100
@@ -1387,7 +1390,8 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
     TCGMemOp opc = get_memop(oi);
     TCGMemOp size = opc & MO_SIZE;
 
-    reloc_pc19(lb->label_ptr[0], s->code_ptr);
+    bool ok = reloc_pc19(lb->label_ptr[0], s->code_ptr);
+    tcg_debug_assert(ok);
 
     tcg_out_mov(s, TCG_TYPE_PTR, TCG_REG_X0, TCG_AREG0);
     tcg_out_mov(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg);
@@ -1409,7 +1413,8 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
     TCGMemOp opc = get_memop(oi);
     TCGMemOp size = opc & MO_SIZE;
 
-    reloc_pc19(lb->label_ptr[0], s->code_ptr);
+    bool ok = reloc_pc19(lb->label_ptr[0], s->code_ptr);
+    tcg_debug_assert(ok);
 
     tcg_out_mov(s, TCG_TYPE_PTR, TCG_REG_X0, TCG_AREG0);
     tcg_out_mov(s, TARGET_LONG_BITS == 64, TCG_REG_X1, lb->addrlo_reg);
-- 
2.17.2

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

[Alex Bennée]

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.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  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 <alex.bennee@linaro.org>


--
Alex Bennée

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

[Richard Henderson]

On 12/3/18 4:43 AM, Alex Bennée wrote:
>>      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

Would it be clearer changed to g_assert_not_reached()?
Because I'm not intending "unknown relocation" to have
the same effect as "out of range relocation".


r~

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

[Alex Bennée]

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

> On 12/3/18 4:43 AM, Alex Bennée wrote:
>>>      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
>
> Would it be clearer changed to g_assert_not_reached()?
> Because I'm not intending "unknown relocation" to have
> the same effect as "out of range relocation".

g_assert_not_reached would probably be better then.

Is there any particular reason tcg has tcg_abort(), is it just for the
slightly different report string?

>
>
> r~


--
Alex Bennée

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

[Richard Henderson]

On 12/3/18 8:15 AM, Alex Bennée wrote:
> 
> Richard Henderson <richard.henderson@linaro.org> writes:
> 
>> On 12/3/18 4:43 AM, Alex Bennée wrote:
>>>>      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
>>
>> Would it be clearer changed to g_assert_not_reached()?
>> Because I'm not intending "unknown relocation" to have
>> the same effect as "out of range relocation".
> 
> g_assert_not_reached would probably be better then.
> 
> Is there any particular reason tcg has tcg_abort(), is it just for the
> slightly different report string?

It's just old, pre-dating a more concerted use of glib.


r~

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

[Richard Henderson]

This does require an extra two checks within the slow paths
to replace the assert that we're moving.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/arm/tcg-target.inc.c | 22 ++++++++++++++++------
 1 file changed, 16 insertions(+), 6 deletions(-)

diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
index deefa20fbf..49f57d655e 100644
--- a/tcg/arm/tcg-target.inc.c
+++ b/tcg/arm/tcg-target.inc.c
@@ -187,10 +187,14 @@ static const uint8_t tcg_cond_to_arm_cond[] = {
     [TCG_COND_GTU] = COND_HI,
 };
 
-static inline void reloc_pc24(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
+static inline bool reloc_pc24(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
 {
     ptrdiff_t offset = (tcg_ptr_byte_diff(target, code_ptr) - 8) >> 2;
-    *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff);
+    if (offset == sextract32(offset, 0, 24)) {
+        *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff);
+        return true;
+    }
+    return false;
 }
 
 static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
@@ -199,7 +203,7 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
     tcg_debug_assert(addend == 0);
 
     if (type == R_ARM_PC24) {
-        reloc_pc24(code_ptr, (tcg_insn_unit *)value);
+        return reloc_pc24(code_ptr, (tcg_insn_unit *)value);
     } else if (type == R_ARM_PC13) {
         intptr_t diff = value - (uintptr_t)(code_ptr + 2);
         tcg_insn_unit insn = *code_ptr;
@@ -213,7 +217,11 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
         } else {
             int rd = extract32(insn, 12, 4);
             int rt = rd == TCG_REG_PC ? TCG_REG_TMP : rd;
-            assert(diff >= 0x1000 && diff < 0x100000);
+
+            if (diff < 0x1000 || diff >= 0x100000) {
+                return false;
+            }
+
             /* add rt, pc, #high */
             *code_ptr++ = ((insn & 0xf0000000) | (1 << 25) | ARITH_ADD
                            | (TCG_REG_PC << 16) | (rt << 12)
@@ -1372,7 +1380,8 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
     TCGMemOp opc = get_memop(oi);
     void *func;
 
-    reloc_pc24(lb->label_ptr[0], s->code_ptr);
+    bool ok = reloc_pc24(lb->label_ptr[0], s->code_ptr);
+    tcg_debug_assert(ok);
 
     argreg = tcg_out_arg_reg32(s, TCG_REG_R0, TCG_AREG0);
     if (TARGET_LONG_BITS == 64) {
@@ -1432,7 +1441,8 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
     TCGMemOpIdx oi = lb->oi;
     TCGMemOp opc = get_memop(oi);
 
-    reloc_pc24(lb->label_ptr[0], s->code_ptr);
+    bool ok = reloc_pc24(lb->label_ptr[0], s->code_ptr);
+    tcg_debug_assert(ok);
 
     argreg = TCG_REG_R0;
     argreg = tcg_out_arg_reg32(s, argreg, TCG_AREG0);
-- 
2.17.2

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

[Alex Bennée]

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.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/arm/tcg-target.inc.c | 22 ++++++++++++++++------
>  1 file changed, 16 insertions(+), 6 deletions(-)
>
> diff --git a/tcg/arm/tcg-target.inc.c b/tcg/arm/tcg-target.inc.c
> index deefa20fbf..49f57d655e 100644
> --- a/tcg/arm/tcg-target.inc.c
> +++ b/tcg/arm/tcg-target.inc.c
> @@ -187,10 +187,14 @@ static const uint8_t tcg_cond_to_arm_cond[] = {
>      [TCG_COND_GTU] = COND_HI,
>  };
>
> -static inline void reloc_pc24(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
> +static inline bool reloc_pc24(tcg_insn_unit *code_ptr, tcg_insn_unit *target)
>  {
>      ptrdiff_t offset = (tcg_ptr_byte_diff(target, code_ptr) - 8) >> 2;
> -    *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff);
> +    if (offset == sextract32(offset, 0, 24)) {
> +        *code_ptr = (*code_ptr & ~0xffffff) | (offset & 0xffffff);
> +        return true;
> +    }
> +    return false;
>  }
>
>  static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
> @@ -199,7 +203,7 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>      tcg_debug_assert(addend == 0);
>
>      if (type == R_ARM_PC24) {
> -        reloc_pc24(code_ptr, (tcg_insn_unit *)value);
> +        return reloc_pc24(code_ptr, (tcg_insn_unit *)value);
>      } else if (type == R_ARM_PC13) {
>          intptr_t diff = value - (uintptr_t)(code_ptr + 2);
>          tcg_insn_unit insn = *code_ptr;
> @@ -213,7 +217,11 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>          } else {
>              int rd = extract32(insn, 12, 4);
>              int rt = rd == TCG_REG_PC ? TCG_REG_TMP : rd;
> -            assert(diff >= 0x1000 && diff < 0x100000);
> +
> +            if (diff < 0x1000 || diff >= 0x100000) {
> +                return false;
> +            }
> +
>              /* add rt, pc, #high */
>              *code_ptr++ = ((insn & 0xf0000000) | (1 << 25) | ARITH_ADD
>                             | (TCG_REG_PC << 16) | (rt << 12)
> @@ -1372,7 +1380,8 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
>      TCGMemOp opc = get_memop(oi);
>      void *func;
>
> -    reloc_pc24(lb->label_ptr[0], s->code_ptr);
> +    bool ok = reloc_pc24(lb->label_ptr[0], s->code_ptr);
> +    tcg_debug_assert(ok);
>
>      argreg = tcg_out_arg_reg32(s, TCG_REG_R0, TCG_AREG0);
>      if (TARGET_LONG_BITS == 64) {
> @@ -1432,7 +1441,8 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
>      TCGMemOpIdx oi = lb->oi;
>      TCGMemOp opc = get_memop(oi);
>
> -    reloc_pc24(lb->label_ptr[0], s->code_ptr);
> +    bool ok = reloc_pc24(lb->label_ptr[0], s->code_ptr);
> +    tcg_debug_assert(ok);
>
>      argreg = TCG_REG_R0;
>      argreg = tcg_out_arg_reg32(s, argreg, TCG_AREG0);


--
Alex Bennée

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

[Richard Henderson]

The reloc_pc{14,24}_val routines retain their asserts.
Use these directly within the slow paths.

Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/ppc/tcg-target.inc.c | 32 +++++++++++++++++++++-----------
 1 file changed, 21 insertions(+), 11 deletions(-)

diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
index 860b0d36e1..8c1cfdd7ac 100644
--- a/tcg/ppc/tcg-target.inc.c
+++ b/tcg/ppc/tcg-target.inc.c
@@ -193,9 +193,14 @@ static uint32_t reloc_pc24_val(tcg_insn_unit *pc, tcg_insn_unit *target)
     return disp & 0x3fffffc;
 }
 
-static void reloc_pc24(tcg_insn_unit *pc, tcg_insn_unit *target)
+static bool reloc_pc24(tcg_insn_unit *pc, tcg_insn_unit *target)
 {
-    *pc = (*pc & ~0x3fffffc) | reloc_pc24_val(pc, target);
+    ptrdiff_t disp = tcg_ptr_byte_diff(target, pc);
+    if (in_range_b(disp)) {
+        *pc = (*pc & ~0x3fffffc) | (disp & 0x3fffffc);
+        return true;
+    }
+    return false;
 }
 
 static uint16_t reloc_pc14_val(tcg_insn_unit *pc, tcg_insn_unit *target)
@@ -205,9 +210,14 @@ static uint16_t reloc_pc14_val(tcg_insn_unit *pc, tcg_insn_unit *target)
     return disp & 0xfffc;
 }
 
-static void reloc_pc14(tcg_insn_unit *pc, tcg_insn_unit *target)
+static bool reloc_pc14(tcg_insn_unit *pc, tcg_insn_unit *target)
 {
-    *pc = (*pc & ~0xfffc) | reloc_pc14_val(pc, target);
+    ptrdiff_t disp = tcg_ptr_byte_diff(target, pc);
+    if (disp == (int16_t) disp) {
+        *pc = (*pc & ~0xfffc) | (disp & 0xfffc);
+        return true;
+    }
+    return false;
 }
 
 /* parse target specific constraints */
@@ -524,11 +534,9 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
 
     switch (type) {
     case R_PPC_REL14:
-        reloc_pc14(code_ptr, target);
-        break;
+        return reloc_pc14(code_ptr, target);
     case R_PPC_REL24:
-        reloc_pc24(code_ptr, target);
-        break;
+        return reloc_pc24(code_ptr, target);
     case R_PPC_ADDR16:
         /* We are abusing this relocation type.  This points to a pair
            of insns, addis + load.  If the displacement is small, we
@@ -540,7 +548,9 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
         } else {
             int16_t lo = value;
             int hi = value - lo;
-            assert(hi + lo == value);
+            if (hi + lo != value) {
+                return false;
+            }
             code_ptr[0] = deposit32(code_ptr[0], 0, 16, hi >> 16);
             code_ptr[1] = deposit32(code_ptr[1], 0, 16, lo);
         }
@@ -1638,7 +1648,7 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
     TCGMemOp opc = get_memop(oi);
     TCGReg hi, lo, arg = TCG_REG_R3;
 
-    reloc_pc14(lb->label_ptr[0], s->code_ptr);
+    **lb->label_ptr |= reloc_pc14_val(*lb->label_ptr, s->code_ptr);
 
     tcg_out_mov(s, TCG_TYPE_PTR, arg++, TCG_AREG0);
 
@@ -1683,7 +1693,7 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
     TCGMemOp s_bits = opc & MO_SIZE;
     TCGReg hi, lo, arg = TCG_REG_R3;
 
-    reloc_pc14(lb->label_ptr[0], s->code_ptr);
+    **lb->label_ptr |= reloc_pc14_val(*lb->label_ptr, s->code_ptr);
 
     tcg_out_mov(s, TCG_TYPE_PTR, arg++, TCG_AREG0);
 
-- 
2.17.2

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

[Alex Bennée]

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

> The reloc_pc{14,24}_val routines retain their asserts.
> Use these directly within the slow paths.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>

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

> ---
>  tcg/ppc/tcg-target.inc.c | 32 +++++++++++++++++++++-----------
>  1 file changed, 21 insertions(+), 11 deletions(-)
>
> diff --git a/tcg/ppc/tcg-target.inc.c b/tcg/ppc/tcg-target.inc.c
> index 860b0d36e1..8c1cfdd7ac 100644
> --- a/tcg/ppc/tcg-target.inc.c
> +++ b/tcg/ppc/tcg-target.inc.c
> @@ -193,9 +193,14 @@ static uint32_t reloc_pc24_val(tcg_insn_unit *pc, tcg_insn_unit *target)
>      return disp & 0x3fffffc;
>  }
>
> -static void reloc_pc24(tcg_insn_unit *pc, tcg_insn_unit *target)
> +static bool reloc_pc24(tcg_insn_unit *pc, tcg_insn_unit *target)
>  {
> -    *pc = (*pc & ~0x3fffffc) | reloc_pc24_val(pc, target);
> +    ptrdiff_t disp = tcg_ptr_byte_diff(target, pc);
> +    if (in_range_b(disp)) {
> +        *pc = (*pc & ~0x3fffffc) | (disp & 0x3fffffc);
> +        return true;
> +    }
> +    return false;
>  }
>
>  static uint16_t reloc_pc14_val(tcg_insn_unit *pc, tcg_insn_unit *target)
> @@ -205,9 +210,14 @@ static uint16_t reloc_pc14_val(tcg_insn_unit *pc, tcg_insn_unit *target)
>      return disp & 0xfffc;
>  }
>
> -static void reloc_pc14(tcg_insn_unit *pc, tcg_insn_unit *target)
> +static bool reloc_pc14(tcg_insn_unit *pc, tcg_insn_unit *target)
>  {
> -    *pc = (*pc & ~0xfffc) | reloc_pc14_val(pc, target);
> +    ptrdiff_t disp = tcg_ptr_byte_diff(target, pc);
> +    if (disp == (int16_t) disp) {
> +        *pc = (*pc & ~0xfffc) | (disp & 0xfffc);
> +        return true;
> +    }
> +    return false;
>  }
>
>  /* parse target specific constraints */
> @@ -524,11 +534,9 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>
>      switch (type) {
>      case R_PPC_REL14:
> -        reloc_pc14(code_ptr, target);
> -        break;
> +        return reloc_pc14(code_ptr, target);
>      case R_PPC_REL24:
> -        reloc_pc24(code_ptr, target);
> -        break;
> +        return reloc_pc24(code_ptr, target);
>      case R_PPC_ADDR16:
>          /* We are abusing this relocation type.  This points to a pair
>             of insns, addis + load.  If the displacement is small, we
> @@ -540,7 +548,9 @@ static bool patch_reloc(tcg_insn_unit *code_ptr, int type,
>          } else {
>              int16_t lo = value;
>              int hi = value - lo;
> -            assert(hi + lo == value);
> +            if (hi + lo != value) {
> +                return false;
> +            }
>              code_ptr[0] = deposit32(code_ptr[0], 0, 16, hi >> 16);
>              code_ptr[1] = deposit32(code_ptr[1], 0, 16, lo);
>          }
> @@ -1638,7 +1648,7 @@ static void tcg_out_qemu_ld_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
>      TCGMemOp opc = get_memop(oi);
>      TCGReg hi, lo, arg = TCG_REG_R3;
>
> -    reloc_pc14(lb->label_ptr[0], s->code_ptr);
> +    **lb->label_ptr |= reloc_pc14_val(*lb->label_ptr, s->code_ptr);
>
>      tcg_out_mov(s, TCG_TYPE_PTR, arg++, TCG_AREG0);
>
> @@ -1683,7 +1693,7 @@ static void tcg_out_qemu_st_slow_path(TCGContext *s, TCGLabelQemuLdst *lb)
>      TCGMemOp s_bits = opc & MO_SIZE;
>      TCGReg hi, lo, arg = TCG_REG_R3;
>
> -    reloc_pc14(lb->label_ptr[0], s->code_ptr);
> +    **lb->label_ptr |= reloc_pc14_val(*lb->label_ptr, s->code_ptr);
>
>      tcg_out_mov(s, TCG_TYPE_PTR, arg++, TCG_AREG0);


--
Alex Bennée

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

[Richard Henderson]

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>
---
 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) {
-- 
2.17.2

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

[Alex Bennée]

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