Hey Matt,

Thanks for reviewing the patch. 

On 08/27/2018 11:06 AM, Matt Turner wrote:
> On Mon, Aug 20, 2018 at 3:13 PM Sagar Ghuge <sagar.gh...@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.gh...@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.
> I agree. 
>> +
>> +/* 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.
> 
Don't be. I will keep reworking the patches until it gets perfect. Soon I will 
resend next version of patch with all the changes. 
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev

Reply via email to