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

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

[Helge Deller]

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)

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

[Laurent Vivier]

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

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

[Richard Henderson]

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~

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

[Helge Deller]

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

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

[Richard Henderson]

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~

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

[Helge Deller]

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

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

[Richard Henderson]

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~

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

[Richard Henderson]

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~