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

[Helge Deller]

Hi Philippe,

On 9/27/22 13:13, Philippe Mathieu-Daudé wrote:
> 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.

Sure.
The idea was to get feedback first before touching that many files.
If people think this approach is right, I'll send a v2 with the targets
updated accordingly.
Maybe better: s/EXCP_DUMP/target_exception_dump/

> >   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/

Ok.
Maybe: s/excp_dump/excp_dump_file/ ?


> > +{
> > +    if (logfile) {
>
> Rather:
>
>      assert(logfile);
>
> (programming error).

I don't think this is a good idea here.
The logfile comes from qemu_log_trylock() which can return 0 and
this is ignored in many other places too.
The application will stop anyway, so aborting - just because we can't
write to the log file - doesn't seem appropriate for me.

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

Thanks!
Helge