tcg: Remove assert from set_jmp_reset_offset

[PATCH for-5.2] tcg: Remove assert from set_jmp_reset_offset

Posted by Richard Henderson 3 years, 6 months ago

The range check done here is done later, more appropriately,
at the end of tcg_gen_code.  There, a failing range check
results in a returned error code, which causes the TB to be
restarted at half the size.

Reported-by: Sai Pavan Boddu <saipava@xilinx.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---

Sai, would you try this against your failing testcase?


r~

---
 tcg/tcg.c | 9 +++++----
 1 file changed, 5 insertions(+), 4 deletions(-)

diff --git a/tcg/tcg.c b/tcg/tcg.c
index f49f1a7f35..43c6cf8f52 100644
--- a/tcg/tcg.c
+++ b/tcg/tcg.c
@@ -335,10 +335,11 @@ static bool tcg_resolve_relocs(TCGContext *s)
 
 static void set_jmp_reset_offset(TCGContext *s, int which)
 {
-    size_t off = tcg_current_code_size(s);
-    s->tb_jmp_reset_offset[which] = off;
-    /* Make sure that we didn't overflow the stored offset.  */
-    assert(s->tb_jmp_reset_offset[which] == off);
+    /*
+     * We will check for overflow at the end of the opcode loop in
+     * tcg_gen_code, where we bound tcg_current_code_size to UINT16_MAX.
+     */
+    s->tb_jmp_reset_offset[which] = tcg_current_code_size(s);
 }
 
 #include "tcg-target.c.inc"
-- 
2.25.1

RE: [PATCH for-5.2] tcg: Remove assert from set_jmp_reset_offset

Posted by Sai Pavan Boddu 3 years, 6 months ago

Hi Richard

> -----Original Message-----
> From: Richard Henderson <richard.henderson@linaro.org>
> Sent: Tuesday, November 3, 2020 9:10 AM
> To: qemu-devel@nongnu.org
> Cc: Sai Pavan Boddu <saipava@xilinx.com>
> Subject: [PATCH for-5.2] tcg: Remove assert from set_jmp_reset_offset
> 
> The range check done here is done later, more appropriately, at the end of
> tcg_gen_code.  There, a failing range check results in a returned error code,
> which causes the TB to be restarted at half the size.
> 
> Reported-by: Sai Pavan Boddu <saipava@xilinx.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> Sai, would you try this against your failing testcase?
[Sai Pavan Boddu] Thanks, this patch fixes the test. Thanks for the support.


Tested-by: Sai Pavan Boddu <sai.pavan.boddu@xilinx.com>


Regards,
Sai Pavan
> 
> 
> r~
> 
> ---
>  tcg/tcg.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index f49f1a7f35..43c6cf8f52 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -335,10 +335,11 @@ static bool tcg_resolve_relocs(TCGContext *s)
> 
>  static void set_jmp_reset_offset(TCGContext *s, int which)  {
> -    size_t off = tcg_current_code_size(s);
> -    s->tb_jmp_reset_offset[which] = off;
> -    /* Make sure that we didn't overflow the stored offset.  */
> -    assert(s->tb_jmp_reset_offset[which] == off);
> +    /*
> +     * We will check for overflow at the end of the opcode loop in
> +     * tcg_gen_code, where we bound tcg_current_code_size to UINT16_MAX.
> +     */
> +    s->tb_jmp_reset_offset[which] = tcg_current_code_size(s);
>  }
> 
>  #include "tcg-target.c.inc"
> --
> 2.25.1

Re: [PATCH for-5.2] tcg: Remove assert from set_jmp_reset_offset

Posted by Philippe Mathieu-Daudé 3 years, 6 months ago

On 11/3/20 4:39 AM, Richard Henderson wrote:
> The range check done here is done later, more appropriately,
> at the end of tcg_gen_code.

Maybe mention commit 6e6c4efed99 ("tcg: Restart after TB code generation
overflow")? (no need to repost).

>  There, a failing range check
> results in a returned error code, which causes the TB to be
> restarted at half the size.
> 
> Reported-by: Sai Pavan Boddu <saipava@xilinx.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
> 
> Sai, would you try this against your failing testcase?
> 
> 
> r~
> 
> ---
>  tcg/tcg.c | 9 +++++----
>  1 file changed, 5 insertions(+), 4 deletions(-)
> 
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index f49f1a7f35..43c6cf8f52 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -335,10 +335,11 @@ static bool tcg_resolve_relocs(TCGContext *s)
>  
>  static void set_jmp_reset_offset(TCGContext *s, int which)
>  {
> -    size_t off = tcg_current_code_size(s);
> -    s->tb_jmp_reset_offset[which] = off;
> -    /* Make sure that we didn't overflow the stored offset.  */
> -    assert(s->tb_jmp_reset_offset[which] == off);
> +    /*
> +     * We will check for overflow at the end of the opcode loop in
> +     * tcg_gen_code, where we bound tcg_current_code_size to UINT16_MAX.
> +     */
> +    s->tb_jmp_reset_offset[which] = tcg_current_code_size(s);
>  }
>  
>  #include "tcg-target.c.inc"
> 

Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>