On Thu, Aug 16, 2018 at 1:51 PM Sagar Ghuge <sagar.gh...@intel.com> wrote:
>
> Adds a new i965 instruction disassemble tool
This looks very good. A few comments about the structure inline.
> Signed-off-by: Sagar Ghuge <sagar.gh...@intel.com>
> ---
> src/intel/Makefile.tools.am | 15 +++
> src/intel/tools/i965_disasm.c | 202 ++++++++++++++++++++++++++++++++++
> src/intel/tools/i965_disasm.h | 46 ++++++++
> src/intel/tools/meson.build | 11 ++
> 4 files changed, 274 insertions(+)
> create mode 100644 src/intel/tools/i965_disasm.c
> create mode 100644 src/intel/tools/i965_disasm.h
>
> diff --git a/src/intel/Makefile.tools.am b/src/intel/Makefile.tools.am
> index 00624084e6..36a3a70a28 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,20 @@ tools_aubinator_error_decode_CFLAGS = \
> $(AM_CFLAGS) \
> $(ZLIB_CFLAGS)
>
> +tools_i965_disasm_SOURCES = \
> + tools/i965_disasm.c \
> + tools/i965_disasm.h
> +
> +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)
> +
Looks good.
> 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..c880559827
> --- /dev/null
> +++ b/src/intel/tools/i965_disasm.c
> @@ -0,0 +1,202 @@
> +/*
> + * 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 "i965_disasm.h"
> +
> +uint64_t INTEL_DEBUG;
> +uint16_t pci_id = 0;
> +FILE *outfile;
> +
> +struct i965_disasm {
> + struct gen_device_info devinfo;
> +};
> +
> +/* Return size of file in bytes pointed by fp */
> +static size_t
> +i965_disasm_get_file_size(FILE *fp)
> +{
> + size_t size = 0;
No need for initialization.
> +
> + 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;
> +}
> +
> +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 }
I think we're missing a single space before the last 0 here for
vertical alignment.
> + };
> +
> + 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 ivb, byt, hsw,
> "
> + "bdw, chv, skl, kbl or bxt\n", optarg);
Rather than listing all the platforms, I'd change this to say "3
letter platform name" like above.
> + /* 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);
> + 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(0);
> + }
> + 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);
> + }
> +
> + i965_disasm_init(&disasm);
> + end = i965_disasm_read_binary(fp, &assembly);
> + i965_disasm_disassemble(disasm, assembly, start, end, outfile);
> +
> + free(binary_path);
> +
> + return EXIT_SUCCESS;
> +}
> +
> +/* Disassemble i965 instructions from buffer assembly
> + * start : starting offset within buffer
> + * end : points to last byte of buffer
> + */
> +void
> +i965_disasm_disassemble(struct i965_disasm *disasm, void *assembly,
> + int start, int end, FILE *out)
> +{
> + brw_disassemble(&disasm->devinfo, assembly, start, end, out);
> +
> + free(assembly);
> +
> + i965_disasm_destroy(disasm);
I would move the free and the i965_disasm_destroy out of this
function. I don't think it should perform those actions.
> +}
> +
> +void
> +i965_disasm_init(struct i965_disasm **disasm)
> +{
> + struct gen_device_info devinfo;
> +
> + if(!gen_get_device_info(pci_id, &devinfo)) {
> + fprintf(outfile, "can't find device information: pci_id=0x%x\n",
> + pci_id);
> + exit(EXIT_FAILURE);
> + }
> +
> + *disasm = i965_disasm_create(&devinfo);
> +}
> +
> +struct i965_disasm *
> +i965_disasm_create(const struct gen_device_info *devinfo)
> +{
> + struct i965_disasm *i965d;
> +
> + i965d = malloc(sizeof *i965d);
> + if (i965d == NULL)
> + return NULL;
> +
> + i965d->devinfo = *devinfo;
> +
> + /* initialize compaction table in order
> + * to handle compacted instructions
> + */
> + brw_init_compaction_tables(&i965d->devinfo);
> +
> + return i965d;
> +}
_create and _init should just be a single function.
> +
> +void
> +i965_disasm_destroy(struct i965_disasm *disasm)
> +{
> + free(disasm);
> +}
> diff --git a/src/intel/tools/i965_disasm.h b/src/intel/tools/i965_disasm.h
> new file mode 100644
> index 0000000000..45cb7a015c
> --- /dev/null
> +++ b/src/intel/tools/i965_disasm.h
> @@ -0,0 +1,46 @@
> +/*
> + * 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.
> + *
> + */
> +
> +#ifndef _I965_DISASM_H
> +#define _I965_DISASM_H
> +
> +#include "dev/gen_device_info.h"
> +
> +#ifdef __cplusplus
> +extern "C" {
> +#endif
> +
> +struct i965_disasm;
> +
> +struct i965_disasm *i965_disasm_create(const struct gen_device_info
> *devinfo);
> +void i965_disasm_disassemble(struct i965_disasm *disasm, void *assembly,
> + int start, int end, FILE *out);
> +void i965_disasm_init(struct i965_disasm **disasm);
> +void i965_disasm_destroy(struct i965_disasm *disasm);
> +
All of these functions are really just internal to i965_disasm.c, and
since it's an executable binary nothing will ever want to link with
it. I would just remove the i965_disasm.h header and mark the
functions static in i965_disasm.c. In addition, it's usually good form
to order static functions in the file so that prototypes are not
necessary (with the main() function ordered last).
Nice work!
_______________________________________________
mesa-dev mailing list
mesa-dev@lists.freedesktop.org
https://lists.freedesktop.org/mailman/listinfo/mesa-dev
