diff mbox series

[2/2] tcg: Propagate new TCGOp to add_as_label_use

Message ID 20240910212351.977753-3-richard.henderson@linaro.org (mailing list archive)
State New, archived
Headers show
Series tcg: Fix branch/label link during plugin expansion | expand

Commit Message

Richard Henderson Sept. 10, 2024, 9:23 p.m. UTC 
The use of tcg_last_op does not interact well with
TCGContext.emit_before_op, resulting in the label
being linked to something other than the branch op.

In this case it is easier to simply collect the emitted
branch op and pass it directly to add_as_label_use.

Reported-by: Elisha Hollander <just4now666666@gmail.com>
Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
---
 tcg/tcg-op.c | 63 ++++++++++++++++++++++++++--------------------------
 1 file changed, 32 insertions(+), 31 deletions(-)

Comments

Pierrick Bouvier Sept. 10, 2024, 9:50 p.m. UTC | #1 
On 9/10/24 14:23, Richard Henderson wrote:
> The use of tcg_last_op does not interact well with
> TCGContext.emit_before_op, resulting in the label
> being linked to something other than the branch op.
> 
> In this case it is easier to simply collect the emitted
> branch op and pass it directly to add_as_label_use.
> 
> Reported-by: Elisha Hollander <just4now666666@gmail.com>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>   tcg/tcg-op.c | 63 ++++++++++++++++++++++++++--------------------------
>   1 file changed, 32 insertions(+), 31 deletions(-)
> 
> diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
> index 28c41b37a4..4a7e705367 100644
> --- a/tcg/tcg-op.c
> +++ b/tcg/tcg-op.c
> @@ -117,9 +117,9 @@ static void DNI tcg_gen_op1_i64(TCGOpcode opc, TCGv_i64 a1)
>       tcg_gen_op1(opc, tcgv_i64_arg(a1));
>   }
>   
> -static void DNI tcg_gen_op1i(TCGOpcode opc, TCGArg a1)
> +static TCGOp * DNI tcg_gen_op1i(TCGOpcode opc, TCGArg a1)
>   {
> -    tcg_gen_op1(opc, a1);
> +    return tcg_gen_op1(opc, a1);
>   }
>   
>   static void DNI tcg_gen_op2_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2)
> @@ -196,16 +196,16 @@ static void DNI tcg_gen_op4i_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2,
>                   tcgv_i64_arg(a3), a4);
>   }
>   
> -static void DNI tcg_gen_op4ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2,
> -                                  TCGArg a3, TCGArg a4)
> +static TCGOp * DNI tcg_gen_op4ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2,
> +                                     TCGArg a3, TCGArg a4)
>   {
> -    tcg_gen_op4(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), a3, a4);
> +    return tcg_gen_op4(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), a3, a4);
>   }
>   
> -static void DNI tcg_gen_op4ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2,
> -                                  TCGArg a3, TCGArg a4)
> +static TCGOp * DNI tcg_gen_op4ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2,
> +                                     TCGArg a3, TCGArg a4)
>   {
> -    tcg_gen_op4(opc, tcgv_i64_arg(a1), tcgv_i64_arg(a2), a3, a4);
> +    return tcg_gen_op4(opc, tcgv_i64_arg(a1), tcgv_i64_arg(a2), a3, a4);
>   }
>   
>   static void DNI tcg_gen_op5_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2,
> @@ -270,12 +270,12 @@ static void DNI tcg_gen_op6i_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2,
>                   tcgv_i64_arg(a3), tcgv_i64_arg(a4), tcgv_i64_arg(a5), a6);
>   }
>   
> -static void DNI tcg_gen_op6ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2,
> -                                  TCGv_i32 a3, TCGv_i32 a4,
> -                                  TCGArg a5, TCGArg a6)
> +static TCGOp * DNI tcg_gen_op6ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2,
> +                                     TCGv_i32 a3, TCGv_i32 a4,
> +                                     TCGArg a5, TCGArg a6)
>   {
> -    tcg_gen_op6(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2),
> -                tcgv_i32_arg(a3), tcgv_i32_arg(a4), a5, a6);
> +    return tcg_gen_op6(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2),
> +                       tcgv_i32_arg(a3), tcgv_i32_arg(a4), a5, a6);
>   }
>   
>   /* Generic ops.  */
> @@ -286,18 +286,17 @@ void gen_set_label(TCGLabel *l)
>       tcg_gen_op1(INDEX_op_set_label, label_arg(l));
>   }
>   
> -static void add_last_as_label_use(TCGLabel *l)
> +static void add_as_label_use(TCGLabel *l, TCGOp *op)
>   {
>       TCGLabelUse *u = tcg_malloc(sizeof(TCGLabelUse));
>   
> -    u->op = tcg_last_op();
> +    u->op = op;
>       QSIMPLEQ_INSERT_TAIL(&l->branches, u, next);
>   }
>   
>   void tcg_gen_br(TCGLabel *l)
>   {
> -    tcg_gen_op1(INDEX_op_br, label_arg(l));
> -    add_last_as_label_use(l);
> +    add_as_label_use(l, tcg_gen_op1(INDEX_op_br, label_arg(l)));
>   }
>   
>   void tcg_gen_mb(TCGBar mb_type)
> @@ -514,8 +513,9 @@ void tcg_gen_brcond_i32(TCGCond cond, TCGv_i32 arg1, TCGv_i32 arg2, TCGLabel *l)
>       if (cond == TCG_COND_ALWAYS) {
>           tcg_gen_br(l);
>       } else if (cond != TCG_COND_NEVER) {
> -        tcg_gen_op4ii_i32(INDEX_op_brcond_i32, arg1, arg2, cond, label_arg(l));
> -        add_last_as_label_use(l);
> +        TCGOp *op = tcg_gen_op4ii_i32(INDEX_op_brcond_i32,
> +                                      arg1, arg2, cond, label_arg(l));
> +        add_as_label_use(l, op);
>       }
>   }
>   
> @@ -1934,15 +1934,16 @@ void tcg_gen_brcond_i64(TCGCond cond, TCGv_i64 arg1, TCGv_i64 arg2, TCGLabel *l)
>       if (cond == TCG_COND_ALWAYS) {
>           tcg_gen_br(l);
>       } else if (cond != TCG_COND_NEVER) {
> +        TCGOp *op;
>           if (TCG_TARGET_REG_BITS == 32) {
> -            tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, TCGV_LOW(arg1),
> -                              TCGV_HIGH(arg1), TCGV_LOW(arg2),
> -                              TCGV_HIGH(arg2), cond, label_arg(l));
> +            op = tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, TCGV_LOW(arg1),
> +                                   TCGV_HIGH(arg1), TCGV_LOW(arg2),
> +                                   TCGV_HIGH(arg2), cond, label_arg(l));
>           } else {
> -            tcg_gen_op4ii_i64(INDEX_op_brcond_i64, arg1, arg2, cond,
> -                              label_arg(l));
> +            op = tcg_gen_op4ii_i64(INDEX_op_brcond_i64, arg1, arg2, cond,
> +                                   label_arg(l));
>           }
> -        add_last_as_label_use(l);
> +        add_as_label_use(l, op);
>       }
>   }
>   
> @@ -1953,12 +1954,12 @@ void tcg_gen_brcondi_i64(TCGCond cond, TCGv_i64 arg1, int64_t arg2, TCGLabel *l)
>       } else if (cond == TCG_COND_ALWAYS) {
>           tcg_gen_br(l);
>       } else if (cond != TCG_COND_NEVER) {
> -        tcg_gen_op6ii_i32(INDEX_op_brcond2_i32,
> -                          TCGV_LOW(arg1), TCGV_HIGH(arg1),
> -                          tcg_constant_i32(arg2),
> -                          tcg_constant_i32(arg2 >> 32),
> -                          cond, label_arg(l));
> -        add_last_as_label_use(l);
> +        TCGOp *op = tcg_gen_op6ii_i32(INDEX_op_brcond2_i32,
> +                                      TCGV_LOW(arg1), TCGV_HIGH(arg1),
> +                                      tcg_constant_i32(arg2),
> +                                      tcg_constant_i32(arg2 >> 32),
> +                                      cond, label_arg(l));
> +        add_as_label_use(l, op);
>       }
>   }
>   

the tcg_last_op() referred to in this case can be another op than the 
one expected?
Are there other cases where usage of tcg_last_op could be a problem?

Reviewed-by: Pierrick Bouvier <pierrick.bouvier@linaro.org>
Richard Henderson Sept. 10, 2024, 9:56 p.m. UTC | #2 
On 9/10/24 14:50, Pierrick Bouvier wrote:
> the tcg_last_op() referred to in this case can be another op than the one expected?
> Are there other cases where usage of tcg_last_op could be a problem?
I don't think so.  Aside from plugins, there are (currently) no other uses of emit_before_op.


r~
diff mbox series

Patch

diff --git a/tcg/tcg-op.c b/tcg/tcg-op.c
index 28c41b37a4..4a7e705367 100644
--- a/tcg/tcg-op.c
+++ b/tcg/tcg-op.c
@@ -117,9 +117,9 @@  static void DNI tcg_gen_op1_i64(TCGOpcode opc, TCGv_i64 a1)
     tcg_gen_op1(opc, tcgv_i64_arg(a1));
 }
 
-static void DNI tcg_gen_op1i(TCGOpcode opc, TCGArg a1)
+static TCGOp * DNI tcg_gen_op1i(TCGOpcode opc, TCGArg a1)
 {
-    tcg_gen_op1(opc, a1);
+    return tcg_gen_op1(opc, a1);
 }
 
 static void DNI tcg_gen_op2_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2)
@@ -196,16 +196,16 @@  static void DNI tcg_gen_op4i_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2,
                 tcgv_i64_arg(a3), a4);
 }
 
-static void DNI tcg_gen_op4ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2,
-                                  TCGArg a3, TCGArg a4)
+static TCGOp * DNI tcg_gen_op4ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2,
+                                     TCGArg a3, TCGArg a4)
 {
-    tcg_gen_op4(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), a3, a4);
+    return tcg_gen_op4(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2), a3, a4);
 }
 
-static void DNI tcg_gen_op4ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2,
-                                  TCGArg a3, TCGArg a4)
+static TCGOp * DNI tcg_gen_op4ii_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2,
+                                     TCGArg a3, TCGArg a4)
 {
-    tcg_gen_op4(opc, tcgv_i64_arg(a1), tcgv_i64_arg(a2), a3, a4);
+    return tcg_gen_op4(opc, tcgv_i64_arg(a1), tcgv_i64_arg(a2), a3, a4);
 }
 
 static void DNI tcg_gen_op5_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2,
@@ -270,12 +270,12 @@  static void DNI tcg_gen_op6i_i64(TCGOpcode opc, TCGv_i64 a1, TCGv_i64 a2,
                 tcgv_i64_arg(a3), tcgv_i64_arg(a4), tcgv_i64_arg(a5), a6);
 }
 
-static void DNI tcg_gen_op6ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2,
-                                  TCGv_i32 a3, TCGv_i32 a4,
-                                  TCGArg a5, TCGArg a6)
+static TCGOp * DNI tcg_gen_op6ii_i32(TCGOpcode opc, TCGv_i32 a1, TCGv_i32 a2,
+                                     TCGv_i32 a3, TCGv_i32 a4,
+                                     TCGArg a5, TCGArg a6)
 {
-    tcg_gen_op6(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2),
-                tcgv_i32_arg(a3), tcgv_i32_arg(a4), a5, a6);
+    return tcg_gen_op6(opc, tcgv_i32_arg(a1), tcgv_i32_arg(a2),
+                       tcgv_i32_arg(a3), tcgv_i32_arg(a4), a5, a6);
 }
 
 /* Generic ops.  */
@@ -286,18 +286,17 @@  void gen_set_label(TCGLabel *l)
     tcg_gen_op1(INDEX_op_set_label, label_arg(l));
 }
 
-static void add_last_as_label_use(TCGLabel *l)
+static void add_as_label_use(TCGLabel *l, TCGOp *op)
 {
     TCGLabelUse *u = tcg_malloc(sizeof(TCGLabelUse));
 
-    u->op = tcg_last_op();
+    u->op = op;
     QSIMPLEQ_INSERT_TAIL(&l->branches, u, next);
 }
 
 void tcg_gen_br(TCGLabel *l)
 {
-    tcg_gen_op1(INDEX_op_br, label_arg(l));
-    add_last_as_label_use(l);
+    add_as_label_use(l, tcg_gen_op1(INDEX_op_br, label_arg(l)));
 }
 
 void tcg_gen_mb(TCGBar mb_type)
@@ -514,8 +513,9 @@  void tcg_gen_brcond_i32(TCGCond cond, TCGv_i32 arg1, TCGv_i32 arg2, TCGLabel *l)
     if (cond == TCG_COND_ALWAYS) {
         tcg_gen_br(l);
     } else if (cond != TCG_COND_NEVER) {
-        tcg_gen_op4ii_i32(INDEX_op_brcond_i32, arg1, arg2, cond, label_arg(l));
-        add_last_as_label_use(l);
+        TCGOp *op = tcg_gen_op4ii_i32(INDEX_op_brcond_i32,
+                                      arg1, arg2, cond, label_arg(l));
+        add_as_label_use(l, op);
     }
 }
 
@@ -1934,15 +1934,16 @@  void tcg_gen_brcond_i64(TCGCond cond, TCGv_i64 arg1, TCGv_i64 arg2, TCGLabel *l)
     if (cond == TCG_COND_ALWAYS) {
         tcg_gen_br(l);
     } else if (cond != TCG_COND_NEVER) {
+        TCGOp *op;
         if (TCG_TARGET_REG_BITS == 32) {
-            tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, TCGV_LOW(arg1),
-                              TCGV_HIGH(arg1), TCGV_LOW(arg2),
-                              TCGV_HIGH(arg2), cond, label_arg(l));
+            op = tcg_gen_op6ii_i32(INDEX_op_brcond2_i32, TCGV_LOW(arg1),
+                                   TCGV_HIGH(arg1), TCGV_LOW(arg2),
+                                   TCGV_HIGH(arg2), cond, label_arg(l));
         } else {
-            tcg_gen_op4ii_i64(INDEX_op_brcond_i64, arg1, arg2, cond,
-                              label_arg(l));
+            op = tcg_gen_op4ii_i64(INDEX_op_brcond_i64, arg1, arg2, cond,
+                                   label_arg(l));
         }
-        add_last_as_label_use(l);
+        add_as_label_use(l, op);
     }
 }
 
@@ -1953,12 +1954,12 @@  void tcg_gen_brcondi_i64(TCGCond cond, TCGv_i64 arg1, int64_t arg2, TCGLabel *l)
     } else if (cond == TCG_COND_ALWAYS) {
         tcg_gen_br(l);
     } else if (cond != TCG_COND_NEVER) {
-        tcg_gen_op6ii_i32(INDEX_op_brcond2_i32,
-                          TCGV_LOW(arg1), TCGV_HIGH(arg1),
-                          tcg_constant_i32(arg2),
-                          tcg_constant_i32(arg2 >> 32),
-                          cond, label_arg(l));
-        add_last_as_label_use(l);
+        TCGOp *op = tcg_gen_op6ii_i32(INDEX_op_brcond2_i32,
+                                      TCGV_LOW(arg1), TCGV_HIGH(arg1),
+                                      tcg_constant_i32(arg2),
+                                      tcg_constant_i32(arg2 >> 32),
+                                      cond, label_arg(l));
+        add_as_label_use(l, op);
     }
 }