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

[Wu, Fei]

On 6/1/2023 10:40 AM, Richard Henderson wrote:
>> +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.
> 
Yes, it's usually okay for kernel address, but cannot re-gen the code
for userspace virtual address (guest kernel panic on riscv guest). I
tried monitor_disas() but it failed to disas too:
    monitor_disas(mon, mon_get_cpu(mon), tbs->phys_pc, num_inst, true);

How to use this function correctly?

Thanks,
Fei.

>> +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~