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

[Philippe Mathieu-Daudé]

Hi Helge,

On Tue, Sep 27, 2022 at 12:09 PM Helge Deller <deller@gmx.de> wrote:
>
> 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 un-macronizing EXCP_DUMP() and turn
> it into a function located in syscall.c.
> Beside a reduced code footprint, this approach allows to add the memory
> dump and simplify the code to print to console and log file.
>
> Signed-off-by: Helge Deller <deller@gmx.de>
>
> diff --git a/linux-user/cpu_loop-common.h b/linux-user/cpu_loop-common.h
> index 36ff5b14f2..0b26b56915 100644
> --- a/linux-user/cpu_loop-common.h
> +++ b/linux-user/cpu_loop-common.h
> @@ -23,18 +23,7 @@
>  #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 EXCP_DUMP(CPUArchState *env, const char *fmt, int code);

s/EXCP_DUMP/target_cpu_dump_exception/

Worth split as a preliminary patch updating all targets.

>  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 d17f5d1c66..00861e9351 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 */
> @@ -8177,6 +8178,33 @@ static int is_proc_myself(const char *filename, const 
> char *entry)
>      return 0;
>  }
>
> +static void excp_dump(FILE *logfile, CPUArchState *env,
> +                      const char *fmt, int code)

s/excp_dump/dump_excp_file/

> +{
> +    if (logfile) {

Rather:

    assert(logfile);

(programming error).

> +        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 EXCP_DUMP(CPUArchState *env, const char *fmt, int code)

s/EXCP_DUMP/target_cpu_dump_exception/

> +{
> +    /* dump to console */
> +    excp_dump(stderr, env, fmt, code);
> +
> +    /* dump to log file */
> +    if (qemu_log_separate()) {
> +        FILE *logfile = qemu_log_trylock();
> +
> +        excp_dump(logfile, env, fmt, code);
> +        qemu_log_unlock(logfile);
> +    }
> +}

Nitpicking a bit, otherwise LGTM.