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.
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)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~
Le 24/10/2022 à 22:18, 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>
> ---
>
> 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.
>
>
> 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)
Applied to my linux-user-for-7.2 branch.
Thanks,
Laurent
© 2016 - 2024 Red Hat, Inc.