RE: [PATCH v2 06/47] gdb, intelgt: add the target-dependent definitions for the Intel GT architecture

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

Hi Thiago,

On Tuesday, July 8, 2025 4:43 AM, Thiago Jung Bauermann wrote:
> Hello Baris,
> 
> Reviewing this patch series has been on my todo list for a long
> time. I'll try to go through it this week.

Much appreciated.  Thank you.
Below I respond to a number of comments.  For all the other comments, I updated the
code locally and will include the change in the next version.

> > diff --git a/gdb/intelgt-tdep.c b/gdb/intelgt-tdep.c
> > new file mode 100755
> > index 0000000000000000000000000000000000000000..57c359bf355c5771db38b8d213f6681a043c2b33
> > --- /dev/null
> > +++ b/gdb/intelgt-tdep.c
> > @@ -0,0 +1,980 @@
> > +/* Target-dependent code for the Intel(R) Graphics Technology architecture.
> > +
> > +   Copyright (C) 2019-2024 Free Software Foundation, Inc.
> 
> Does the copyright on this file really start on 2019?

Yes, 2019 is correct.

> > +
> > +/* Helper functions to request and translate the device id/version.  */
> > +
> > +[[maybe_unused]] static xe_version get_xe_version (unsigned int device_id);
> 
> If the definition of get_xe_version is guarded by "#ifdef
> HAVE_LIBIGA64", then this prototype isn't needed.
> 
> Also, the definition of get_xe_version can be moved to patch 8 ("gdb,
> intelgt: add disassemble feature for the Intel GT architecture."), which
> uses it.

We moved the definition to arch/intelgt.h and .c.  There are/will be uses in
gdbserver/intelgt-ze-low.cc, too, in the next version after some refactoring.

> > +/* The 'unwind_pc' gdbarch method.  */
> > +
> > +static CORE_ADDR
> > +intelgt_unwind_pc (gdbarch *gdbarch, const frame_info_ptr &next_frame)
> > +{
> > +  /* Use ip register here, as IGC uses 32bit values (pc is 64bit).  */
> > +  int ip_regnum = intelgt_pseudo_register_num (gdbarch, "ip");
> > +  CORE_ADDR prev_ip = frame_unwind_register_unsigned (next_frame,
> > +						      ip_regnum);
> 
> There's no need to break the line above. Everything fits in one line.

It goes until column 77, which was beyond the soft limit.  Hence it was broken.
Similarly, for some

  intelgt_gdbarch_tdep *data
    = gdbarch_tdep<intelgt_gdbarch_tdep> (gdbarch);

cases for which you commented, the line goes up to column 76 if not broken.
I kept them as is.  For others, I removed the line break.
(This does not mean, however, that the file always follows the soft-limit rule.)

> > +/* Frame unwinding.  */
> > +
> > +static void
> > +intelgt_frame_this_id (const frame_info_ptr &this_frame,
> > +		       void **this_prologue_cache, frame_id *this_id)
> > +{
> > +  /* FIXME: Assembly-level unwinding for intelgt is not available at
> > +     the moment.  Stop at the first frame.  */
> > +  *this_id = outer_frame_id;
> > +}
> > +
> > +static const struct frame_unwind intelgt_unwinder =
> > +  {
> > +    "intelgt prologue",
> > +    NORMAL_FRAME,			/* type */
> > +    default_frame_unwind_stop_reason,	/* stop_reason */
> > +    intelgt_frame_this_id,		/* this_id */
> > +    nullptr,				/* prev_register */
> > +    nullptr,				/* unwind_data */
> > +    default_frame_sniffer,		/* sniffer */
> > +    nullptr,				/* dealloc_cache */
> > +  };
> 
> This unwinder doesn't do much. Is it necessary? If so, I suggest a
> comment explaining why.

Without this, if a program that has no debug info is debugged, GDB crashes with

  Recursive internal problem.

This unwinder avoids that problem.  I'll add a comment.

> > +static int
> > +intelgt_memory_remove_breakpoint (gdbarch *gdbarch, struct bp_target_info *bp)
> > +{
> > +  intelgt_debug_printf ("req ip: %s, placed ip: %s",
> > +			paddress (gdbarch, bp->reqstd_address),
> > +			paddress (gdbarch, bp->placed_address));
> > +
> > +  /* Warn if we're inserting a permanent breakpoint.  */
> > +  if (intelgt::has_breakpoint (bp->shadow_contents))
> > +    warning (_("Re-inserting permanent breakpoint at %s."),
> > +	     paddress (gdbarch, bp->placed_address));
> > +
> > +  /* See comment in mem-break.c on write_inferior_memory.  */
> 
> I wasn't able to find that comment. Has it been removed from GDB?

Seems like so.  I removed the dangling reference to the comment.

> > +  return data->enabled_pseudo_regs[regnum - base_num].c_str ();
> > +}
> > +
> > +/* Return the GDB type object for the "standard" data type of data in
> > +   pseudo-register REGNUM.  */
> > +
> > +static type *
> > +intelgt_pseudo_register_type (gdbarch *arch, int regnum)
> > +{
> > +  const char *name = intelgt_pseudo_register_name (arch, regnum);
> > +  const struct builtin_type *bt = builtin_type (arch);
> > +  intelgt_gdbarch_tdep *data
> > +    = gdbarch_tdep<intelgt_gdbarch_tdep> (arch);
> > +
> > +  if (strcmp (name, "framedesc") == 0)
> > +    {
> > +      if (data->framedesc_type != nullptr)
> > +	return data->framedesc_type;
> > +      type *frame = arch_composite_type (arch, "frame_desc", TYPE_CODE_STRUCT);
> > +      append_composite_type_field (frame, "return_ip", bt->builtin_uint32);
> 
> Isn't it better to use a function pointer type? This is what x86_64 and
> aarch64 do:
> 
> (gdb) ptype $pc
> type = void (*)()
> 
> Though I see further below that pointer and address lengths in gdbarch
> are set to 64 bits, so perhaps things aren't very straightforward in
> this architecture...

An "IP" is a 32b offset that has to be combined with a 64b base address to
refer to an actual instruction.  Hence, we preferred to use the plain 32b type.
 
> > +      append_composite_type_field (frame, "return_callmask",
> > +				   bt->builtin_uint32);
> > +      append_composite_type_field (frame, "be_sp", bt->builtin_uint32);
> > +      append_composite_type_field (frame, "be_fp", bt->builtin_uint32);
> > +      append_composite_type_field (frame, "fe_fp", bt->builtin_uint64);
> > +      append_composite_type_field (frame, "fe_sp", bt->builtin_uint64);
> 
> If the 'p' in the fields above mean "pointer", would a void pointer
> work? This is what x86_64 and aarch64 do:
> 
> (gdb) ptype $sp
> type = void *

The fe_sp and fe_fp fields are actual addresses.  It makes sense to use
a void pointer type for them.  I updated their types.  The be_ fields,
however, are 32b values that are meaningful only in combination with other
data.  Let me not change them.

> > +      data->framedesc_type = frame;
> > +      return frame;
> > +    }
> > +  else if (strcmp (name, "ip") == 0)
> > +    return bt->builtin_uint32;
> 
> Same note about using a function pointer type as in return_ip above.

Yes, this is again a 32b value that, as is, does not really point to anything.
After being combined with a base address, it becomes a "PC".

> > +      /* Unconditionally enabled pseudo-registers:  */
> > +      data->enabled_pseudo_regs.push_back ("ip");
> > +      data->enabled_pseudo_regs.push_back ("framedesc");
> > +
> > +      set_gdbarch_num_pseudo_regs (gdbarch, data->enabled_pseudo_regs.size ());
> > +      set_gdbarch_pseudo_register_read_value (
> > +	  gdbarch, intelgt_pseudo_register_read_value);
> > +      set_gdbarch_pseudo_register_write (gdbarch,
> > +					 intelgt_pseudo_register_write);
> > +      set_tdesc_pseudo_register_type (gdbarch, intelgt_pseudo_register_type);
> > +      set_tdesc_pseudo_register_name (gdbarch, intelgt_pseudo_register_name);
> > +      set_gdbarch_read_pc (gdbarch, intelgt_read_pc);
> > +      set_gdbarch_write_pc (gdbarch, intelgt_write_pc);
> > +    }
> > +
> > +  /* Populate gdbarch fields.  */
> > +  set_gdbarch_ptr_bit (gdbarch, 64);
> > +  set_gdbarch_addr_bit (gdbarch, 64);
> 
> I'm a bit surprised by these, considering that PC is 32 bits. If this is
> correct, then it's worth adding an explanation about the discrepancy.
> Is this why PC is uint32 instead of a pointer type?

PC is still 64b.  The hardware provides a 32b subregister (CR0.2, to be precise)
which is added to a base address called "isabase", to obtain the PC.
The 32b value in CR0.2 is provided as a pseudo register named "ip" for convenience.
intelgt_read_pc and intelgt_write_pc functions compose and decompose PC this way.
Do you think such a comment should be written here?
 
> > +/* Dump the target specific data for this architecture.  */
> > +
> > +static void
> > +intelgt_dump_tdep (gdbarch *gdbarch, ui_file *file)
> > +{
> > +  /* Implement target-specific print output if and
> > +     when gdbarch_tdep is defined for this architecture.  */
> 
> From looking at gdbarch_dump in gdb/gdbarch-gen.c, you can just pass
> nullptr to gdbarch_register instead of having to define a dummy function.
> 
> Though the comment isn't accurate. There is intelgt_gdbarch_tdep, but
> perhaps it's not necessary to dump any of its values?

As far as I remember, it used to be required to pass a non-null function pointer.
It doesn't seem to be case anymore.  Thanks for noting this.  Removing the
intelgt_dump_tdep function.

> > +void _initialize_intelgt_tdep ();
> > +void
> > +_initialize_intelgt_tdep ()
> 
> Update to current style using INIT_GDB_FILE macro.

I couldn't follow.  Could you please clarify or point to an example?

-Baris


Intel Deutschland GmbH
Registered Address: Am Campeon 10, 85579 Neubiberg, Germany
Tel: +49 89 99 8853-0, www.intel.de
Managing Directors: Sean Fennelly, Jeffrey Schneiderman, Tiffany Doon Silva
Chairperson of the Supervisory Board: Nicole Lau
Registered Office: Munich
Commercial Register: Amtsgericht Muenchen HRB 186928