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

[Simon Marchi <simark@simark.ca>]

> diff --git a/gdb/configure.ac b/gdb/configure.ac
> index 226e27e4fe54a9d3ac06e8be4439dd5dce8eb975..0507f03043c61385181b667d0e7117d4b2547d6d 100644
> --- a/gdb/configure.ac
> +++ b/gdb/configure.ac
> @@ -1351,6 +1351,58 @@ fi
>  AC_SUBST(SRCHIGH_LIBS)
>  AC_SUBST(SRCHIGH_CFLAGS)
>  
> +# Check for Intel(R) Graphics Technology assembler library
> +intelgt_target=false
> +
> +for targ_alias in `echo $target_alias $enable_targets | sed 's/,/ /g'`
> +do
> +  if test "$targ_alias" = "all"; then
> +    intelgt_target=true
> +  else
> +    case "$targ_alias" in
> +      intelgt-*)
> +        intelgt_target=true
> +        ;;
> +    esac
> +  fi
> +done
> +
> +case "${target}" in
> +  intelgt-*)
> +    intelgt_target=true
> +    ;;
> +esac

Do you really need this last "case"?  I would think that the main target
would be included in the loop above (as $target_alias).

> +
> +if test "${intelgt_target}" != true; then
> +  HAVE_LIBIGA64=no
> +else
> +  AC_ARG_WITH(libiga64,
> +    AS_HELP_STRING([--with-libiga64], [include IntelGT disassembly support (auto/yes/no)]),
> +    [], [with_libiga64=auto])

Not an autoconf expert, but I think it makes more sense (and is clearer)
to put the AC_ARG_WITH at the top-level.  The --with-libiga64 flag will
always be there, it's not like it will be predicated by $intelgt_target.

> @@ -544,13 +553,75 @@ 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;
> +}
> +#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.  */
> +#if !defined (HAVE_LIBIGA64)
> +  gdb_printf (_("\nDisassemble feature not available: libiga64 "
> +		"is missing.\n"));
>    return -1;
> +#else
> +  std::unique_ptr<bfd_byte[]> insn (new bfd_byte[intelgt::MAX_INST_LENGTH]);

Can this be statically allocated?

  bfd_byte insn[intelgt::MAX_INST_LENGTH];

> +
> +  int status = (*info->read_memory_func) (memaddr, insn.get (),
> +					  intelgt::COMPACT_INST_LENGTH, info);
> +  if (status != 0)
> +    {
> +      /* Aborts disassembling with a memory_error exception.  */
> +      (*info->memory_error_func) (status, memaddr, info);
> +      return -1;
> +    }
> +
> +  uint32_t device_id = get_device_id (current_inferior ());
> +  gdb::array_view<bfd_byte> insn_view
> +    = gdb::make_array_view (insn.get (), intelgt::COMPACT_INST_LENGTH);
> +  unsigned int length = intelgt::inst_length (insn_view, device_id);
> +
> +  if (length == intelgt::MAX_INST_LENGTH)
> +    {
> +      status = (*info->read_memory_func) (memaddr, insn.get (),
> +					  intelgt::MAX_INST_LENGTH, info);
> +      if (status != 0)
> +	{
> +	  /* Aborts disassembling with a memory_error exception.  */
> +	  (*info->memory_error_func) (status, memaddr, info);
> +	  return -1;
> +	}
> +    }
> +
> +  char *dbuf;
> +  iga_disassemble_options_t dopts = IGA_DISASSEMBLE_OPTIONS_INIT ();
> +  gdb_disassemble_info *di
> +    = static_cast<gdb_disassemble_info *>(info->application_data);

Space before paren.

> @@ -771,6 +842,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."));

Perhaps say "... not found in target description."?  Trying to think
about a clueless user seeing this message without much context.

> +	  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;

I don't think these two gdbarch_free are needed, that is managed by
gdbarch_u.

> +	}
> +      else

I would remove this "else" (just de-indent the code below).

> +	{
> +	  iga_version = (iga_gen_t) intelgt::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);

Should probably check the return value of iga_context_create.

Simon