Re: [Qemu-devel] [PATCH] cpu-exec: Allow "-d exec" in non-debug builds (drop CONFIG_DEBUG_EXEC)

["Edgar E. Iglesias" <edgar.iglesias@gmail.com>]

On Thu, Apr 11, 2013 at 09:21:46PM +0100, Peter Maydell wrote:
> The CONFIG_DEBUG_EXEC define compiles out a single qemu_log_mask()
> call, which is a pretty trivial cost even for something in the main
> cpu_exec() loop.  Having this be conditionally defined means that
> '-d exec' on a non-debug build will silently do nothing.  Drop the
> define and the configure machinery that sets it, in favour of just
> always allowing this log option to be enabled at runtime.  As a
> concession to the mainloopiness, we use qemu_loglevel_mask()+qemu_log()
> rather than qemu_log_mask() to avoid the function call overhead.
> 
> Note that DEBUG_DISAS is always defined, so removing the
> '|| defined(CONFIG_DEBUG_EXEC)' from those conditionals makes
> no behavioural change for that logging.
> 
> Signed-off-by: Peter Maydell <peter.maydell@linaro.org>

I agree, lack of -d exec has bitten me a couple of times
aswell. A separate patch with an option to disable tb-
chaining would also be helpful imo.

Acked-by: Edgar E. Iglesias <edgar.iglesias@gmail.com>



> ---
> I've been burnt at least once by asking a user to do a '-d exec'
> log only to find it didn't actually log anything...
> 
>  configure  |  3 ---
>  cpu-exec.c | 15 ++++++---------
>  2 files changed, 6 insertions(+), 12 deletions(-)
> 
> diff --git a/configure b/configure
> index 1ed939a..73df181 100755
> --- a/configure
> +++ b/configure
> @@ -3471,9 +3471,6 @@ echo "ARCH=$ARCH" >> $config_host_mak
>  if test "$debug_tcg" = "yes" ; then
>    echo "CONFIG_DEBUG_TCG=y" >> $config_host_mak
>  fi
> -if test "$debug" = "yes" ; then
> -  echo "CONFIG_DEBUG_EXEC=y" >> $config_host_mak
> -fi
>  if test "$strip_opt" = "yes" ; then
>    echo "STRIP=${strip}" >> $config_host_mak
>  fi
> diff --git a/cpu-exec.c b/cpu-exec.c
> index e74e556..235ddee 100644
> --- a/cpu-exec.c
> +++ b/cpu-exec.c
> @@ -23,8 +23,6 @@
>  #include "qemu/atomic.h"
>  #include "sysemu/qtest.h"
>  
> -//#define CONFIG_DEBUG_EXEC
> -
>  bool qemu_cpu_has_work(CPUState *cpu)
>  {
>      return cpu_has_work(cpu);
> @@ -567,7 +565,7 @@ int cpu_exec(CPUArchState *env)
>                      env->exception_index = EXCP_INTERRUPT;
>                      cpu_loop_exit(env);
>                  }
> -#if defined(DEBUG_DISAS) || defined(CONFIG_DEBUG_EXEC)
> +#if defined(DEBUG_DISAS)
>                  if (qemu_loglevel_mask(CPU_LOG_TB_CPU)) {
>                      /* restore flags in standard format */
>  #if defined(TARGET_I386)
> @@ -582,7 +580,7 @@ int cpu_exec(CPUArchState *env)
>                      log_cpu_state(env, 0);
>  #endif
>                  }
> -#endif /* DEBUG_DISAS || CONFIG_DEBUG_EXEC */
> +#endif /* DEBUG_DISAS */
>                  spin_lock(&tcg_ctx.tb_ctx.tb_lock);
>                  tb = tb_find_fast(env);
>                  /* Note: we do it here to avoid a gcc bug on Mac OS X when
> @@ -594,11 +592,10 @@ int cpu_exec(CPUArchState *env)
>                      next_tb = 0;
>                      tcg_ctx.tb_ctx.tb_invalidated_flag = 0;
>                  }
> -#ifdef CONFIG_DEBUG_EXEC
> -                qemu_log_mask(CPU_LOG_EXEC, "Trace %p [" TARGET_FMT_lx "] %s\n",
> -                             tb->tc_ptr, tb->pc,
> -                             lookup_symbol(tb->pc));
> -#endif
> +                if (qemu_loglevel_mask(CPU_LOG_EXEC)) {
> +                    qemu_log("Trace %p [" TARGET_FMT_lx "] %s\n",
> +                             tb->tc_ptr, tb->pc, lookup_symbol(tb->pc));
> +                }
>                  /* see if we can patch the calling TB. When the TB
>                     spans two pages, we cannot safely do a direct
>                     jump. */
> -- 
> 1.7.11.4
> 
>