Re: [Qemu-devel] [RISU v2 04/11] risu_reginfo_i386: implement arch-specific reginfo interface

[Alex Bennée]

Jan Bobek <address@hidden> writes:

> CPU-specific code in risu_reginfo_* is expected to define and export
> the following symbols:
>
> - arch_long_opts, arch_extra_help, process_arch_opt
> - reginfo_size
> - reginfo_init
> - reginfo_is_eq
> - reginfo_dump, reginfo_dump_mismatch
>
> Make risu_reginfo_i386.c implement this interface; and while we're at
> it, expand the support to x86_64 as well.
>
> Suggested-by: Richard Henderson <address@hidden>
> Signed-off-by: Jan Bobek <address@hidden>

Reviewed-by: Alex Bennée <address@hidden>

> ---
>  risu_reginfo_i386.h |  24 ++++----
>  risu_reginfo_i386.c | 147 ++++++++++++++++++++++++++++++++++----------
>  2 files changed, 127 insertions(+), 44 deletions(-)
>
> diff --git a/risu_reginfo_i386.h b/risu_reginfo_i386.h
> index 5bba439..e350f01 100644
> --- a/risu_reginfo_i386.h
> +++ b/risu_reginfo_i386.h
> @@ -12,7 +12,8 @@
>  #ifndef RISU_REGINFO_I386_H
>  #define RISU_REGINFO_I386_H
>
> -/* This is the data structure we pass over the socket.
> +/*
> + * This is the data structure we pass over the socket.
>   * It is a simplified and reduced subset of what can
>   * be obtained with a ucontext_t*
>   */
> @@ -21,17 +22,14 @@ struct reginfo {
>      gregset_t gregs;
>  };
>
> -#ifndef REG_GS
> -/* Assume that either we get all these defines or none */
> -#   define REG_GS      0
> -#   define REG_FS      1
> -#   define REG_ES      2
> -#   define REG_DS      3
> -#   define REG_ESP     7
> -#   define REG_TRAPNO 12
> -#   define REG_EIP    14
> -#   define REG_EFL    16
> -#   define REG_UESP   17
> -#endif /* !defined(REG_GS) */
> +/*
> + * For i386, the defines are named REG_EAX, etc.
> + * For x86_64, the defines are named REG_RAX, etc.
> + */
> +#ifdef __x86_64__
> +# define REG_E(X)   REG_R##X
> +#else
> +# define REG_E(X)   REG_E##X
> +#endif
>
>  #endif /* RISU_REGINFO_I386_H */
> diff --git a/risu_reginfo_i386.c b/risu_reginfo_i386.c
> index e8d671f..c4dc14a 100644
> --- a/risu_reginfo_i386.c
> +++ b/risu_reginfo_i386.c
> @@ -10,59 +10,144 @@
>   
> ******************************************************************************/
>
>  #include <stdio.h>
> +#include <stdlib.h>
> +#include <string.h>
>  #include <ucontext.h>
> +#include <assert.h>
>
>  #include "risu.h"
>  #include "risu_reginfo_i386.h"
>
> -static void fill_reginfo(struct reginfo *ri, ucontext_t * uc)
> +const struct option * const arch_long_opts;
> +const char * const arch_extra_help;
> +
> +void process_arch_opt(int opt, const char *arg)
> +{
> +    abort();
> +}
> +
> +const int reginfo_size(void)
> +{
> +    return sizeof(struct reginfo);
> +}
> +
> +/* reginfo_init: initialize with a ucontext */
> +void reginfo_init(struct reginfo *ri, ucontext_t *uc)
>  {
>      int i;
> +
> +    memset(ri, 0, sizeof(*ri));
> +
>      for (i = 0; i < NGREG; i++) {
>          switch (i) {
> -        case REG_ESP:
> -        case REG_UESP:
> -        case REG_GS:
> -        case REG_FS:
> -        case REG_ES:
> -        case REG_DS:
> -        case REG_TRAPNO:
> -        case REG_EFL:
> -            /* Don't store these registers as it results in mismatches.
> -             * In particular valgrind has different values for some
> -             * segment registers, and they're boring anyway.
> -             * We really shouldn't be ignoring EFL but valgrind doesn't
> -             * seem to set it right and I don't care to investigate.
> -             */
> -            ri->gregs[i] = 0xDEADBEEF;
> -            break;
> -        case REG_EIP:
> -            /* Store the offset from the start of the test image */
> +        case REG_E(IP):
> +            /* Store the offset from the start of the test image.  */
>              ri->gregs[i] = uc->uc_mcontext.gregs[i] - image_start_address;
>              break;
> -        default:
> +        case REG_EFL:
> +            /* Store only the "flaggy" bits: SF, ZF, AF, PF, CF.  */
> +            ri->gregs[i] = uc->uc_mcontext.gregs[i] & 0xd5;
> +            break;
> +        case REG_E(SP):
> +            /* Ignore the stack.  */
> +            ri->gregs[i] = 0xdeadbeef;
> +            break;
> +        case REG_E(AX):
> +        case REG_E(BX):
> +        case REG_E(CX):
> +        case REG_E(DX):
> +        case REG_E(DI):
> +        case REG_E(SI):
> +        case REG_E(BP):
> +#ifdef __x86_64__
> +        case REG_R8:
> +        case REG_R9:
> +        case REG_R10:
> +        case REG_R11:
> +        case REG_R12:
> +        case REG_R13:
> +        case REG_R14:
> +        case REG_R15:
> +#endif
>              ri->gregs[i] = uc->uc_mcontext.gregs[i];
>              break;
>          }
>      }
> -    /* x86 insns aren't 32 bit but we're not really testing x86 so
> -     * this is just to distinguish 'do compare' from 'stop'
> +
> +    /*
> +     * x86 insns aren't 32 bit but 3 bytes are sufficient to
> +     * distinguish 'do compare' from 'stop'.
>       */
> -    ri->faulting_insn = *((uint32_t *) uc->uc_mcontext.gregs[REG_EIP]);
> +    ri->faulting_insn = *(uint32_t *)uc->uc_mcontext.gregs[REG_E(IP)];
>  }
>
> -static char *regname[] = {
> -    "GS", "FS", "ES", "DS", "EDI", "ESI", "EBP", "ESP",
> -    "EBX", "EDX", "ECX", "EAX", "TRAPNO", "ERR", "EIP",
> -    "CS", "EFL", "UESP", "SS", 0
> +/* reginfo_is_eq: compare the reginfo structs, returns nonzero if equal */
> +int reginfo_is_eq(struct reginfo *m, struct reginfo *a)
> +{
> +    return 0 == memcmp(m, a, sizeof(*m));
> +}
> +
> +static const char *const regname[NGREG] = {
> +    [REG_EFL] = "eflags",
> +#ifdef __x86_64__
> +    [REG_RIP] = "rip",
> +    [REG_RAX] = "rax",
> +    [REG_RBX] = "rbx",
> +    [REG_RCX] = "rcx",
> +    [REG_RDX] = "rdx",
> +    [REG_RDI] = "rdi",
> +    [REG_RSI] = "rsi",
> +    [REG_RBP] = "rbp",
> +    [REG_RSP] = "rsp",
> +    [REG_R8]  = "r8",
> +    [REG_R9]  = "r9",
> +    [REG_R10] = "r10",
> +    [REG_R11] = "r11",
> +    [REG_R12] = "r12",
> +    [REG_R13] = "r13",
> +    [REG_R14] = "r14",
> +    [REG_R15] = "r15",
> +#else
> +    [REG_EIP] = "eip",
> +    [REG_EAX] = "eax",
> +    [REG_EBX] = "ebx",
> +    [REG_ECX] = "ecx",
> +    [REG_EDX] = "edx",
> +    [REG_EDI] = "edi",
> +    [REG_ESI] = "esi",
> +    [REG_EBP] = "ebp",
> +    [REG_ESP] = "esp",
> +#endif
>  };
>
> -static void dump_reginfo(struct reginfo *ri)
> +#ifdef __x86_64__
> +# define PRIxREG   "%016llx"
> +#else
> +# define PRIxREG   "%08x"
> +#endif
> +
> +/* reginfo_dump: print state to a stream, returns nonzero on success */
> +int reginfo_dump(struct reginfo *ri, FILE *f)
>  {
>      int i;
> -    fprintf(stderr, "  faulting insn %x\n", ri->faulting_insn);
> +    fprintf(f, "  faulting insn %x\n", ri->faulting_insn);
>      for (i = 0; i < NGREG; i++) {
> -        fprintf(stderr, "  %s: %x\n", regname[i] ? regname[i] : "???",
> -                ri->gregs[i]);
> +        if (regname[i]) {
> +            fprintf(f, "  %-6s: " PRIxREG "\n", regname[i], ri->gregs[i]);
> +        }
>      }
> +    return !ferror(f);
> +}
> +
> +int reginfo_dump_mismatch(struct reginfo *m, struct reginfo *a, FILE *f)
> +{
> +    int i;
> +    for (i = 0; i < NGREG; i++) {
> +        if (m->gregs[i] != a->gregs[i]) {
> +            assert(regname[i]);
> +            fprintf(f, "Mismatch: %s: " PRIxREG " v " PRIxREG "\n",
> +                    regname[i], m->gregs[i], a->gregs[i]);
> +        }
> +    }
> +    return !ferror(f);
>  }


--
Alex Bennée