Re: [PATCH v2 08/47] gdb, intelgt: add disassemble feature for the Intel GT architecture.

[Thiago Jung Bauermann <thiago.bauermann@linaro.org>]

Tankut Baris Aktemur <tankut.baris.aktemur@intel.com> writes:

> From: Albertano Caruso <albertano.caruso@intel.com>
>
> Disassembly of Intel GT programs is done via the Intel Gen Assembler
> library.  Add this library as an optional dependency.  If the library
> is not found, disassembly is disabled.

There should be information about this optional dependency in the GDB
documentation.

It should be in the "Requirements for Building GDB" section of the
manual, and/or (not sure which) in a new "Intel GPU" section under
"Configuration-Specific Information".

For example, one important piece of information that is missing is a
link to where one can get the Intel Gen Assembler library.

> diff --git a/gdb/intelgt-tdep.c b/gdb/intelgt-tdep.c
> index 57c359bf355c5771db38b8d213f6681a043c2b33..7d0ae38480fe89b145c1c29be0e58c96480e3335 100755
> --- a/gdb/intelgt-tdep.c
> +++ b/gdb/intelgt-tdep.c
> @@ -31,6 +31,10 @@
>  #include "inferior.h"
>  #include "user-regs.h"
>  #include <algorithm>
> +#include "disasm.h"
> +#if defined (HAVE_LIBIGA64)
> +#include "iga/iga.h"
> +#endif /* defined (HAVE_LIBIGA64)  */
>  
>  /* Global debug flag.  */
>  static bool intelgt_debug = false;
> @@ -119,6 +123,11 @@ struct intelgt_gdbarch_tdep : gdbarch_tdep_base
>      gdb_assert (regset_ranges[intelgt::REGSET_GRF].end > 1);
>      return regset_ranges[intelgt::REGSET_GRF].end - 1;
>    }
> +
> +#if defined (HAVE_LIBIGA64)
> +  /* libiga context for disassembly.  */
> +  iga_context_t iga_ctx = nullptr;
> +#endif
>  };
>  
>  /* The 'register_type' gdbarch method.  */
> @@ -511,13 +520,77 @@ intelgt_sw_breakpoint_from_kind (gdbarch *gdbarch, int kind, int *size)
>    return nullptr;
>  }
>  
> +#if defined (HAVE_LIBIGA64)
> +/* Map CORE_ADDR to symbol names for jump labels in an IGA disassembly.  */
> +
> +static const char *
> +intelgt_disasm_sym_cb (int addr, void *ctx)
> +{
> +  disassemble_info *info = (disassemble_info *) ctx;
> +  symbol *sym = find_pc_function (addr + (uintptr_t) info->private_data);
> +  return sym ? sym->linkage_name () : nullptr;
> +}

As I mentioned in my review of patch 6, I think you could put the
definition of get_xe_version inside an #if/#endif such as this one.

> +#endif /* defined (HAVE_LIBIGA64)  */
> +
>  /* Print one instruction from MEMADDR on INFO->STREAM.  */
>  
>  static int
>  intelgt_print_insn (bfd_vma memaddr, struct disassemble_info *info)
>  {
> -  /* Disassembler is to be added in a later patch.  */
> +  unsigned int full_length = intelgt::inst_length_full ();
> +  unsigned int compact_length = intelgt::inst_length_compacted ();
> +
> +  std::unique_ptr<bfd_byte[]> insn (new bfd_byte[full_length]);
> +
> +  int status = (*info->read_memory_func) (memaddr, insn.get (),
> +					  compact_length, info);
> +  if (status != 0)
> +    {
> +      /* Aborts disassembling with a memory_error exception.  */
> +      (*info->memory_error_func) (status, memaddr, info);
> +      return -1;
> +    }
> +  if (!intelgt::is_compacted_inst (gdb::make_array_view (insn.get (),
> +							 compact_length)))
> +    {
> +      status = (*info->read_memory_func) (memaddr, insn.get (),
> +					  full_length, info);
> +      if (status != 0)
> +	{
> +	  /* Aborts disassembling with a memory_error exception.  */
> +	  (*info->memory_error_func) (status, memaddr, info);
> +	  return -1;
> +	}
> +    }

If GDB wasn't compiled with libiga64, then the only thing that this
function can do is print an error message and return -1, as is done in
the #else block below.

Therefore, all the code above should be inside the #if below.

> +#if defined (HAVE_LIBIGA64)
> +  char *dbuf;
> +  iga_disassemble_options_t dopts = IGA_DISASSEMBLE_OPTIONS_INIT ();
> +  gdb_disassemble_info *di
> +    = static_cast<gdb_disassemble_info *>(info->application_data);
> +  struct gdbarch *gdbarch = di->arch ();
> +
> +  iga_context_t iga_ctx
> +    = gdbarch_tdep<intelgt_gdbarch_tdep> (gdbarch)->iga_ctx;
> +  iga_status_t iga_status
> +    = iga_context_disassemble_instruction (iga_ctx, &dopts, insn.get (),
> +					   intelgt_disasm_sym_cb,
> +					   info, &dbuf);
> +  if (iga_status != IGA_SUCCESS)
> +    return -1;
> +
> +  (*info->fprintf_func) (info->stream, "%s", dbuf);
> +
> +  if (intelgt::is_compacted_inst (gdb::make_array_view (insn.get (),
> +							full_length)))
> +    return compact_length;
> +  else
> +    return full_length;
> +#else
> +  gdb_printf (_("\nDisassemble feature not available: libiga64 "
> +		"is missing.\n"));

Is the '\n' at the beginning of the message needed?

>    return -1;
> +#endif /* defined (HAVE_LIBIGA64)  */
>  }
>  
>  /* Utility function to look up the pseudo-register number by name.  Exact
> @@ -864,6 +937,46 @@ intelgt_gdbarch_init (gdbarch_info info, gdbarch_list *arches)
>    intelgt_gdbarch_tdep *data
>      = gdbarch_tdep<intelgt_gdbarch_tdep> (gdbarch);
>  
> +#if defined (HAVE_LIBIGA64)
> +  iga_gen_t iga_version = IGA_GEN_INVALID;
> +
> +  if (tdesc != nullptr)
> +    {
> +      const tdesc_device *device_info = tdesc_device_info (tdesc);
> +      if (!(device_info->vendor_id.has_value ()
> +	    && device_info->target_id.has_value ()))
> +	{
> +	  warning (_("Device vendor id and target id not found."));
> +	  gdbarch_free (gdbarch);
> +	  return nullptr;
> +	}
> +
> +      uint32_t vendor_id = *device_info->vendor_id;
> +      uint32_t device_id = *device_info->target_id;
> +      if (vendor_id != 0x8086)
> +	{
> +	  warning (_("Device not recognized: vendor id=0x%04x,"
> +		     " device id=0x%04x"), vendor_id, device_id);
> +	  gdbarch_free (gdbarch);
> +	  return nullptr;
> +	}
> +      else
> +	{
> +	  iga_version = (iga_gen_t) get_xe_version (device_id);
> +	  if (iga_version == IGA_GEN_INVALID)
> +	    warning (_("Intel GT device id is unrecognized: ID 0x%04x"),
> +		     device_id);
> +	}
> +    }
> +
> +  /* Take the best guess in case IGA_VERSION is still invalid.  */
> +  if (iga_version == IGA_GEN_INVALID)
> +    iga_version = IGA_XE_HPC;
> +
> +  const iga_context_options_t options = IGA_CONTEXT_OPTIONS_INIT (iga_version);
> +  iga_context_create (&options, &data->iga_ctx);
> +#endif
> +
>    /* Initialize register info.  */
>    set_gdbarch_num_regs (gdbarch, 0);
>    set_gdbarch_register_name (gdbarch, tdesc_register_name);
> diff --git a/gdb/top.c b/gdb/top.c
> index d750f3305f2eef7c5a30cef7f647537916a80499..c27f7d60254a4d384dfd770ce98eb4f0923dee49 100644
> --- a/gdb/top.c
> +++ b/gdb/top.c
> @@ -1523,6 +1523,16 @@ This GDB was configured as follows:\n\
>  "));
>  #endif
>  
> +#ifdef HAVE_LIBIGA64
> +  gdb_printf (stream, _("\
> +	     --with-libiga64\n\
> +"));

This patch adds a "--with-libiga64-prefix=..." configure option, right?
So this should print "--with-libiga64-prefix=...".

> +#else
> +  gdb_printf (stream, _("\
> +	     --without-libiga64\n\
> +"));
> +#endif

-- 
Thiago