[Mesa-dev] [PATCH v2] intel/tools: new i965_disasm tool

[Matt Turner]

On Mon, Aug 20, 2018 at 3:13 PM Sagar Ghuge <sagar.ghuge at intel.com> wrote:
>
> Adds a new i965 instruction disassemble tool
>
> v2: 1) fix a few nits (Matt Turner)
>     2) Remove i965_disasm header (Matt Turner)
>
> Signed-off-by: Sagar Ghuge <sagar.ghuge at intel.com>
> ---
>  src/intel/Makefile.tools.am   |  14 +++
>  src/intel/tools/i965_disasm.c | 199 ++++++++++++++++++++++++++++++++++
>  src/intel/tools/meson.build   |  11 ++
>  3 files changed, 224 insertions(+)
>  create mode 100644 src/intel/tools/i965_disasm.c
>
> diff --git a/src/intel/Makefile.tools.am b/src/intel/Makefile.tools.am
> index 00624084e6..385819abc2 100644
> --- a/src/intel/Makefile.tools.am
> +++ b/src/intel/Makefile.tools.am
> @@ -22,6 +22,7 @@
>  noinst_PROGRAMS += \
>         tools/aubinator \
>         tools/aubinator_error_decode \
> +       tools/i965_disasm \
>         tools/error2aub
>
>
> @@ -62,6 +63,19 @@ tools_aubinator_error_decode_CFLAGS = \
>         $(AM_CFLAGS) \
>         $(ZLIB_CFLAGS)
>
> +tools_i965_disasm_SOURCES = \
> +       tools/i965_disasm.c
> +
> +tools_i965_disasm_LDADD = \
> +       common/libintel_common.la \
> +       compiler/libintel_compiler.la \
> +       dev/libintel_dev.la \
> +       $(top_builddir)/src/util/libmesautil.la \
> +       $(PTHREAD_LIBS)
> +
> +tools_i965_disasm_CFLAGS = \
> +       $(AM_CFLAGS)
> +
>
>  tools_error2aub_SOURCES = \
>         tools/gen_context.h \
> diff --git a/src/intel/tools/i965_disasm.c b/src/intel/tools/i965_disasm.c
> new file mode 100644
> index 0000000000..757d2c7db1
> --- /dev/null
> +++ b/src/intel/tools/i965_disasm.c
> @@ -0,0 +1,199 @@
> +/*
> + * Copyright © 2018 Intel Corporation
> + *
> + * Permission is hereby granted, free of charge, to any person obtaining a
> + * copy of this software and associated documentation files (the "Software"),
> + * to deal in the Software without restriction, including without limitation
> + * the rights to use, copy, modify, merge, publish, distribute, sublicense,
> + * and/or sell copies of the Software, and to permit persons to whom the
> + * Software is furnished to do so, subject to the following conditions:
> + *
> + * The above copyright notice and this permission notice (including the next
> + * paragraph) shall be included in all copies or substantial portions of the
> + * Software.
> + *
> + * THE SOFTWARE IS PROVIDED "AS IS", WITHOUT WARRANTY OF ANY KIND, EXPRESS OR
> + * IMPLIED, INCLUDING BUT NOT LIMITED TO THE WARRANTIES OF MERCHANTABILITY,
> + * FITNESS FOR A PARTICULAR PURPOSE AND NONINFRINGEMENT.  IN NO EVENT SHALL
> + * THE AUTHORS OR COPYRIGHT HOLDERS BE LIABLE FOR ANY CLAIM, DAMAGES OR OTHER
> + * LIABILITY, WHETHER IN AN ACTION OF CONTRACT, TORT OR OTHERWISE, ARISING
> + * FROM, OUT OF OR IN CONNECTION WITH THE SOFTWARE OR THE USE OR OTHER DEALINGS
> + * IN THE SOFTWARE.
> + */
> +
> +#include <stdio.h>
> +#include <getopt.h>
> +
> +#include "compiler/brw_inst.h"
> +#include "compiler/brw_eu.h"
> +#include "dev/gen_device_info.h"
> +
> +uint64_t INTEL_DEBUG;
> +uint16_t pci_id = 0;
> +FILE *outfile;

I expected to find code that opens a file and sets outfile, but
outfile is only set to stderr. I'd either add a -o option or get rid
of the outfile variable entirely. Either way works, but in both cases
error messages should be printed to stderr instead of outfile since
you wouldn't want errors intermixed with the disassembly.

So either:

   1) add a -o out option (defaulting outfile to stdout) and change
errors to be printed to stderr; or

   2) remove the outfile variable, output errors to stderr and
disassembly to stdout

If you choose to keep outfile, I think it can become a local variable in main().

> +
> +struct i965_disasm {
> +    struct gen_device_info devinfo;
> +};

I think we should simplify some things by removing the i965_disasm type.

> +
> +/* Return size of file in bytes pointed by fp */
> +static size_t
> +i965_disasm_get_file_size(FILE *fp)
> +{
> +   size_t size;
> +
> +   fseek(fp, 0L, SEEK_END);
> +   size = ftell(fp);
> +   fseek(fp, 0L, SEEK_SET);
> +
> +   return size;
> +}
> +
> +/* Return number of bytes read */
> +static size_t
> +i965_disasm_read_binary(FILE *fp, void **assembly)
> +{
> +   size_t end = i965_disasm_get_file_size(fp);
> +   *assembly = malloc(end + 1);
> +   fread(*assembly, end, 1, fp);
> +   fclose(fp);
> +
> +   return end;
> +}

I think it looks more natural to return assembly and return 'end' via
an out-parameter. I think we do that elsewhere in the compiler or i965
driver but I can't find where at the moment.

> +
> +/* Disassemble i965 instructions from buffer assembly
> + * start : starting offset within buffer
> + * end : points to last byte of buffer
> + */
> +static void
> +i965_disasm_disassemble(struct i965_disasm *disasm, void *assembly,
> +                        int start, int end, FILE *out)
> +{
> +   brw_disassemble(&disasm->devinfo, assembly, start, end, out);
> +}

I would remove this function and call brw_disassemble directly.

> +
> +static struct i965_disasm *
> +i965_disasm_init(void)

I would pass the pciid as an argument to this function, which will
allow it to be a local variable in main().

> +{
> +   struct gen_device_info devinfo;
> +   struct i965_disasm *i965d;
> +
> +   i965d = malloc(sizeof *i965d);
> +   if (i965d == NULL)
> +      return NULL;
> +
> +   if(!gen_get_device_info(pci_id, &devinfo)) {

Space after if

> +      fprintf(outfile, "can't find device information: pci_id=0x%x\n",
> +              pci_id);
> +      exit(EXIT_FAILURE);
> +   }
> +
> +   i965d->devinfo = devinfo;

We're saving a pointer to a stack-allocated devinfo. Let's make this
function return a devinfo pointer directly, which is malloc'd by this
function.

> +
> +   /* initialize compaction table in order
> +    * to handle compacted instructions
> +    */
> +   brw_init_compaction_tables(&i965d->devinfo);
> +
> +   return i965d;
> +}
> +
> +static void
> +i965_disasm_destroy(struct i965_disasm *disasm)
> +{
> +   free(disasm);

And we can remove this function and free(devinfo) directly.

> +}
> +
> +static void
> +print_help(const char *progname, FILE *file)
> +{
> +   fprintf(file,
> +           "Usage: %s [OPTION]...\n"
> +           "Disassemble i965 instructions from binary file.\n\n"
> +           "      --help             display this help and exit\n"
> +           "      --binary-path=PATH read binary file from binary file PATH\n"
> +           "      --gen=platform     disassemble instructions for given \n"
> +           "                         platform (3 letter platform name)\n",
> +           progname);
> +}
> +
> +int main(int argc, char *argv[])
> +{
> +   FILE *fp = NULL;
> +   void *assembly = NULL;
> +   char *binary_path = NULL;
> +   size_t start = 0, end = 0;
> +   int c, i;
> +   struct i965_disasm *disasm;
> +
> +   bool help = false;
> +   const struct option i965_disasm_opts[] = {
> +      { "help",          no_argument,       (int *) &help,      true },
> +      { "binary-path",   required_argument, NULL,               'b' },
> +      { "gen",           required_argument, NULL,               'g'},
> +      { NULL,            0,                 NULL,                0 }
> +   };
> +
> +   outfile = stderr;
> +   i = 0;
> +   while ((c = getopt_long(argc, argv, "", i965_disasm_opts, &i)) != -1) {
> +      switch (c) {
> +      case 'g': {
> +         const int id = gen_device_name_to_pci_device_id(optarg);
> +         if (id < 0) {
> +            fprintf(outfile, "can't parse gen: '%s', expected 3 letter "
> +                                   "platform name\n", optarg);

Vertically align strings.

> +            /* Clean up binary path if given pci id is wrong */
> +            if (binary_path) {
> +               free(binary_path);
> +               fclose(fp);
> +            }
> +            exit(EXIT_FAILURE);
> +         } else {
> +            pci_id = id;
> +         }
> +         break;
> +      }
> +      case 'b':
> +         binary_path = strdup(optarg);

I just learned something new -- I didn't realize optarg must be
strdup()'d. It's too bad that the man page doesn't specify that
clearly. Cool.

> +         fp = fopen(binary_path, "rb");
> +         if (!fp) {
> +            fprintf(outfile, "Unable to read input binary file : %s\n",
> +                    binary_path);
> +            /* free binary_path if path is wrong */
> +            free(binary_path);
> +            exit(EXIT_FAILURE);
> +         }
> +         break;
> +      default:
> +         /* Clean up binary path if given option is wrong */
> +         if (binary_path) {
> +            free(binary_path);
> +            fclose(fp);
> +         }
> +         break;
> +      }
> +   }
> +
> +   if (help || !binary_path || !pci_id) {
> +      print_help(argv[0], outfile);
> +      exit(0);
> +   }
> +
> +   disasm = i965_disasm_init();
> +
> +   if (!disasm) {
> +      fprintf(outfile, "Unable to allocate memory for "
> +                        " i965_disasm struct instance.\n");

Vertically align strings.

I feel like I should have been able to give you all this review the
first time around. Sorry for that.