[Qemu-devel] [RFC PATCH 0/4] add hand-rolled fallback when capstone fails

[Qemu-devel] [RFC PATCH 0/4] add hand-rolled fallback when capstone fails

[Alex Bennée]

Hi,

While capstone is actively maintained it hasn't managed to keep up to
date with newer instructions as they have been added. While these
should eventually be supported we need something in the meantime.

This proof-of-concept series takes advantage of the fact we already
have a parser for SVE instructions. By tweaking the output of
decodetree.py a little we can generate something we can plug into the
assembly dump when capstone fails. Currently it is just the
instruction name (as encoded in sve.decode) but extending it to
include the parameters shouldn't be too hard.

The plumbing into disas is a little ugly and perhaps that can be
solved later with some re-factoring.

So what do you think? Worth pursing or adding to the pile of cute but
not ultimately mergable hacks?

Alex Bennée (4):
  scripts/decodetree.py: add a disassembly generator (HACK!)
  target/arm: move decoder helpers into header
  target/arm: add a fallback disassemble function
  disas: allow capstone to defer to a fallback function on failure

 disas.c                    | 30 +++++++++++++++++++++-
 include/disas/bfd.h        | 11 +++++++-
 scripts/decodetree.py      | 52 +++++++++++++++++++++++++++++++++-----
 target/arm/Makefile.objs   |  8 ++++++
 target/arm/cpu.c           |  4 +++
 target/arm/decoder.h       | 50 ++++++++++++++++++++++++++++++++++++
 target/arm/disassemble.c   | 22 ++++++++++++++++
 target/arm/internals.h     |  2 ++
 target/arm/translate-sve.c | 50 +-----------------------------------
 9 files changed, 172 insertions(+), 57 deletions(-)
 create mode 100644 target/arm/decoder.h
 create mode 100644 target/arm/disassemble.c

-- 
2.17.1

[Qemu-devel] [RFC PATCH 1/4] scripts/decodetree.py: add a disassembly generator (HACK!)

[Alex Bennée]

Given our issues with failing disassembly we could try and re-use the
decode tree data to output what instruction is being decoded. This
will be used if registered as a fall-back for when the "proper"
disassembler fails to decode an instruction.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 scripts/decodetree.py | 52 ++++++++++++++++++++++++++++++++++++++-----
 1 file changed, 46 insertions(+), 6 deletions(-)

diff --git a/scripts/decodetree.py b/scripts/decodetree.py
index 277f9a9bba..f4b4318c96 100755
--- a/scripts/decodetree.py
+++ b/scripts/decodetree.py
@@ -169,6 +169,7 @@ input_file = ''
 output_file = None
 output_fd = None
 insntype = 'uint32_t'
+disassemble = False
 
 re_ident = '[a-zA-Z][a-zA-Z0-9_]*'
 
@@ -467,6 +468,7 @@ class Pattern(General):
 
     def output_code(self, i, extracted, outerbits, outermask):
         global translate_prefix
+        global disassemble
         ind = str_indent(i)
         arg = self.base.base.name
         output(ind, '/* line ', str(self.lineno), ' */\n')
@@ -474,8 +476,34 @@ class Pattern(General):
             output(ind, self.base.extract_name(), '(&u.f_', arg, ', insn);\n')
         for n, f in self.fields.items():
             output(ind, 'u.f_', arg, '.', n, ' = ', f.str_extract(), ';\n')
-        output(ind, 'return ', translate_prefix, '_', self.name,
-               '(ctx, &u.f_', arg, ', insn);\n')
+        output(ind, 'return ', translate_prefix, '_', self.name)
+        if disassemble:
+            output("(dstr, maxd, ")
+        else:
+            output("(ctx, ")
+
+        output('&u.f_', arg)
+
+        if disassemble:
+            output(");\n")
+        else:
+            output(', insn);\n')
+
+    def output_formatter(self):
+        global translate_prefix
+        arg = self.base.base.name
+        output('/* line ', str(self.lineno), ' */\n')
+        output('typedef ', self.base.base.struct_name(),
+               ' arg_', self.name, ';\n')
+        output(translate_scope, 'bool ', translate_prefix, '_', self.name,
+               '(char *ptr, size_t n, arg_', self.name, ' *a)\n')
+        output("{\n")
+        output(str_indent(4), 'snprintf(ptr, n, "', self.name)
+        # fill in arguments here
+        output('"); \n')
+        output(str_indent(4), "return true;\n")
+        output("}\n")
+
 # end Pattern
 
 
@@ -973,11 +1001,12 @@ def main():
     global insnwidth
     global insntype
     global insnmask
+    global disassemble
 
     decode_function = 'decode'
     decode_scope = 'static '
 
-    long_opts = ['decode=', 'translate=', 'output=', 'insnwidth=']
+    long_opts = ['decode=', 'translate=', 'output=', 'insnwidth=', 'disassemble']
     try:
         (opts, args) = getopt.getopt(sys.argv[1:], 'o:w:', long_opts)
     except getopt.GetoptError as err:
@@ -998,6 +1027,8 @@ def main():
                 insnmask = 0xffff
             elif insnwidth != 32:
                 error(0, 'cannot handle insns of width', insnwidth)
+        elif o == '--disassemble':
+            disassemble = True
         else:
             assert False, 'unhandled option'
 
@@ -1031,7 +1062,10 @@ def main():
             if i.base.base != p.base.base:
                 error(0, i.name, ' has conflicting argument sets')
         else:
-            i.output_decl()
+            if disassemble:
+                i.output_formatter()
+            else:
+                i.output_decl()
             out_pats[i.name] = i
     output('\n')
 
@@ -1039,8 +1073,14 @@ def main():
         f = formats[n]
         f.output_extract()
 
-    output(decode_scope, 'bool ', decode_function,
-           '(DisasContext *ctx, ', insntype, ' insn)\n{\n')
+    output(decode_scope, 'bool ', decode_function)
+
+    if disassemble:
+        output("(char *dstr, size_t maxd, ")
+    else:
+        output('(DisasContext *ctx, ')
+
+    output(insntype, ' insn)\n{\n')
 
     i4 = str_indent(4)
     output(i4, 'union {\n')
-- 
2.17.1

[Qemu-devel] [RFC PATCH 2/4] target/arm: move decoder helpers into header

[Alex Bennée]

We want to re-use these helpers.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/decoder.h       | 50 ++++++++++++++++++++++++++++++++++++++
 target/arm/translate-sve.c | 50 +-------------------------------------
 2 files changed, 51 insertions(+), 49 deletions(-)
 create mode 100644 target/arm/decoder.h

diff --git a/target/arm/decoder.h b/target/arm/decoder.h
new file mode 100644
index 0000000000..09a043fb2b
--- /dev/null
+++ b/target/arm/decoder.h
@@ -0,0 +1,50 @@
+/*
+ * Helpers for extracting complex instruction fields.
+ *
+ * These are referenced in the .decode file and emitted by decodetree.py
+ */
+
+/* See e.g. ASR (immediate, predicated).
+ * Returns -1 for unallocated encoding; diagnose later.
+ */
+static inline int tszimm_esz(int x)
+{
+    x >>= 3;  /* discard imm3 */
+    return 31 - clz32(x);
+}
+
+static inline int tszimm_shr(int x)
+{
+    return (16 << tszimm_esz(x)) - x;
+}
+
+/* See e.g. LSL (immediate, predicated).  */
+static inline int tszimm_shl(int x)
+{
+    return x - (8 << tszimm_esz(x));
+}
+
+static inline int plus1(int x)
+{
+    return x + 1;
+}
+
+/* The SH bit is in bit 8.  Extract the low 8 and shift.  */
+static inline int expand_imm_sh8s(int x)
+{
+    return (int8_t)x << (x & 0x100 ? 8 : 0);
+}
+
+static inline int expand_imm_sh8u(int x)
+{
+    return (uint8_t)x << (x & 0x100 ? 8 : 0);
+}
+
+/* Convert a 2-bit memory size (msz) to a 4-bit data type (dtype)
+ * with unsigned data.  C.f. SVE Memory Contiguous Load Group.
+ */
+static inline int msz_dtype(int msz)
+{
+    static const uint8_t dtype[4] = { 0, 5, 10, 15 };
+    return dtype[msz];
+}
diff --git a/target/arm/translate-sve.c b/target/arm/translate-sve.c
index 374051cd20..e3ec5c8ec2 100644
--- a/target/arm/translate-sve.c
+++ b/target/arm/translate-sve.c
@@ -33,7 +33,7 @@
 #include "trace-tcg.h"
 #include "translate-a64.h"
 #include "fpu/softfloat.h"
-
+#include "decoder.h"
 
 typedef void GVecGen2sFn(unsigned, uint32_t, uint32_t,
                          TCGv_i64, uint32_t, uint32_t);
@@ -47,54 +47,6 @@ typedef void gen_helper_gvec_mem(TCGv_env, TCGv_ptr, TCGv_i64, TCGv_i32);
 typedef void gen_helper_gvec_mem_scatter(TCGv_env, TCGv_ptr, TCGv_ptr,
                                          TCGv_ptr, TCGv_i64, TCGv_i32);
 
-/*
- * Helpers for extracting complex instruction fields.
- */
-
-/* See e.g. ASR (immediate, predicated).
- * Returns -1 for unallocated encoding; diagnose later.
- */
-static int tszimm_esz(int x)
-{
-    x >>= 3;  /* discard imm3 */
-    return 31 - clz32(x);
-}
-
-static int tszimm_shr(int x)
-{
-    return (16 << tszimm_esz(x)) - x;
-}
-
-/* See e.g. LSL (immediate, predicated).  */
-static int tszimm_shl(int x)
-{
-    return x - (8 << tszimm_esz(x));
-}
-
-static inline int plus1(int x)
-{
-    return x + 1;
-}
-
-/* The SH bit is in bit 8.  Extract the low 8 and shift.  */
-static inline int expand_imm_sh8s(int x)
-{
-    return (int8_t)x << (x & 0x100 ? 8 : 0);
-}
-
-static inline int expand_imm_sh8u(int x)
-{
-    return (uint8_t)x << (x & 0x100 ? 8 : 0);
-}
-
-/* Convert a 2-bit memory size (msz) to a 4-bit data type (dtype)
- * with unsigned data.  C.f. SVE Memory Contiguous Load Group.
- */
-static inline int msz_dtype(int msz)
-{
-    static const uint8_t dtype[4] = { 0, 5, 10, 15 };
-    return dtype[msz];
-}
 
 /*
  * Include the generated decoder.
-- 
2.17.1

[Qemu-devel] [RFC PATCH 3/4] target/arm: add a fallback disassemble function

[Alex Bennée]

Now we can generate a disassembler we need a function to hook into it.
As we only deal with SVE instructions at the moment we don't need to
differentiate the various decoders.

I special case 0x5af0 as it is used by RISU for checkpoints.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 target/arm/Makefile.objs |  8 ++++++++
 target/arm/disassemble.c | 22 ++++++++++++++++++++++
 target/arm/internals.h   |  2 ++
 3 files changed, 32 insertions(+)
 create mode 100644 target/arm/disassemble.c

diff --git a/target/arm/Makefile.objs b/target/arm/Makefile.objs
index 11c7baf8a3..4339353df8 100644
--- a/target/arm/Makefile.objs
+++ b/target/arm/Makefile.objs
@@ -20,3 +20,11 @@ target/arm/decode-sve.inc.c: $(SRC_PATH)/target/arm/sve.decode $(DECODETREE)
 
 target/arm/translate-sve.o: target/arm/decode-sve.inc.c
 obj-$(TARGET_AARCH64) += translate-sve.o sve_helper.o
+
+target/arm/disas-sve.inc.c: $(SRC_PATH)/target/arm/sve.decode $(DECODETREE)
+	$(call quiet-command,\
+	  $(PYTHON) $(DECODETREE) --disassemble -o $@ $<,\
+	  "GEN", $(TARGET_DIR)$@)
+
+target/arm/disassemble.o: target/arm/disas-sve.inc.c
+obj-$(TARGET_AARCH64) += disassemble.o
diff --git a/target/arm/disassemble.c b/target/arm/disassemble.c
new file mode 100644
index 0000000000..801f9680cb
--- /dev/null
+++ b/target/arm/disassemble.c
@@ -0,0 +1,22 @@
+/*
+ * Fallback dissasembly
+ */
+#include "qemu/osdep.h"
+#include "cpu.h"
+#include "internals.h"
+#include "decoder.h"
+
+#include "disas-sve.inc.c"
+
+size_t do_aarch64_fallback_disassembly(const uint8_t *insnp, char *ptr, size_t n)
+{
+    uint32_t insn = ldl_p(insnp);
+
+    if (insn == 0x5af0) {
+        snprintf(ptr, n, "illegal insn (risu checkpoint?)");
+    } else if (!decode(ptr, n, insn)) {
+        snprintf(ptr, n, "failed decode");
+    }
+
+    return 4;
+}
diff --git a/target/arm/internals.h b/target/arm/internals.h
index dc9357766c..80796632a2 100644
--- a/target/arm/internals.h
+++ b/target/arm/internals.h
@@ -796,4 +796,6 @@ static inline uint32_t arm_debug_exception_fsr(CPUARMState *env)
     }
 }
 
+size_t do_aarch64_fallback_disassembly(const uint8_t *insn, char *ptr, size_t n);
+
 #endif
-- 
2.17.1

[Qemu-devel] [RFC PATCH 4/4] disas: allow capstone to defer to a fallback function on failure

[Alex Bennée]

We can abuse the CS_OPT_SKIPDATA by providing a call back when
capstone can't disassemble something. The passing of the string to the
dump function is a little clunky but works.

Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
---
 disas.c             | 30 +++++++++++++++++++++++++++++-
 include/disas/bfd.h | 11 ++++++++++-
 target/arm/cpu.c    |  4 ++++
 3 files changed, 43 insertions(+), 2 deletions(-)

diff --git a/disas.c b/disas.c
index 5325b7e6be..dfd2c251c5 100644
--- a/disas.c
+++ b/disas.c
@@ -178,6 +178,20 @@ static int print_insn_od_target(bfd_vma pc, disassemble_info *info)
    to share this across calls and across host vs target disassembly.  */
 static __thread cs_insn *cap_insn;
 
+
+/* Handle fall-back dissasembly. We don't print here but we do set
+ * cap_fallback_str for cap_dump_insn to used*/
+static size_t cap_disas_fallback(const uint8_t *code, size_t code_size,
+                                 size_t offset, void *user_data)
+{
+    disassemble_info *info = (disassemble_info *) user_data;
+    info->cap_fallback_str = g_malloc0(256);
+    size_t skip = info->capstone_fallback_func(code + offset,
+                                               info->cap_fallback_str, 256);
+    return skip;
+}
+
+
 /* Initialize the Capstone library.  */
 /* ??? It would be nice to cache this.  We would need one handle for the
    host and one for the target.  For most targets we can reset specific
@@ -206,6 +220,14 @@ static cs_err cap_disas_start(disassemble_info *info, csh *handle)
         cs_option(*handle, CS_OPT_SYNTAX, CS_OPT_SYNTAX_ATT);
     }
 
+    if (info->capstone_fallback_func) {
+        cs_opt_skipdata skipdata = {
+            .callback = cap_disas_fallback,
+            .user_data = info,
+        };
+        cs_option(*handle, CS_OPT_SKIPDATA_SETUP, (size_t) &skipdata);
+    }
+
     /* "Disassemble" unknown insns as ".byte W,X,Y,Z".  */
     cs_option(*handle, CS_OPT_SKIPDATA, CS_OPT_ON);
 
@@ -281,7 +303,13 @@ static void cap_dump_insn(disassemble_info *info, cs_insn *insn)
     }
 
     /* Print the actual instruction.  */
-    print(info->stream, "  %-8s %s\n", insn->mnemonic, insn->op_str);
+    if (info->cap_fallback_str) {
+        print(info->stream, "  %s\n", info->cap_fallback_str);
+        g_free(info->cap_fallback_str);
+        info->cap_fallback_str = NULL;
+    } else {
+        print(info->stream, "  %-8s %s\n", insn->mnemonic, insn->op_str);
+    }
 
     /* Dump any remaining part of the insn on subsequent lines.  */
     for (i = split; i < n; i += split) {
diff --git a/include/disas/bfd.h b/include/disas/bfd.h
index 1f69a6e9d3..9d99bfef48 100644
--- a/include/disas/bfd.h
+++ b/include/disas/bfd.h
@@ -377,6 +377,12 @@ typedef struct disassemble_info {
   int cap_insn_unit;
   int cap_insn_split;
 
+  /* Fallback function to disassemble things capstone can't. */
+  size_t (*capstone_fallback_func)
+    (const uint8_t *insn, char *ptr, size_t n);
+
+  char *cap_fallback_str;
+
 } disassemble_info;
 
 \f

@@ -491,7 +497,10 @@ int generic_symbol_at_address(bfd_vma, struct disassemble_info *);
   (INFO).bytes_per_chunk = 0, \
   (INFO).display_endian = BFD_ENDIAN_UNKNOWN, \
   (INFO).disassembler_options = NULL, \
-  (INFO).insn_info_valid = 0
+  (INFO).insn_info_valid = 0, \
+  (INFO).capstone_fallback_func = NULL, \
+  (INFO).cap_fallback_str = NULL
+
 
 #ifndef ATTRIBUTE_UNUSED
 #define ATTRIBUTE_UNUSED __attribute__((unused))
diff --git a/target/arm/cpu.c b/target/arm/cpu.c
index 64a8005a4b..cfefbfb0b9 100644
--- a/target/arm/cpu.c
+++ b/target/arm/cpu.c
@@ -519,6 +519,10 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info *info)
         info->cap_arch = CS_ARCH_ARM64;
         info->cap_insn_unit = 4;
         info->cap_insn_split = 4;
+
+#if defined(TARGET_AARCH64)
+        info->capstone_fallback_func = do_aarch64_fallback_disassembly;
+#endif
     } else {
         int cap_mode;
         if (env->thumb) {
-- 
2.17.1

Re: [Qemu-devel] [RFC PATCH 4/4] disas: allow capstone to defer to a fallback function on failure

[Alex Bennée]

Alex Bennée <alex.bennee@linaro.org> writes:

> We can abuse the CS_OPT_SKIPDATA by providing a call back when
> capstone can't disassemble something. The passing of the string to the
> dump function is a little clunky but works.
>
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> ---
>  disas.c             | 30 +++++++++++++++++++++++++++++-
>  include/disas/bfd.h | 11 ++++++++++-
>  target/arm/cpu.c    |  4 ++++
>  3 files changed, 43 insertions(+), 2 deletions(-)
>
> diff --git a/disas.c b/disas.c
> index 5325b7e6be..dfd2c251c5 100644
> --- a/disas.c
> +++ b/disas.c
> @@ -178,6 +178,20 @@ static int print_insn_od_target(bfd_vma pc, disassemble_info *info)
>     to share this across calls and across host vs target disassembly.  */
>  static __thread cs_insn *cap_insn;
>
> +
> +/* Handle fall-back dissasembly. We don't print here but we do set
> + * cap_fallback_str for cap_dump_insn to used*/
> +static size_t cap_disas_fallback(const uint8_t *code, size_t code_size,
> +                                 size_t offset, void *user_data)
> +{
> +    disassemble_info *info = (disassemble_info *) user_data;
> +    info->cap_fallback_str = g_malloc0(256);
> +    size_t skip = info->capstone_fallback_func(code + offset,
> +                                               info->cap_fallback_str, 256);
> +    return skip;
> +}
> +
> +
>  /* Initialize the Capstone library.  */
>  /* ??? It would be nice to cache this.  We would need one handle for the
>     host and one for the target.  For most targets we can reset specific
> @@ -206,6 +220,14 @@ static cs_err cap_disas_start(disassemble_info *info, csh *handle)
>          cs_option(*handle, CS_OPT_SYNTAX, CS_OPT_SYNTAX_ATT);
>      }
>
> +    if (info->capstone_fallback_func) {
> +        cs_opt_skipdata skipdata = {
> +            .callback = cap_disas_fallback,
> +            .user_data = info,

This also needs:

            .mnemonic = "deadbeef",

just to stop the capstone skip handling crashing. For some reason this
only showed up when I started doing Aarch64-to-Aarch64 testing.

> +        };
> +        cs_option(*handle, CS_OPT_SKIPDATA_SETUP, (size_t) &skipdata);
> +    }
> +
>      /* "Disassemble" unknown insns as ".byte W,X,Y,Z".  */
>      cs_option(*handle, CS_OPT_SKIPDATA, CS_OPT_ON);
>
> @@ -281,7 +303,13 @@ static void cap_dump_insn(disassemble_info *info, cs_insn *insn)
>      }
>
>      /* Print the actual instruction.  */
> -    print(info->stream, "  %-8s %s\n", insn->mnemonic, insn->op_str);
> +    if (info->cap_fallback_str) {
> +        print(info->stream, "  %s\n", info->cap_fallback_str);
> +        g_free(info->cap_fallback_str);
> +        info->cap_fallback_str = NULL;
> +    } else {
> +        print(info->stream, "  %-8s %s\n", insn->mnemonic, insn->op_str);
> +    }
>
>      /* Dump any remaining part of the insn on subsequent lines.  */
>      for (i = split; i < n; i += split) {
> diff --git a/include/disas/bfd.h b/include/disas/bfd.h
> index 1f69a6e9d3..9d99bfef48 100644
> --- a/include/disas/bfd.h
> +++ b/include/disas/bfd.h
> @@ -377,6 +377,12 @@ typedef struct disassemble_info {
>    int cap_insn_unit;
>    int cap_insn_split;
>
> +  /* Fallback function to disassemble things capstone can't. */
> +  size_t (*capstone_fallback_func)
> +    (const uint8_t *insn, char *ptr, size_t n);
> +
> +  char *cap_fallback_str;
> +
>  } disassemble_info;
>
>  \f

> @@ -491,7 +497,10 @@ int generic_symbol_at_address(bfd_vma, struct disassemble_info *);
>    (INFO).bytes_per_chunk = 0, \
>    (INFO).display_endian = BFD_ENDIAN_UNKNOWN, \
>    (INFO).disassembler_options = NULL, \
> -  (INFO).insn_info_valid = 0
> +  (INFO).insn_info_valid = 0, \
> +  (INFO).capstone_fallback_func = NULL, \
> +  (INFO).cap_fallback_str = NULL
> +
>
>  #ifndef ATTRIBUTE_UNUSED
>  #define ATTRIBUTE_UNUSED __attribute__((unused))
> diff --git a/target/arm/cpu.c b/target/arm/cpu.c
> index 64a8005a4b..cfefbfb0b9 100644
> --- a/target/arm/cpu.c
> +++ b/target/arm/cpu.c
> @@ -519,6 +519,10 @@ static void arm_disas_set_info(CPUState *cpu, disassemble_info *info)
>          info->cap_arch = CS_ARCH_ARM64;
>          info->cap_insn_unit = 4;
>          info->cap_insn_split = 4;
> +
> +#if defined(TARGET_AARCH64)
> +        info->capstone_fallback_func = do_aarch64_fallback_disassembly;
> +#endif
>      } else {
>          int cap_mode;
>          if (env->thumb) {


--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH 1/4] scripts/decodetree.py: add a disassembly generator (HACK!)

[Eduardo Habkost]

On Wed, Aug 08, 2018 at 01:39:31PM +0100, Alex Bennée wrote:
> Given our issues with failing disassembly we could try and re-use the
> decode tree data to output what instruction is being decoded. This
> will be used if registered as a fall-back for when the "proper"
> disassembler fails to decode an instruction.
> 
> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>

I don't have an opinion on the approach you are taking, but the
Python code you are adding is consistent with the existing style
of the script.

That said, I find the existing code full of output() calls very
hard to read.  If anybody wants to volunteer to improve the
readability of the output generation, it would be welcome.

Acked-by: Eduardo Habkost <ehabkost@redhat.com>

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH 1/4] scripts/decodetree.py: add a disassembly generator (HACK!)

[Alex Bennée]

Eduardo Habkost <ehabkost@redhat.com> writes:

> On Wed, Aug 08, 2018 at 01:39:31PM +0100, Alex Bennée wrote:
>> Given our issues with failing disassembly we could try and re-use the
>> decode tree data to output what instruction is being decoded. This
>> will be used if registered as a fall-back for when the "proper"
>> disassembler fails to decode an instruction.
>>
>> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
>
> I don't have an opinion on the approach you are taking, but the
> Python code you are adding is consistent with the existing style
> of the script.
>
> That said, I find the existing code full of output() calls very
> hard to read.  If anybody wants to volunteer to improve the
> readability of the output generation, it would be welcome.

If we expect to have the script output a number of different forms I
guess re-factoring it with some sort of template based approach would be
worthwhile. I wonder if there are other examples in the code base? We
wouldn't want to have multiple template types.

>
> Acked-by: Eduardo Habkost <ehabkost@redhat.com>


--
Alex Bennée

Re: [Qemu-devel] [RFC PATCH 1/4] scripts/decodetree.py: add a disassembly generator (HACK!)

[Eduardo Habkost]

On Fri, Aug 10, 2018 at 09:55:50AM +0100, Alex Bennée wrote:
> 
> Eduardo Habkost <ehabkost@redhat.com> writes:
> 
> > On Wed, Aug 08, 2018 at 01:39:31PM +0100, Alex Bennée wrote:
> >> Given our issues with failing disassembly we could try and re-use the
> >> decode tree data to output what instruction is being decoded. This
> >> will be used if registered as a fall-back for when the "proper"
> >> disassembler fails to decode an instruction.
> >>
> >> Signed-off-by: Alex Bennée <alex.bennee@linaro.org>
> >
> > I don't have an opinion on the approach you are taking, but the
> > Python code you are adding is consistent with the existing style
> > of the script.
> >
> > That said, I find the existing code full of output() calls very
> > hard to read.  If anybody wants to volunteer to improve the
> > readability of the output generation, it would be welcome.
> 
> If we expect to have the script output a number of different forms I
> guess re-factoring it with some sort of template based approach would be
> worthwhile. I wonder if there are other examples in the code base? We
> wouldn't want to have multiple template types.

QAPI scripts also generates C code, and I find them more
readable[1].

I think a true template language would be even better than QAPI's
approach, but I don't see an obvious candidate that would be
worth adding another build dependency to QEMU.


[1] Compare:

def output_decl(self):
    global translate_scope
    global translate_prefix
    output('typedef ', self.base.base.struct_name(),
           ' arg_', self.name, ';\n')
    output(translate_scope, 'bool ', translate_prefix, '_', self.name,
           '(DisasContext *ctx, arg_', self.name,
           ' *a, ', insntype, ' insn);\n')

And:

def gen_visit_members_decl(name):
    return mcgen('''

void visit_type_%(c_name)s_members(Visitor *v, %(c_name)s *obj, Error **errp);
''',
                 c_name=c_name(name))

-- 
Eduardo

Re: [Qemu-devel] [RFC PATCH 0/4] add hand-rolled fallback when capstone fails

[no-reply]

Hi,

This series seems to have some coding style problems. See output below for
more information:

Type: series
Message-id: 20180808123934.17450-1-alex.bennee@linaro.org
Subject: [Qemu-devel] [RFC PATCH 0/4] add hand-rolled fallback when capstone fails

=== TEST SCRIPT BEGIN ===
#!/bin/bash

BASE=base
n=1
total=$(git log --oneline $BASE.. | wc -l)
failed=0

git config --local diff.renamelimit 0
git config --local diff.renames True
git config --local diff.algorithm histogram

commits="$(git log --format=%H --reverse $BASE..)"
for c in $commits; do
    echo "Checking PATCH $n/$total: $(git log -n 1 --format=%s $c)..."
    if ! git show $c --format=email | ./scripts/checkpatch.pl --mailback -; then
        failed=1
        echo
    fi
    n=$((n+1))
done

exit $failed
=== TEST SCRIPT END ===

Updating 3c8cf5a9c21ff8782164d1def7f44bd888713384
Switched to a new branch 'test'
def0f1ec46 disas: allow capstone to defer to a fallback function on failure
08f250b571 target/arm: add a fallback disassemble function
4891a8fbb7 target/arm: move decoder helpers into header
b1e41656ef scripts/decodetree.py: add a disassembly generator (HACK!)

=== OUTPUT BEGIN ===
Checking PATCH 1/4: scripts/decodetree.py: add a disassembly generator (HACK!)...
ERROR: unnecessary whitespace before a quoted newline
#68: FILE: scripts/decodetree.py:503:
+        output('"); \n')

WARNING: line over 80 characters
#85: FILE: scripts/decodetree.py:1009:
+    long_opts = ['decode=', 'translate=', 'output=', 'insnwidth=', 'disassemble']

total: 1 errors, 1 warnings, 98 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.

Checking PATCH 2/4: target/arm: move decoder helpers into header...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#15: 
new file mode 100644

total: 0 errors, 1 warnings, 112 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 3/4: target/arm: add a fallback disassemble function...
WARNING: added, moved or deleted file(s), does MAINTAINERS need updating?
#35: 
new file mode 100644

WARNING: line over 80 characters
#50: FILE: target/arm/disassemble.c:11:
+size_t do_aarch64_fallback_disassembly(const uint8_t *insnp, char *ptr, size_t n)

WARNING: line over 80 characters
#70: FILE: target/arm/internals.h:799:
+size_t do_aarch64_fallback_disassembly(const uint8_t *insn, char *ptr, size_t n);

total: 0 errors, 3 warnings, 39 lines checked

Your patch has style problems, please review.  If any of these errors
are false positives report them to the maintainer, see
CHECKPATCH in MAINTAINERS.
Checking PATCH 4/4: disas: allow capstone to defer to a fallback function on failure...
=== OUTPUT END ===

Test command exited with code: 1


---
Email generated automatically by Patchew [http://patchew.org/].
Please send your feedback to patchew-devel@redhat.com