Re: [PATCH v2 10/10] disas/capstone: Add skipdata hook for s390x

[Thomas Huth]

On 15/09/2020 01.02, Richard Henderson wrote:
> It is always possible to tell the length of an insn, even if the
> actual insn is unknown.  Skip the correct number of bytes, so that
> we stay in sync with the instruction stream.
> 
> Reviewed-by: Philippe Mathieu-Daudé <f4bug@amsat.org>
> Signed-off-by: Richard Henderson <richard.henderson@linaro.org>
> ---
>  disas/capstone.c | 42 +++++++++++++++++++++++++++++++++++++++++-
>  1 file changed, 41 insertions(+), 1 deletion(-)
> 
> diff --git a/disas/capstone.c b/disas/capstone.c
> index b48f83958d..0a9ef9c892 100644
> --- a/disas/capstone.c
> +++ b/disas/capstone.c
> @@ -16,6 +16,39 @@
>   */
>  static __thread cs_insn *cap_insn;
>  
> +/*
> + * The capstone library always skips 2 bytes for S390X.
> + * This is less than ideal, since we can tell from the first two bits
> + * the size of the insn and thus stay in sync with the insn stream.
> + */
> +static size_t CAPSTONE_API
> +cap_skipdata_s390x_cb(const uint8_t *code, size_t code_size,
> +                      size_t offset, void *user_data)
> +{
> +    size_t ilen;
> +
> +    /* See get_ilen() in target/s390x/internal.h.  */
> +    switch (code[offset] >> 6) {
> +    case 0:
> +        ilen = 2;
> +        break;
> +    case 1:
> +    case 2:
> +        ilen = 4;
> +        break;
> +    default:
> +        ilen = 6;
> +        break;
> +    }
> +
> +    return ilen;
> +}
> +
> +static const cs_opt_skipdata cap_skipdata_s390x = {
> +    .mnemonic = ".byte",
> +    .callback = cap_skipdata_s390x_cb
> +};
> +
>  /*
>   * Initialize the Capstone library.
>   *
> @@ -42,13 +75,20 @@ static cs_err cap_disas_start(disassemble_info *info, csh 
> *handle)
>      /* "Disassemble" unknown insns as ".byte W,X,Y,Z".  */
>      cs_option(*handle, CS_OPT_SKIPDATA, CS_OPT_ON);
>  
> -    if (info->cap_arch == CS_ARCH_X86) {
> +    switch (info->cap_arch) {
> +    case CS_ARCH_SYSZ:
> +        cs_option(*handle, CS_OPT_SKIPDATA_SETUP,
> +                  (uintptr_t)&cap_skipdata_s390x);
> +        break;
> +
> +    case CS_ARCH_X86:
>          /*
>           * We don't care about errors (if for some reason the library
>           * is compiled without AT&T syntax); the user will just have
>           * to deal with the Intel syntax.
>           */
>          cs_option(*handle, CS_OPT_SYNTAX, CS_OPT_SYNTAX_ATT);
> +        break;
>      }
>  
>      /* Allocate temp space for cs_disasm_iter.  */
> 

Acked-by: Thomas Huth <thuth@redhat.com>