Re: [PATCH v2] linux-user: Add guest memory layout to exception dump

[Laurent Vivier <laurent@vivier.eu>]

Le 21/10/2022 à 16:57, Laurent Vivier a écrit :
> Le 27/09/2022 à 18:58, Helge Deller a écrit :
>> When the emulation stops with a hard exception it's very useful for
>> debugging purposes to dump the current guest memory layout (for an
>> example see /proc/self/maps) beside the CPU registers.
>>
>> The open_self_maps() function provides such a memory dump, but since
>> it's located in the syscall.c file, various changes (add #includes, make
>> this function externally visible, ...) are needed to be able to call it
>> from the existing EXCP_DUMP() macro.
>>
>> This patch takes another approach by re-defining EXCP_DUMP() to call
>> target_exception_dump(), which is in syscall.c, consolidates the log
>> print functions and allows to add the call to dump the memory layout.
>>
>> Beside a reduced code footprint, this approach keeps the changes across
>> the various callers minimal, and keeps EXCP_DUMP() highlighted as
>> important macro/function.
>>
>> Signed-off-by: Helge Deller <deller@gmx.de>
>>
>> ---
>>
>> v2:
>> Based on feedback by Philippe Mathieu-Daudé, renamed the two functions
>> to excp_dump_file() and target_exception_dump(), and #define'ed
>> EXCP_DUMP() to target_exception_dump().
>> I intentionally did not replace all occurences of EXCP_DUMP() by
>> target_exception_dump() as I think it's unneccesary and not beneficial.
>> If this is really wished, I will send a v3.
>>
>>
>> diff --git a/linux-user/cpu_loop-common.h b/linux-user/cpu_loop-common.h
>> index 36ff5b14f2..e644d2ef90 100644
>> --- a/linux-user/cpu_loop-common.h
>> +++ b/linux-user/cpu_loop-common.h
>> @@ -23,18 +23,9 @@
>>   #include "exec/log.h"
>>   #include "special-errno.h"
>>
>> -#define EXCP_DUMP(env, fmt, ...)                                        \
>> -do {                                                                    \
>> -    CPUState *cs = env_cpu(env);                                        \
>> -    fprintf(stderr, fmt , ## __VA_ARGS__);                              \
>> -    fprintf(stderr, "Failing executable: %s\n", exec_path);             \
>> -    cpu_dump_state(cs, stderr, 0);                                      \
>> -    if (qemu_log_separate()) {                                          \
>> -        qemu_log(fmt, ## __VA_ARGS__);                                  \
>> -        qemu_log("Failing executable: %s\n", exec_path);                \
>> -        log_cpu_state(cs, 0);                                           \
>> -    }                                                                   \
>> -} while (0)
>> +void target_exception_dump(CPUArchState *env, const char *fmt, int code);
>> +#define EXCP_DUMP(env, fmt, code) \
>> +    target_exception_dump(env, fmt, code)
>>
>>   void target_cpu_copy_regs(CPUArchState *env, struct target_pt_regs *regs);
>>   #endif
>> diff --git a/linux-user/syscall.c b/linux-user/syscall.c
>> index 2e954d8dbd..7d29c4c396 100644
>> --- a/linux-user/syscall.c
>> +++ b/linux-user/syscall.c
>> @@ -158,6 +158,7 @@
>>   #include "qapi/error.h"
>>   #include "fd-trans.h"
>>   #include "tcg/tcg.h"
>> +#include "cpu_loop-common.h"
>>
>>   #ifndef CLONE_IO
>>   #define CLONE_IO                0x80000000      /* Clone io context */
>> @@ -8144,6 +8145,33 @@ static int is_proc_myself(const char *filename, const char *entry)
>>       return 0;
>>   }
>>
>> +static void excp_dump_file(FILE *logfile, CPUArchState *env,
>> +                      const char *fmt, int code)
>> +{
>> +    if (logfile) {
>> +        CPUState *cs = env_cpu(env);
>> +
>> +        fprintf(logfile, fmt, code);
>> +        fprintf(logfile, "Failing executable: %s\n", exec_path);
>> +        cpu_dump_state(cs, logfile, 0);
>> +        open_self_maps(env, fileno(logfile));
>> +    }
>> +}
>> +
>> +void target_exception_dump(CPUArchState *env, const char *fmt, int code)
>> +{
>> +    /* dump to console */
>> +    excp_dump_file(stderr, env, fmt, code);
>> +
>> +    /* dump to log file */
>> +    if (qemu_log_separate()) {
>> +        FILE *logfile = qemu_log_trylock();
>> +
>> +        excp_dump_file(logfile, env, fmt, code);
>> +        qemu_log_unlock(logfile);
>> +    }
>> +}
>> +
>>   #if HOST_BIG_ENDIAN != TARGET_BIG_ENDIAN || \
>>       defined(TARGET_SPARC) || defined(TARGET_M68K) || defined(TARGET_HPPA)
>>   static int is_proc(const char *filename, const char *entry)
>>
> 
> Applied to my linux-user-for-7.2 branch.

This breaks build with:

.../linux-user/i386/cpu_loop.c: In function 'cpu_loop':
...linux-user/i386/cpu_loop.c:312:39: error: macro "EXCP_DUMP" passed 4 arguments, but takes just 3
   312 |                       (long)pc, trapnr);
       |                                       ^
In file included from .../linux-user/i386/cpu_loop.c:24:
.../linux-user/cpu_loop-common.h:27: note: macro "EXCP_DUMP" defined here
    27 | #define EXCP_DUMP(env, fmt, code) \
       |
.../linux-user/i386/cpu_loop.c:311:13: error: 'EXCP_DUMP' undeclared (first use in this function)
   311 |             EXCP_DUMP(env, "qemu: 0x%08lx: unhandled CPU exception 0x%x - aborting\n",
       |             ^~~~~~~~~
.../linux-user/i386/cpu_loop.c:311:13: note: each undeclared identifier is reported only once for 
each function it appears in
.../linux-user/i386/cpu_loop.c:204:15: error: variable 'pc' set but not used 
[-Werror=unused-but-set-variable]
   204 |     abi_ulong pc;
       |               ^~
cc1: all warnings being treated as errors