Support for i128 tcg temp (#51) + add -Werror and fix compilation warnings

* Added testcase and binary for i128.
* Add symbolic handling for i128.
* Enable werror and tcg debug asserts, fix compilation warnings.
* Fix casting error appearing with GCC 13. (to reproduce, compile QEMU with gcc 13 using --cc).

Author: rmalmain

Dockerfile

@@ -35,7 +35,9 @@ RUN ./configure                                                       \
           --target-list=x86_64-linux-user,riscv64-linux-user          \ 
           --enable-debug                                              \
           --symcc-source=/symcc                                       \
-          --symcc-build=/symcc/build
+          --symcc-build=/symcc/build                                  \
+          --enable-debug-tcg                                          \
+          --enable-werror
 
 RUN make -j
 

accel/tcg/tcg-runtime-sym-vec.c

@@ -387,7 +387,7 @@ void *HELPER(sym_cmp_vec)(
         CPUArchState *env,
         void *arg1_concrete, void *arg1_symbolic,
         void *arg2_concrete, void *arg2_symbolic,
-        TCGCond comparison_operator, void *result_concrete,
+        /* TCGCond */ uint32_t comparison_operator, void *result_concrete,
         uint64_t vector_size, uint64_t vece
 ) {
     g_assert(vector_size == 64 || vector_size == 128 || vector_size == 256);
@@ -468,7 +468,7 @@ void *HELPER(sym_ternary_vec)(
         CPUArchState *env,
         void *arg1_concrete, void *arg1_symbolic,
         void *arg2_concrete, void *arg2_symbolic,
-        TCGCond comparison_operator, void *concrete_result,
+        /* TCGCond */ uint32_t comparison_operator, void *concrete_result,
         uint64_t vector_size, uint64_t vece
 ) {
     g_assert(vector_size == 64 || vector_size == 128 || vector_size == 256);

include/tcg/tcg.h

@@ -416,6 +416,8 @@ typedef struct TCGTemp {
     unsigned int temp_allocated:1;
     unsigned int temp_subindex:1;
 
+    /* Maximum len of subindex. Must be the same size as temp_subindex */
+    unsigned int temp_subindex_len:1;
     /* If true, this temp contains a symbolic expression. */
     unsigned int symbolic_expression:1;
 
@@ -632,10 +634,15 @@ static inline TCGTemp *tcgv_i32_temp(TCGv_i32 v)
 #endif
 
 static inline TCGTemp *temp_expr(TCGTemp *ts) {
-    tcg_debug_assert(temp_idx(ts) + 1 < tcg_ctx->nb_temps);
+    // Length of subindex. For now, it can only be 0 or 1 (for i128)
+    // Could become bigger in the future.
+    unsigned char temp_sz = ts->temp_subindex_len;
+
+    tcg_debug_assert(temp_idx(ts) + temp_sz + 1 < tcg_ctx->nb_temps);
     tcg_debug_assert(!ts->symbolic_expression);
-    tcg_debug_assert((ts + 1)->symbolic_expression);
-    return ts + 1;
+    tcg_debug_assert((ts + temp_sz + 1)->symbolic_expression);
+
+    return ts + temp_sz + 1;
 }
 
 static inline TCGTemp *tcgv_i64_temp(TCGv_i64 v)

tcg/tcg-op-ldst.c

@@ -453,31 +453,31 @@ void tcg_gen_qemu_st_i64_chk(TCGv_i64 val, TCGTemp *addr, TCGArg idx,
  * does not require 16-byte atomicity, and it would be adventagous
  * to avoid a call to a helper function.
  */
-static bool use_two_i64_for_i128(MemOp mop)
-{
-#ifdef CONFIG_SOFTMMU
-    /* Two softmmu tlb lookups is larger than one function call. */
-    return false;
-#else
-    /*
-     * For user-only, two 64-bit operations may well be smaller than a call.
-     * Determine if that would be legal for the requested atomicity.
-     */
-    switch (mop & MO_ATOM_MASK) {
-    case MO_ATOM_NONE:
-    case MO_ATOM_IFALIGN_PAIR:
-        return true;
-    case MO_ATOM_IFALIGN:
-    case MO_ATOM_SUBALIGN:
-    case MO_ATOM_WITHIN16:
-    case MO_ATOM_WITHIN16_PAIR:
-        /* In a serialized context, no atomicity is required. */
-        return !(tcg_ctx->gen_tb->cflags & CF_PARALLEL);
-    default:
-        g_assert_not_reached();
-    }
-#endif
-}
+// static bool use_two_i64_for_i128(MemOp mop)
+// {
+// #ifdef CONFIG_SOFTMMU
+//     /* Two softmmu tlb lookups is larger than one function call. */
+//     return false;
+// #else
+//     /*
+//      * For user-only, two 64-bit operations may well be smaller than a call.
+//      * Determine if that would be legal for the requested atomicity.
+//      */
+//     switch (mop & MO_ATOM_MASK) {
+//     case MO_ATOM_NONE:
+//     case MO_ATOM_IFALIGN_PAIR:
+//         return true;
+//     case MO_ATOM_IFALIGN:
+//     case MO_ATOM_SUBALIGN:
+//     case MO_ATOM_WITHIN16:
+//     case MO_ATOM_WITHIN16_PAIR:
+//         /* In a serialized context, no atomicity is required. */
+//         return !(tcg_ctx->gen_tb->cflags & CF_PARALLEL);
+//     default:
+//         g_assert_not_reached();
+//     }
+// #endif
+// }
 
 static void canonicalize_memop_i128_as_i64(MemOp ret[2], MemOp orig)
 {
@@ -551,7 +551,7 @@ static void tcg_gen_qemu_ld_i128_int(TCGv_i128 val, TCGTemp *addr,
     tcg_gen_req_mo(TCG_MO_LD_LD | TCG_MO_ST_LD);
 
     /* TODO: For now, force 32-bit hosts to use the helper. */
-    if (TCG_TARGET_HAS_qemu_ldst_i128 && TCG_TARGET_REG_BITS == 64) {
+    /* if (TCG_TARGET_HAS_qemu_ldst_i128 && TCG_TARGET_REG_BITS == 64) {
         TCGv_i64 lo, hi;
         bool need_bswap = false;
         MemOpIdx oi = orig_oi;
@@ -577,11 +577,13 @@ static void tcg_gen_qemu_ld_i128_int(TCGv_i128 val, TCGTemp *addr,
             tcg_gen_bswap64_i64(lo, lo);
             tcg_gen_bswap64_i64(hi, hi);
         }
-    } else if (use_two_i64_for_i128(memop)) {
+    } else if (use_two_i64_for_i128(memop)) { */
+        // Always take this path to get symbolic propagation
         MemOp mop[2];
         TCGTemp *addr_p8;
         TCGv_i64 x, y;
         bool need_bswap;
+        TCGv_i64 load_size, mmu_idx;
 
         canonicalize_memop_i128_as_i64(mop, memop);
         need_bswap = (mop[0] ^ memop) & MO_BSWAP;
@@ -607,6 +609,14 @@ static void tcg_gen_qemu_ld_i128_int(TCGv_i128 val, TCGTemp *addr,
 
         gen_ldst_i64(opc, x, addr, make_memop_idx(mop[0], idx));
 
+        /* Perform the symbolic memory access. Doing so _after_ the concrete
+        * operation ensures that the target address is in the TLB. */
+        mmu_idx = tcg_constant_i64(idx);
+        load_size = tcg_constant_i64(1 << MO_64);
+        gen_helper_sym_load_guest_i64(tcgv_i64_expr(x), cpu_env,
+                                      temp_tcgv_i64(addr), tcgv_i64_expr(temp_tcgv_i64(addr)),
+                                      load_size, mmu_idx);
+
         if (need_bswap) {
             tcg_gen_bswap64_i64(x, x);
         }
@@ -622,20 +632,27 @@ static void tcg_gen_qemu_ld_i128_int(TCGv_i128 val, TCGTemp *addr,
         }
 
         gen_ldst_i64(opc, y, addr_p8, make_memop_idx(mop[1], idx));
+
+        /* Perform the symbolic memory access. Doing so _after_ the concrete
+        * operation ensures that the target address is in the TLB. */
+        gen_helper_sym_load_guest_i64(tcgv_i64_expr(y), cpu_env,
+                                      temp_tcgv_i64(addr_p8), tcgv_i64_expr(temp_tcgv_i64(addr_p8)),
+                                      load_size, mmu_idx);
+
         tcg_temp_free_internal(addr_p8);
 
         if (need_bswap) {
             tcg_gen_bswap64_i64(y, y);
         }
-    } else {
+    /* } else {
         if (tcg_ctx->addr_type == TCG_TYPE_I32) {
             ext_addr = tcg_temp_ebb_new_i64();
             tcg_gen_extu_i32_i64(ext_addr, temp_tcgv_i32(addr));
             addr = tcgv_i64_temp(ext_addr);
         }
         gen_helper_ld_i128(val, cpu_env, temp_tcgv_i64(addr),
                            tcg_constant_i32(orig_oi));
-    }
+    } */
 
     plugin_gen_mem_callbacks(ext_addr, addr, orig_oi, QEMU_PLUGIN_MEM_R);
 }
@@ -661,7 +678,7 @@ static void tcg_gen_qemu_st_i128_int(TCGv_i128 val, TCGTemp *addr,
 
     /* TODO: For now, force 32-bit hosts to use the helper. */
 
-    if (TCG_TARGET_HAS_qemu_ldst_i128 && TCG_TARGET_REG_BITS == 64) {
+    /* if (TCG_TARGET_HAS_qemu_ldst_i128 && TCG_TARGET_REG_BITS == 64) {
         TCGv_i64 lo, hi;
         MemOpIdx oi = orig_oi;
         bool need_bswap = false;
@@ -689,10 +706,12 @@ static void tcg_gen_qemu_st_i128_int(TCGv_i128 val, TCGTemp *addr,
             tcg_temp_free_i64(lo);
             tcg_temp_free_i64(hi);
         }
-    } else if (use_two_i64_for_i128(memop)) {
+    } else if (use_two_i64_for_i128(memop)) { */
+        // Always take this path to get symbolic propagation
         MemOp mop[2];
         TCGTemp *addr_p8;
         TCGv_i64 x, y, b = NULL;
+        TCGv_i64 store_size, mmu_idx;
 
         canonicalize_memop_i128_as_i64(mop, memop);
 
@@ -718,6 +737,15 @@ static void tcg_gen_qemu_st_i128_int(TCGv_i128 val, TCGTemp *addr,
 
         gen_ldst_i64(opc, x, addr, make_memop_idx(mop[0], idx));
 
+        /* Perform the symbolic memory access. Doing so _after_ the concrete
+        * operation ensures that the target address is in the TLB. */
+        mmu_idx = tcg_constant_i64(idx);
+        store_size = tcg_constant_i64(1 << MO_64);
+        gen_helper_sym_store_guest_i64(cpu_env,
+                                       x, tcgv_i64_expr(x),
+                                       temp_tcgv_i64(addr), tcgv_i64_expr(temp_tcgv_i64(addr)),
+                                       store_size, mmu_idx);
+
         if (tcg_ctx->addr_type == TCG_TYPE_I32) {
             TCGv_i32 t = tcg_temp_ebb_new_i32();
             tcg_gen_addi_i32(t, temp_tcgv_i32(addr), 8);
@@ -731,20 +759,35 @@ static void tcg_gen_qemu_st_i128_int(TCGv_i128 val, TCGTemp *addr,
         if (b) {
             tcg_gen_bswap64_i64(b, y);
             gen_ldst_i64(opc, b, addr_p8, make_memop_idx(mop[1], idx));
+
+            /* Perform the symbolic memory access. Doing so _after_ the concrete
+            * operation ensures that the target address is in the TLB. */
+            gen_helper_sym_store_guest_i64(cpu_env,
+                                           b, tcgv_i64_expr(b),
+                                           temp_tcgv_i64(addr_p8), tcgv_i64_expr(temp_tcgv_i64(addr_p8)),
+                                           store_size, mmu_idx);
+
             tcg_temp_free_i64(b);
         } else {
             gen_ldst_i64(opc, y, addr_p8, make_memop_idx(mop[1], idx));
+
+            /* Perform the symbolic memory access. Doing so _after_ the concrete
+            * operation ensures that the target address is in the TLB. */
+            gen_helper_sym_store_guest_i64(cpu_env,
+                                           y, tcgv_i64_expr(y),
+                                           temp_tcgv_i64(addr_p8), tcgv_i64_expr(temp_tcgv_i64(addr_p8)),
+                                           store_size, mmu_idx);
         }
         tcg_temp_free_internal(addr_p8);
-    } else {
+    /* } else {
         if (tcg_ctx->addr_type == TCG_TYPE_I32) {
             ext_addr = tcg_temp_ebb_new_i64();
             tcg_gen_extu_i32_i64(ext_addr, temp_tcgv_i32(addr));
             addr = tcgv_i64_temp(ext_addr);
         }
         gen_helper_st_i128(cpu_env, temp_tcgv_i64(addr), val,
                            tcg_constant_i32(orig_oi));
-    }
+    } */
 
     plugin_gen_mem_callbacks(ext_addr, addr, orig_oi, QEMU_PLUGIN_MEM_W);
 }

tcg/tcg-op-vec.c

@@ -279,7 +279,7 @@ void vec_gen_4(TCGOpcode opc, TCGType type, unsigned vece,
     op->args[3] = c;
 }
 
-static void vec_gen_6(TCGOpcode opc, TCGType type, unsigned vece, TCGArg r,
+/* static void vec_gen_6(TCGOpcode opc, TCGType type, unsigned vece, TCGArg r,
                       TCGArg a, TCGArg b, TCGArg c, TCGArg d, TCGArg e)
 {
     TCGOp *op = tcg_emit_op(opc, 6);
@@ -291,7 +291,7 @@ static void vec_gen_6(TCGOpcode opc, TCGType type, unsigned vece, TCGArg r,
     op->args[3] = c;
     op->args[4] = d;
     op->args[5] = e;
-}
+} */
 
 static void vec_gen_op2(TCGOpcode opc, unsigned vece, TCGv_vec r, TCGv_vec a)
 {
@@ -562,7 +562,7 @@ void tcg_gen_eqv_vec(unsigned vece, TCGv_vec r, TCGv_vec a, TCGv_vec b)
     /* } */
 }
 
-static bool do_op2(unsigned vece, TCGv_vec r, TCGv_vec a, TCGOpcode opc)
+/* static bool do_op2(unsigned vece, TCGv_vec r, TCGv_vec a, TCGOpcode opc)
 {
     TCGTemp *rt = tcgv_vec_temp(r);
     TCGTemp *at = tcgv_vec_temp(a);
@@ -584,7 +584,7 @@ static bool do_op2(unsigned vece, TCGv_vec r, TCGv_vec a, TCGOpcode opc)
         return false;
     }
     return true;
-}
+} */
 
 void tcg_gen_not_vec(unsigned vece, TCGv_vec r, TCGv_vec a)
 {
@@ -1067,14 +1067,14 @@ void tcg_gen_cmpsel_vec(TCGCond cond, unsigned vece, TCGv_vec r,
     TCGTemp *bt = tcgv_vec_temp(b);
     TCGTemp *ct = tcgv_vec_temp(c);
     TCGTemp *dt = tcgv_vec_temp(d);
-    TCGArg ri = temp_arg(rt);
+    /* TCGArg ri = temp_arg(rt);
     TCGArg ai = temp_arg(at);
     TCGArg bi = temp_arg(bt);
     TCGArg ci = temp_arg(ct);
-    TCGArg di = temp_arg(dt);
+    TCGArg di = temp_arg(dt); */
     TCGType type = rt->base_type;
     const TCGOpcode *hold_list;
-    int can;
+    /* int can; */
 
     tcg_debug_assert(at->base_type >= type);
     tcg_debug_assert(bt->base_type >= type);
@@ -1083,7 +1083,7 @@ void tcg_gen_cmpsel_vec(TCGCond cond, unsigned vece, TCGv_vec r,
 
     tcg_assert_listed_vecop(INDEX_op_cmpsel_vec);
     hold_list = tcg_swap_vecop_list(NULL);
-    can = tcg_can_emit_vec_op(INDEX_op_cmpsel_vec, type, vece);
+    /* can = tcg_can_emit_vec_op(INDEX_op_cmpsel_vec, type, vece); */
 
     /*
     TODO (SymQEMU):

tcg/tcg.c

@@ -1509,6 +1509,7 @@ void tcg_func_start(TCGContext *s)
 
 static TCGTemp *tcg_temp_alloc(TCGContext *s)
 {
+    /* Not necessarily true with i128
     int last_temp_idx = s->nb_temps - 1;
     if (last_temp_idx > 0) {
         if (last_temp_idx % 2 == 0) {
@@ -1517,6 +1518,7 @@ static TCGTemp *tcg_temp_alloc(TCGContext *s)
             g_assert((s->temps[last_temp_idx].symbolic_expression));
         }
     }
+    */
 
     int n = s->nb_temps++;
 
@@ -1711,13 +1713,13 @@ TCGTemp *tcg_temp_new_internal(TCGType type, TCGTempKind kind)
     ts->temp_allocated = 1;
     ts->kind = kind;
 
+    // To change if subindex can take bigger values
+    tcg_debug_assert(n >= 0 && n - 1 <= 1);
+    ts->temp_subindex_len = n - 1;
+
     if (n == 1) {
         ts->type = type;
     } else {
-        /* The current implementation of SymQEMU does not support this case
-         * as the symbolic version of a TCGTemp is expected to be located right after the concrete version */
-        g_assert_not_reached();
-
         ts->type = TCG_TYPE_REG;
 
         for (int i = 1; i < n; ++i) {
@@ -1728,6 +1730,7 @@ TCGTemp *tcg_temp_new_internal(TCGType type, TCGTempKind kind)
             ts2->type = TCG_TYPE_REG;
             ts2->temp_allocated = 1;
             ts2->temp_subindex = i;
+            ts2->temp_subindex_len = n - 1;
             ts2->kind = kind;
         }
     }
@@ -1736,9 +1739,29 @@ TCGTemp *tcg_temp_new_internal(TCGType type, TCGTempKind kind)
     ts_expr->base_type = TCG_TYPE_PTR;
     ts_expr->type = TCG_TYPE_PTR;
     ts_expr->temp_allocated = 1;
+    ts_expr->temp_subindex = 0;
     ts_expr->kind = kind;
     ts_expr->symbolic_expression = 1;
 
+    // We allocate multiple symbolic expressions if n > 1 since TCG's i128 are
+    // split in 64 bits blocks for now, so it is more convenient.
+    // It could be more efficient to have 1 symbolic expression if later TCG's i128 are
+    // handled as a single block.
+    if (n > 1) {
+        for (int i = 1; i < n; ++i) {
+            TCGTemp *ts2_expr = tcg_temp_alloc(s);
+
+            tcg_debug_assert(ts2_expr == ts_expr + i);
+            tcg_debug_assert(ts2_expr == ts + n + i);
+            ts2_expr->base_type = TCG_TYPE_PTR;
+            ts2_expr->type = TCG_TYPE_PTR;
+            ts2_expr->temp_allocated = 1;
+            ts2_expr->temp_subindex = i;
+            ts2_expr->kind = kind;
+            ts2_expr->symbolic_expression = 1;
+        }
+    }
+
     return ts;
 }