Re: [PATCH v2 11/39] tcg: Pass the locked filepointer to tcg_dump_ops

[Alex Bennée]

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

> We have already looked up and locked the filepointer.
> Use fprintf instead of qemu_log directly for output
> in and around tcg_dump_ops.
>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  tcg/tcg.c | 109 ++++++++++++++++++++++++++----------------------------
>  1 file changed, 52 insertions(+), 57 deletions(-)
>
> diff --git a/tcg/tcg.c b/tcg/tcg.c
> index 892f640fce..25e987d881 100644
> --- a/tcg/tcg.c
> +++ b/tcg/tcg.c
> @@ -1808,7 +1808,11 @@ static inline TCGReg tcg_regset_first(TCGRegSet d)
>      }
>  }
>  
> -static void tcg_dump_ops(TCGContext *s, bool have_prefs)
> +/* Return only the number of characters output -- no error return. */
> +#define ne_fprintf(...) \
> +    ({ int ret_ = fprintf(__VA_ARGS__); ret_ >= 0 ? ret_ : 0; })
> +
> +static void tcg_dump_ops(TCGContext *s, FILE *f, bool have_prefs)
>  {
>      char buf[128];
>      TCGOp *op;
> @@ -1824,7 +1828,7 @@ static void tcg_dump_ops(TCGContext *s, bool have_prefs)
>  
>          if (c == INDEX_op_insn_start) {
>              nb_oargs = 0;
> -            col += qemu_log("\n ----");
> +            col += ne_fprintf(f, "\n ----");
>  
>              for (i = 0; i < TARGET_INSN_START_WORDS; ++i) {
>                  target_ulong a;
> @@ -1833,7 +1837,7 @@ static void tcg_dump_ops(TCGContext *s, bool have_prefs)
>  #else
>                  a = op->args[i];
>  #endif
> -                col += qemu_log(" " TARGET_FMT_lx, a);
> +                col += ne_fprintf(f, " " TARGET_FMT_lx, a);
>              }
>          } else if (c == INDEX_op_call) {
>              const TCGHelperInfo *info = tcg_call_info(op);
> @@ -1844,7 +1848,7 @@ static void tcg_dump_ops(TCGContext *s, bool have_prefs)
>              nb_iargs = TCGOP_CALLI(op);
>              nb_cargs = def->nb_cargs;
>  
> -            col += qemu_log(" %s ", def->name);
> +            col += ne_fprintf(f, " %s ", def->name);
>  
>              /*
>               * Print the function name from TCGHelperInfo, if available.
> @@ -1852,15 +1856,15 @@ static void tcg_dump_ops(TCGContext *s, bool 
> have_prefs)
>               * but the actual function pointer comes from the plugin.
>               */
>              if (func == info->func) {
> -                col += qemu_log("%s", info->name);
> +                col += ne_fprintf(f, "%s", info->name);
>              } else {
> -                col += qemu_log("plugin(%p)", func);
> +                col += ne_fprintf(f, "plugin(%p)", func);
>              }
>  
> -            col += qemu_log(",$0x%x,$%d", info->flags, nb_oargs);
> +            col += ne_fprintf(f, ",$0x%x,$%d", info->flags, nb_oargs);
>              for (i = 0; i < nb_oargs; i++) {
> -                col += qemu_log(",%s", tcg_get_arg_str(s, buf, sizeof(buf),
> -                                                       op->args[i]));
> +                col += ne_fprintf(f, ",%s", tcg_get_arg_str(s, buf, 
> sizeof(buf),
> +                                                            op->args[i]));
>              }
>              for (i = 0; i < nb_iargs; i++) {
>                  TCGArg arg = op->args[nb_oargs + i];
> @@ -1868,34 +1872,32 @@ static void tcg_dump_ops(TCGContext *s, bool 
> have_prefs)
>                  if (arg != TCG_CALL_DUMMY_ARG) {
>                      t = tcg_get_arg_str(s, buf, sizeof(buf), arg);
>                  }
> -                col += qemu_log(",%s", t);
> +                col += ne_fprintf(f, ",%s", t);
>              }
>          } else {
> -            col += qemu_log(" %s ", def->name);
> +            col += ne_fprintf(f, " %s ", def->name);
>  
>              nb_oargs = def->nb_oargs;
>              nb_iargs = def->nb_iargs;
>              nb_cargs = def->nb_cargs;
>  
>              if (def->flags & TCG_OPF_VECTOR) {
> -                col += qemu_log("v%d,e%d,", 64 << TCGOP_VECL(op),
> -                                8 << TCGOP_VECE(op));
> +                col += ne_fprintf(f, "v%d,e%d,", 64 << TCGOP_VECL(op),
> +                                  8 << TCGOP_VECE(op));
>              }
>  
>              k = 0;
>              for (i = 0; i < nb_oargs; i++) {
> -                if (k != 0) {
> -                    col += qemu_log(",");
> -                }
> -                col += qemu_log("%s", tcg_get_arg_str(s, buf, sizeof(buf),
> -                                                      op->args[k++]));
> +                const char *sep =  k ? "," : "";
> +                col += ne_fprintf(f, "%s%s", sep,
> +                                  tcg_get_arg_str(s, buf, sizeof(buf),
> +                                                  op->args[k++]));
>              }
>              for (i = 0; i < nb_iargs; i++) {
> -                if (k != 0) {
> -                    col += qemu_log(",");
> -                }
> -                col += qemu_log("%s", tcg_get_arg_str(s, buf, sizeof(buf),
> -                                                      op->args[k++]));
> +                const char *sep =  k ? "," : "";
> +                col += ne_fprintf(f, "%s%s", sep,
> +                                  tcg_get_arg_str(s, buf, sizeof(buf),
> +                                                  op->args[k++]));
>              }
>              switch (c) {
>              case INDEX_op_brcond_i32:
> @@ -1910,9 +1912,9 @@ static void tcg_dump_ops(TCGContext *s, bool have_prefs)
>              case INDEX_op_cmpsel_vec:
>                  if (op->args[k] < ARRAY_SIZE(cond_name)
>                      && cond_name[op->args[k]]) {
> -                    col += qemu_log(",%s", cond_name[op->args[k++]]);
> +                    col += ne_fprintf(f, ",%s", cond_name[op->args[k++]]);
>                  } else {
> -                    col += qemu_log(",$0x%" TCG_PRIlx, op->args[k++]);
> +                    col += ne_fprintf(f, ",$0x%" TCG_PRIlx, op->args[k++]);
>                  }
>                  i = 1;
>                  break;
> @@ -1927,12 +1929,12 @@ static void tcg_dump_ops(TCGContext *s, bool 
> have_prefs)
>                      unsigned ix = get_mmuidx(oi);
>  
>                      if (op & ~(MO_AMASK | MO_BSWAP | MO_SSIZE)) {
> -                        col += qemu_log(",$0x%x,%u", op, ix);
> +                        col += ne_fprintf(f, ",$0x%x,%u", op, ix);
>                      } else {
>                          const char *s_al, *s_op;
>                          s_al = alignment_name[(op & MO_AMASK) >> MO_ASHIFT];
>                          s_op = ldst_name[op & (MO_BSWAP | MO_SSIZE)];
> -                        col += qemu_log(",%s%s,%u", s_al, s_op, ix);
> +                        col += ne_fprintf(f, ",%s%s,%u", s_al, s_op, ix);
>                      }
>                      i = 1;
>                  }
> @@ -1950,9 +1952,9 @@ static void tcg_dump_ops(TCGContext *s, bool have_prefs)
>                          name = bswap_flag_name[flags];
>                      }
>                      if (name) {
> -                        col += qemu_log(",%s", name);
> +                        col += ne_fprintf(f, ",%s", name);
>                      } else {
> -                        col += qemu_log(",$0x%" TCG_PRIlx, flags);
> +                        col += ne_fprintf(f, ",$0x%" TCG_PRIlx, flags);
>                      }
>                      i = k = 1;
>                  }
> @@ -1967,49 +1969,42 @@ static void tcg_dump_ops(TCGContext *s, bool 
> have_prefs)
>              case INDEX_op_brcond_i32:
>              case INDEX_op_brcond_i64:
>              case INDEX_op_brcond2_i32:
> -                col += qemu_log("%s$L%d", k ? "," : "",
> -                                arg_label(op->args[k])->id);
> +                col += ne_fprintf(f, "%s$L%d", k ? "," : "",
> +                                  arg_label(op->args[k])->id);
>                  i++, k++;
>                  break;
>              default:
>                  break;
>              }
>              for (; i < nb_cargs; i++, k++) {
> -                col += qemu_log("%s$0x%" TCG_PRIlx, k ? "," : "", 
> op->args[k]);
> +                col += ne_fprintf(f, "%s$0x%" TCG_PRIlx, k ? "," : "",
> +                                  op->args[k]);
>              }
>          }
>  
>          if (have_prefs || op->life) {
> -
> -            QemuLogFile *logfile;
> -
> -            rcu_read_lock();
> -            logfile = qatomic_rcu_read(&qemu_logfile);
> -            if (logfile) {
> -                for (; col < 40; ++col) {
> -                    putc(' ', logfile->fd);
> -                }
> +            for (; col < 40; ++col) {
> +                putc(' ', f);
>              }
> -            rcu_read_unlock();
>          }
>  
>          if (op->life) {
>              unsigned life = op->life;
>  
>              if (life & (SYNC_ARG * 3)) {
> -                qemu_log("  sync:");
> +                ne_fprintf(f, "  sync:");
>                  for (i = 0; i < 2; ++i) {
>                      if (life & (SYNC_ARG << i)) {
> -                        qemu_log(" %d", i);
> +                        ne_fprintf(f, " %d", i);
>                      }
>                  }
>              }
>              life /= DEAD_ARG;
>              if (life) {
> -                qemu_log("  dead:");
> +                ne_fprintf(f, "  dead:");
>                  for (i = 0; life; ++i, life >>= 1) {
>                      if (life & 1) {
> -                        qemu_log(" %d", i);
> +                        ne_fprintf(f, " %d", i);
>                      }
>                  }
>              }
> @@ -2020,28 +2015,28 @@ static void tcg_dump_ops(TCGContext *s, bool 
> have_prefs)
>                  TCGRegSet set = op->output_pref[i];
>  
>                  if (i == 0) {
> -                    qemu_log("  pref=");
> +                    ne_fprintf(f, "  pref=");
>                  } else {
> -                    qemu_log(",");
> +                    ne_fprintf(f, ",");
>                  }
>                  if (set == 0) {
> -                    qemu_log("none");
> +                    ne_fprintf(f, "none");
>                  } else if (set == MAKE_64BIT_MASK(0, TCG_TARGET_NB_REGS)) {
> -                    qemu_log("all");
> +                    ne_fprintf(f, "all");
>  #ifdef CONFIG_DEBUG_TCG
>                  } else if (tcg_regset_single(set)) {
>                      TCGReg reg = tcg_regset_first(set);
> -                    qemu_log("%s", tcg_target_reg_names[reg]);
> +                    ne_fprintf(f, "%s", tcg_target_reg_names[reg]);
>  #endif
>                  } else if (TCG_TARGET_NB_REGS <= 32) {
> -                    qemu_log("%#x", (uint32_t)set);
> +                    ne_fprintf(f, "%#x", (uint32_t)set);
>                  } else {
> -                    qemu_log("%#" PRIx64, (uint64_t)set);
> +                    ne_fprintf(f, "%#" PRIx64, (uint64_t)set);

checkpatch saw these lines change and complains:

  ERROR: Don't use '#' flag of printf format ('%#') in format strings, use '0x' 
prefix instead

probably worth fixing up as we go.

Otherwise:

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

-- 
Alex Bennée