| Message ID | Y1bzAWbw07WBKPxw@p100 (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v3] linux-user: Add guest memory layout to exception dump | expand |
On 10/25/22 06:18, Helge Deller 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. /proc/self/maps has all of the qemu mappings in it as well. The page_dump() function provides exclusively the guest mappings. r~
On 10/25/22 00:35, Richard Henderson wrote: > On 10/25/22 06:18, Helge Deller 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. > > /proc/self/maps has all of the qemu mappings in it as well. I'm not quite sure on how to understand your comments above. Just comments or NAK to the patch? *Main* feature of this patch is that output like /proc/self/maps ends up on stdout and in the log file (if qemu log was enabled) at all, before the program exits and /proc/self/maps is gone. Quite useful for bug reports from other users too... > The page_dump() function provides exclusively the guest mappings. Which is usually sufficient in this case, and has the advantage that it shows the guest-stack and -heap areas. Helge
On 10/25/22 11:57, Helge Deller wrote: > On 10/25/22 00:35, Richard Henderson wrote: >> On 10/25/22 06:18, Helge Deller 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. >> >> /proc/self/maps has all of the qemu mappings in it as well. > > I'm not quite sure on how to understand your comments above. > Just comments or NAK to the patch? A question. Did you really wanted the host mappings included? If so, fine. If not, pointing out there's a better function to use. r~
On 10/25/22 04:25, Richard Henderson wrote: > On 10/25/22 11:57, Helge Deller wrote: >> On 10/25/22 00:35, Richard Henderson wrote: >>> On 10/25/22 06:18, Helge Deller 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. >>> >>> /proc/self/maps has all of the qemu mappings in it as well. >> >> I'm not quite sure on how to understand your comments above. >> Just comments or NAK to the patch? > > A question. > > Did you really wanted the host mappings included? No. I wanted just the guest mappings. > If so, fine. > If not, pointing out there's a better function to use. I'm not sure if it's the better choice. It depends on the targetted audience of such output. This is linux-user, so if someone runs a program he would expect output of crash dumps like as he would see them on a native machine. Showing "external host emulation mappings" seems strange. I'm running a debian hppa buildd server with linux-user. I've seen many guest crashes like SEGVs, out-of-memory (in guest), glibc's ABORT() calls [which e.g. triggers a CPU exception] or other things. In all those cases the guest mapping was relevant, not the host mapping. Helge
On 10/25/22 12:51, Helge Deller wrote: > On 10/25/22 04:25, Richard Henderson wrote: >> On 10/25/22 11:57, Helge Deller wrote: >>> On 10/25/22 00:35, Richard Henderson wrote: >>>> On 10/25/22 06:18, Helge Deller 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. >>>> >>>> /proc/self/maps has all of the qemu mappings in it as well. >>> >>> I'm not quite sure on how to understand your comments above. >>> Just comments or NAK to the patch? >> >> A question. >> >> Did you really wanted the host mappings included? > > No. > I wanted just the guest mappings. > >> If so, fine. >> If not, pointing out there's a better function to use. > > I'm not sure if it's the better choice. > It depends on the targetted audience of such output. > > This is linux-user, so if someone runs a program he would expect > output of crash dumps like as he would see them on a native machine. > Showing "external host emulation mappings" seems strange. Oh, I see. My comments above confused read_self_maps (host) with open_self_maps (filtered guest). I'll note that the output of page_dump() could be improved to exactly match /proc/self/maps format (which would probably be less confusing), and then re-implement open_self_maps in terms of that. This would avoid read_self_maps entirely. It would also fix a bug in that the host page permissions do not exactly match guest page permissions, and you're reporting host page permissions. r~
On 10/25/22 15:24, Richard Henderson wrote: > It would also fix a bug in that the host page permissions do not exactly match guest page > permissions, and you're reporting host page permissions. Gah, not true, we've already probed page_flags. Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
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/i386/cpu_loop.c b/linux-user/i386/cpu_loop.c index 42837399bc..404f6d6634 100644 --- a/linux-user/i386/cpu_loop.c +++ b/linux-user/i386/cpu_loop.c @@ -308,8 +308,8 @@ void cpu_loop(CPUX86State *env) break; default: pc = env->segs[R_CS].base + env->eip; - EXCP_DUMP(env, "qemu: 0x%08lx: unhandled CPU exception 0x%x - aborting\n", - (long)pc, trapnr); + EXCP_DUMP(env, "qemu: unhandled CPU exception 0x%x - aborting\n", + trapnr); abort(); } process_pending_signals(env); 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)
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> --- v3: Fix build error in i386/cpu_loop.c 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.