Re: [PATCH v14 08/10] Adding info [tb-list|tb] commands to HMP (WIP)

[Richard Henderson <richard.henderson@linaro.org>]

On 5/30/23 01:35, Fei Wu wrote:
> +static void do_dump_tbs_info(int total, int sort_by)
> +{
> +    id = 1;
> +    GList *i;
> +    int count = total;
> +
> +    g_list_free(last_search);
> +    last_search = NULL;
> +
> +    qht_iter(&tb_ctx.tb_stats, collect_tb_stats, NULL);
> +
> +    last_search = g_list_sort_with_data(last_search, inverse_sort_tbs,
> +                                        &sort_by);
> +

Why are you sorting on a list and not an array?
Intuitively, sorting a list of 1 million elements seems like the wrong choice.

Why did you put all the comparisons in one inverse_sort_tbs function, and require 
examining sort_by?  Better would be N sorting functions where sort_by is evaluated once 
before starting the sort.


> +++ b/disas/disas.c
> @@ -8,6 +8,8 @@
>  #include "hw/core/cpu.h"
>  #include "exec/memory.h"
>  
> +#include "qemu/log-for-trace.h"
> +
>  /* Filled in by elfload.c.  Simplistic, but will do for now. */
>  struct syminfo *syminfos = NULL;
>  
> @@ -199,6 +201,24 @@ static void initialize_debug_host(CPUDebug *s)
>  #endif
>  }
>  
> +static int
> +__attribute__((format(printf, 2, 3)))
> +fprintf_log(FILE *a, const char *b, ...)
> +{
> +    va_list ap;
> +    va_start(ap, b);
> +
> +    if (!to_string) {
> +        vfprintf(a, b, ap);
> +    } else {
> +        qemu_vlog(b, ap);
> +    }
> +
> +    va_end(ap);
> +
> +    return 1;
> +}
> +

Not need on this either.  Global variable being checked on each callback, instead of 
selecting the proper callback earlier -- preferably without the global variable.

Did you really need something different than monitor_disas?  You almost certainly want to 
read physical memory and not virtual anyway.

> +void qemu_log_to_monitor(bool enable)
> +{
> +    to_monitor = enable;
> +}
> +
> +void qemu_log_to_string(bool enable, GString *s)
> +{
> +    to_string = enable;
> +    string = s;
> +}

What are these for, and why do you need them?
Why would to_string ever be anything other than (string != NULL)?
Why are you using such very generic names for global variables?


r~