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

["Aktemur, Tankut Baris" <tankut.baris.aktemur@intel.com>]

Hi Simon,

On Thursday, December 11, 2025 8:37 PM, Simon Marchi wrote:
> > diff --git a/gdb/configure.ac b/gdb/configure.ac
> > index
> 226e27e4fe54a9d3ac06e8be4439dd5dce8eb975..0507f03043c61385181b667d0e7117
> d4b2547d6d 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).

No, this shouldn't be needed.  We'll remove it in the next revision.

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

We'll move it outside 'else'.

> > @@ -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];

Yes.

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

Fixed.

> > @@ -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.

Updating with "... not found in intelgt target description."

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

Thanks.  These are leftovers from the time when gdbarch_u was not used.
Removed.
 
> > +	}
> > +      else
> 
> I would remove this "else" (just de-indent the code below).

Done.

> > +	{
> > +	  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.

Done.

> 
> Simon

Thank you.
-Baris


Intel Deutschland GmbH
Registered Address: Dornacher Straße 1, 85622 Feldkirchen, Germany
Tel: +49 89 991 430, www.intel.de
Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell
Chairperson of the Supervisory Board: Nicole Lau
Registered Seat: Munich
Commercial Register: Amtsgericht München HRB 186928