RE: [PATCH v3 07/44] gdb, intelgt: add the target-dependent definitions for the Intel GT architecture

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

On Thursday, December 11, 2025 7:54 PM, Simon Marchi wrote:
> On 2025-08-01 05:37, Tankut Baris Aktemur wrote:
> > Introduce gdb/intelgt-tdep.c for the Intel GT target.  The target is
> > defined in a future patch as a gdbserver low target implementation.
> >
> > Unlike, for example, X86-64, IntelGT does not have a dedicated
> > breakpoint instruction.  Instead, it has a breakpoint bit in each
> > instruction.  Hence, any instruction can be used as a breakpoint
> > instruction.
> >
> > It further supports ignoring breakpoints for stepping over or resuming
> > from a breakpoint.  This only works, of course, if we use breakpoint
> > bits inside the original instruction rather than replacing it with a
> > fixed breakpoint instruction.
> >
> > Add gdbarch methods for inserting and removing memory breakpoints by
> > setting and clearing those breakpoint bits.
> >
> > We define one pseudo-register, $framedesc, that provides a type alias
> > for the frame descriptor register in GRF, representing it as a struct.
> >
> > The value of the $pc register is the $ip register, which is provided
> > in $cr0.2, offset by $isabase.
> >
> > A translation function to map the device_id to a gen_version is
> > provided.  This will be useful in various places, in particular to
> > generate the correct disassembly.
> >
> > Co-authored-by: Markus Metzger <markus.t.metzger@intel.com>
> > Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> > Co-authored-by: Mihails Strasuns <mihails.strasuns@intel.com>
> > Co-authored-by: Mohamed Bouhaouel <mohamed.bouhaouel@intel.com>
> 
> Only minor comments noted below.
> 
> Approved-By: Simon Marchi <simon.marchi@efficios.com>

Thank you.  Below I reply to some of your comments.  Those that I omitted
have been addressed in the local branch.  They will be included in the
next revision.

> > diff --git a/gdb/intelgt-tdep.c b/gdb/intelgt-tdep.c
...
> > +/* Read part of REGNUM at OFFSET into BUFFER.  The length of data to
> > +   read is SIZE.  Consider using this helper function when reading
> > +   subregisters of CR0, SR0, and R0.  */
> > +
> > +static void
> > +intelgt_read_register_part (readable_regcache *regcache, int regnum,
> > +			    size_t offset,
> > +			    gdb::array_view<gdb_byte> buffer,
> > +			    const char *error_message)
> > +{
> > +  if (regnum == -1)
> > +    error (_("%s  Unexpected reg num '-1'."), error_message);
> 
> This should probably be an assert.
> 
> > +
> > +  gdbarch *arch = regcache->arch ();
> > +  const char *regname = gdbarch_register_name (arch, regnum);
> > +  int regsize = register_size (arch, regnum);
> > +
> > +  if (offset + buffer.size () > regsize)
> > +    error (_("%s  %s[%zu:%zu] is outside the range of %s[%d:0]."),
> > +	   error_message, regname, (offset + buffer.size () - 1), offset,
> > +	   regname, (regsize - 1));
> 
> I also wonder about this check, does it protect against bad user input,
> or a bug elsewhere in GDB?

Regarding this check and the check above:

Register sets are obtained from the debug API, which specifies the number
and the size of register sets.  The errors here and above protect against
debug library bugs, in case it reports incomplete register sets or incorrect
information about the number/size of regs.  So, we're checking against
input obtained from outside GDB.

> > +
> > +  register_status reg_status
> > +    = regcache->cooked_read_part (regnum, offset, buffer);
> > +
> > +  if (reg_status == REG_UNAVAILABLE)
> > +    throw_error (NOT_AVAILABLE_ERROR,
> > +		 _("%s  Register %s (%d) is not available."),
> > +		 error_message, regname, regnum);
> > +
> > +  if (reg_status == REG_UNKNOWN)
> > +    error (_("%s  Register %s (%d) is unknown."), error_message,
> > +	   regname, regnum);
> 
> Does this REG_UNKNOWN case happen in real life?  I always thought that
> REG_UNKNOWN was the initial state of registers in the regcache, and that
> after filling a regcache (a call to target_ops::fetch_registers), then
> all registers would become either REG_VALID or REG_UNAVAILABLE.  I
> presumed that if a register was left in the REG_UNKNOWN, it would be a
> target bug.

I don't think this case ever happened in real life.  We included the check
for completeness' sake.

...
> > +/* 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);
> > +  intelgt_debug_printf ("prev_ip: %s", paddress (gdbarch, prev_ip));
> > +
> > +  /* Program counter is $ip + $isabase.  Read directly from the
> > +     regcache instead of unwinding, as the frame unwind info may
> > +     simply be unavailable.  The isabase register does not change
> > +     during kernel execution, so this must be safe.  */
> > +  regcache *regcache = get_thread_regcache (inferior_thread ());
> > +  CORE_ADDR isabase = intelgt_get_isabase (regcache);
> 
> Why do you not want to read isabase from NEXT_FRAME, because it's
> slower?  I'm asking because this use of inferior_thread slightly annoys
> me.

`isabase` is a virtual register.  Compiler does not emit unwind information
for it.  However, GDB is able to cope with that.  The register would be
eventually read from regcache.  We can replace this part with 
frame_unwind_register_unsigned to get rid of `inferior_thread ()`.

...
> > +/* The memory_remove_breakpoint gdbarch method.  */
> > +
> > +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.  */
> 
> inserting -> removing?

Here, we are inserting the original contents of the instruction
and raising a warning if it had a permanent breakpoint.  I agree,
however, that saying "inserting" in this function sounds confusing.
I thought about rephrasing the comment, but both the original comment
and my rewording would be repeating what the code is actually doing.
So, I removed the comment.

-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