Re: [PATCH v3 6/8] gdbstub: Add support for info proc mappings

[Alex Bennée]

Ilya Leoshkevich <iii@linux.ibm.com> writes:

> Currently the GDB's generate-core-file command doesn't work well with
> qemu-user: the resulting dumps are huge [1] and at the same time
> incomplete (argv and envp are missing). The reason is that GDB has no
> access to proc mappings and therefore has to fall back to using
> heuristics for discovering them. This is, in turn, because qemu-user
> does not implement the Host I/O feature of the GDB Remote Serial
> Protocol.
>
> Implement vFile:{open,close,pread,readlink} and also
> qXfer:exec-file:read+. With that, generate-core-file begins to work on
> aarch64 and s390x.
>
> [1] https://sourceware.org/pipermail/gdb-patches/2023-May/199432.html
>
> Co-developed-by: Dominik 'Disconnect3d' Czarnota 
> <dominik.b.czarnota@gmail.com>
> Signed-off-by: Ilya Leoshkevich <iii@linux.ibm.com>
> ---
>  gdbstub/gdbstub.c     |  45 +++++++++++++-
>  gdbstub/internals.h   |   5 ++
>  gdbstub/user-target.c | 139 ++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 187 insertions(+), 2 deletions(-)
>
> diff --git a/gdbstub/gdbstub.c b/gdbstub/gdbstub.c
> index c7e3ee71f2f..d2efefd3528 100644
> --- a/gdbstub/gdbstub.c
> +++ b/gdbstub/gdbstub.c
> @@ -1337,6 +1337,36 @@ static const GdbCmdParseEntry gdb_v_commands_table[] = 
> {
>          .cmd = "Kill;",
>          .cmd_startswith = 1
>      },
> +#ifdef CONFIG_USER_ONLY
> +    /*
> +     * Host I/O Packets. See [1] for details.
> +     * [1] 
> https://sourceware.org/gdb/onlinedocs/gdb/Host-I_002fO-Packets.html
> +     */
> +    {
> +        .handler = gdb_handle_v_file_open,
> +        .cmd = "File:open:",
> +        .cmd_startswith = 1,
> +        .schema = "s,L,L0"
> +    },
> +    {
> +        .handler = gdb_handle_v_file_close,
> +        .cmd = "File:close:",
> +        .cmd_startswith = 1,
> +        .schema = "l0"
> +    },
> +    {
> +        .handler = gdb_handle_v_file_pread,
> +        .cmd = "File:pread:",
> +        .cmd_startswith = 1,
> +        .schema = "l,L,L0"
> +    },
> +    {
> +        .handler = gdb_handle_v_file_readlink,
> +        .cmd = "File:readlink:",
> +        .cmd_startswith = 1,
> +        .schema = "s0"
> +    },
> +#endif
>  };
>  
>  static void handle_v_commands(GArray *params, void *user_ctx)
> @@ -1482,11 +1512,14 @@ static void handle_query_supported(GArray *params, 
> void *user_ctx)
>              ";ReverseStep+;ReverseContinue+");
>      }
>  
> -#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
> +#if defined(CONFIG_USER_ONLY)
> +#if defined(CONFIG_LINUX)
>      if (gdbserver_state.c_cpu->opaque) {
>          g_string_append(gdbserver_state.str_buf, ";qXfer:auxv:read+");
>      }
>  #endif
> +    g_string_append(gdbserver_state.str_buf, ";qXfer:exec-file:read+");
> +#endif
>  
>      if (params->len &&
>          strstr(get_param(params, 0)->data, "multiprocess+")) {
> @@ -1625,13 +1658,21 @@ static const GdbCmdParseEntry gdb_gen_query_table[] = 
> {
>          .cmd_startswith = 1,
>          .schema = "s:l,l0"
>      },
> -#if defined(CONFIG_USER_ONLY) && defined(CONFIG_LINUX)
> +#if defined(CONFIG_USER_ONLY)
> +#if defined(CONFIG_LINUX)
>      {
>          .handler = gdb_handle_query_xfer_auxv,
>          .cmd = "Xfer:auxv:read::",
>          .cmd_startswith = 1,
>          .schema = "l,l0"
>      },
> +#endif
> +    {
> +        .handler = gdb_handle_query_xfer_exec_file,
> +        .cmd = "Xfer:exec-file:read:",
> +        .cmd_startswith = 1,
> +        .schema = "l:l,l0"
> +    },
>  #endif
>      {
>          .handler = gdb_handle_query_attached,
> diff --git a/gdbstub/internals.h b/gdbstub/internals.h
> index 25e4d5eeaa6..f2b46cce412 100644
> --- a/gdbstub/internals.h
> +++ b/gdbstub/internals.h
> @@ -189,6 +189,11 @@ typedef union GdbCmdVariant {
>  void gdb_handle_query_rcmd(GArray *params, void *user_ctx); /* softmmu */
>  void gdb_handle_query_offsets(GArray *params, void *user_ctx); /* user */
>  void gdb_handle_query_xfer_auxv(GArray *params, void *user_ctx); /*user */
> +void gdb_handle_v_file_open(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_v_file_close(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_v_file_pread(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_v_file_readlink(GArray *params, void *user_ctx); /* user */
> +void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx); /* 
> user */
>  
>  void gdb_handle_query_attached(GArray *params, void *user_ctx); /* both */
>  
> diff --git a/gdbstub/user-target.c b/gdbstub/user-target.c
> index fa0e59ec9a5..aa64a8b9440 100644
> --- a/gdbstub/user-target.c
> +++ b/gdbstub/user-target.c
> @@ -11,6 +11,10 @@
>  #include "exec/gdbstub.h"
>  #include "qemu.h"
>  #include "internals.h"
> +#ifdef CONFIG_LINUX
> +#include "linux-user/loader.h"
> +#include "linux-user/qemu.h"
> +#endif
>  
>  /*
>   * Map target signal numbers to GDB protocol signal numbers and vice
> @@ -281,3 +285,138 @@ void gdb_handle_query_xfer_auxv(GArray *params, void 
> *user_ctx)
>                        gdbserver_state.str_buf->len, true);
>  }
>  #endif
> +
> +static const char *get_filename_param(GArray *params, int i)
> +{
> +    const char *hex_filename = get_param(params, i)->data;
> +    gdb_hextomem(gdbserver_state.mem_buf, hex_filename,
> +                 strlen(hex_filename) / 2);
> +    g_byte_array_append(gdbserver_state.mem_buf, (const guint8 *)"", 1);
> +    return (const char *)gdbserver_state.mem_buf->data;
> +}
> +
> +static void hostio_reply_with_data(const void *buf, size_t n)
> +{
> +    g_string_printf(gdbserver_state.str_buf, "F%lx;", n);
> +    gdb_memtox(gdbserver_state.str_buf, buf, n);
> +    gdb_put_packet_binary(gdbserver_state.str_buf->str,
> +                          gdbserver_state.str_buf->len, true);
> +}

This fails on 64/32 builds:

  ../gdbstub/user-target.c: In function ‘hostio_reply_with_data’:
  ../gdbstub/user-target.c:300:50: error: format ‘%lx’ expects argument of type 
‘long unsigned int’, but argument 3 has type ‘size_t’ {aka ‘unsigned int’} 
[-Werror=format=]
    300 |     g_string_printf(gdbserver_state.str_buf, "F%lx;", n);
        |                                                ~~^    ~
        |                                                  |    |
        |                                                  |    size_t {aka 
unsigned int}
        |                                                  long unsigned int
        |                                                %x
  cc1: all warnings being treated as errors

I think "%zx" is the portable format string you want.


> +void gdb_handle_v_file_open(GArray *params, void *user_ctx)
> +{
> +    const char *filename = get_filename_param(params, 0);
> +    uint64_t flags = get_param(params, 1)->val_ull;
> +    uint64_t mode = get_param(params, 2)->val_ull;
> +
> +#ifdef CONFIG_LINUX
> +    int fd = do_guest_openat(gdbserver_state.g_cpu->env_ptr, 0, filename,
> +                             flags, mode, false);
> +#else
> +    int fd = open(filename, flags, mode);
> +#endif
> +    if (fd < 0) {
> +        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> +    } else {
> +        g_string_printf(gdbserver_state.str_buf, "F%d", fd);
> +    }
> +    gdb_put_strbuf();
> +}
> +
> +void gdb_handle_v_file_close(GArray *params, void *user_ctx)
> +{
> +    int fd = get_param(params, 0)->val_ul;
> +
> +    if (close(fd) == -1) {
> +        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> +        gdb_put_strbuf();
> +        return;
> +    }
> +
> +    gdb_put_packet("F00");
> +}
> +
> +#define BUFSIZ 8192
> +
> +void gdb_handle_v_file_pread(GArray *params, void *user_ctx)
> +{
> +    int fd = get_param(params, 0)->val_ul;
> +    size_t count = get_param(params, 1)->val_ull;
> +    off_t offset = get_param(params, 2)->val_ull;
> +
> +    size_t bufsiz = MIN(count, BUFSIZ);
> +    g_autofree char *buf = g_try_malloc(bufsiz);
> +    if (buf == NULL) {
> +        gdb_put_packet("E12");
> +        return;
> +    }
> +
> +    ssize_t n = pread(fd, buf, bufsiz, offset);
> +    if (n < 0) {
> +        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> +        gdb_put_strbuf();
> +        return;
> +    }
> +    hostio_reply_with_data(buf, n);
> +}
> +
> +void gdb_handle_v_file_readlink(GArray *params, void *user_ctx)
> +{
> +    const char *filename = get_filename_param(params, 0);
> +
> +    g_autofree char *buf = g_try_malloc(BUFSIZ);
> +    if (buf == NULL) {
> +        gdb_put_packet("E12");
> +        return;
> +    }
> +
> +#ifdef CONFIG_LINUX
> +    ssize_t n = do_guest_readlink(filename, buf, BUFSIZ);
> +#else
> +    ssize_t n = readlink(filename, buf, BUFSIZ);
> +#endif
> +    if (n < 0) {
> +        g_string_printf(gdbserver_state.str_buf, "F-1,%d", errno);
> +        gdb_put_strbuf();
> +        return;
> +    }
> +    hostio_reply_with_data(buf, n);
> +}
> +
> +void gdb_handle_query_xfer_exec_file(GArray *params, void *user_ctx)
> +{
> +    uint32_t pid = get_param(params, 0)->val_ul;
> +    uint32_t offset = get_param(params, 1)->val_ul;
> +    uint32_t length = get_param(params, 2)->val_ul;
> +
> +    GDBProcess *process = gdb_get_process(pid);
> +    if (!process) {
> +        gdb_put_packet("E00");
> +        return;
> +    }
> +
> +    CPUState *cpu = gdb_get_first_cpu_in_process(process);
> +    if (!cpu) {
> +        gdb_put_packet("E00");
> +        return;
> +    }
> +
> +    TaskState *ts = cpu->opaque;
> +    if (!ts || !ts->bprm || !ts->bprm->filename) {
> +        gdb_put_packet("E00");
> +        return;
> +    }
> +
> +    size_t total_length = strlen(ts->bprm->filename);
> +    if (offset > total_length) {
> +        gdb_put_packet("E00");
> +        return;
> +    }
> +    if (offset + length > total_length) {
> +        length = total_length - offset;
> +    }
> +
> +    g_string_printf(gdbserver_state.str_buf, "l%.*s", length,
> +                    ts->bprm->filename + offset);
> +    gdb_put_strbuf();
> +}


-- 
Alex Bennée
Virtualisation Tech Lead @ Linaro