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

[Simon Marchi <simark@simark.ca>]

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>

> diff --git a/gdb/intelgt-tdep.c b/gdb/intelgt-tdep.c
> new file mode 100644
> index 0000000000000000000000000000000000000000..3919947fed44c9e939bef6c14b854ae416ab7c41
> --- /dev/null
> +++ b/gdb/intelgt-tdep.c
> @@ -0,0 +1,875 @@
> +/* Target-dependent code for the Intel(R) Graphics Technology architecture.
> +
> +   Copyright (C) 2019-2025 Free Software Foundation, Inc.
> +
> +   This file is part of GDB.
> +
> +   This program is free software; you can redistribute it and/or modify
> +   it under the terms of the GNU General Public License as published by
> +   the Free Software Foundation; either version 3 of the License, or
> +   (at your option) any later version.
> +
> +   This program is distributed in the hope that it will be useful,
> +   but WITHOUT ANY WARRANTY; without even the implied warranty of
> +   MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.  See the
> +   GNU General Public License for more details.
> +
> +   You should have received a copy of the GNU General Public License
> +   along with this program.  If not, see <http://www.gnu.org/licenses/>.  */
> +
> +#include "arch-utils.h"
> +#include "arch/intelgt.h"
> +#include "cli/cli-cmds.h"
> +#include "dwarf2/frame.h"
> +#include "frame-unwind.h"
> +#include "gdbsupport/gdb_obstack.h"

gdb_obstack.h is unused.

> +/* Data specific for this architecture.  */
> +
> +struct intelgt_gdbarch_tdep : gdbarch_tdep_base
> +{
> +  /* $r0 GRF register number.  */
> +  int r0_regnum = -1;
> +
> +  /* $ce register number in the regcache.  */
> +  int ce_regnum = -1;
> +
> +  /* Register number for the GRF containing function return value.  */
> +  int retval_regnum = -1;
> +
> +  /* Register number for the control register.  */
> +  int cr0_regnum = -1;
> +
> +  /* Register number for the state register.  */
> +  int sr0_regnum = -1;
> +
> +  /* Register number for the instruction base virtual register.  */
> +  int isabase_regnum = -1;
> +
> +  /* Register number for the general state base SBA register.  */
> +  int genstbase_regnum = -1;
> +
> +  /* Register number for the DBG0 register.  */
> +  int dbg0_regnum = -1;
> +
> +  /* Assigned regnum ranges for DWARF regsets.  */
> +  regnum_range regset_ranges[intelgt::REGSET_COUNT];
> +
> +  /* Enabled pseudo-registers for the current target description.  */
> +  std::vector<std::string> enabled_pseudo_regs;
> +
> +  /* Cached $framedesc pseudo-register type.  */
> +  type *framedesc_type = nullptr;
> +
> +  /* Initialize ranges to -1 as "not-yet-set" indicator.  */
> +  intelgt_gdbarch_tdep ()
> +  {
> +    memset (&regset_ranges, -1, sizeof regset_ranges);
> +  }

Instead of this memset, can you make it the default state of a
regnum_range?

struct regnum_range
{
  int start = -1;
  int end = -1;
};

Otherwise, I would prefer some loop that assigns {-1, -1} to each
field, rather than memset-ing objects.

Also, we usually put the constructors at the beginning...

> +
> +  /* Return regnum where frame descriptors are stored.  */
> +  int framedesc_base_regnum ()
> +  {
> +    /* For EM_INTELGT frame descriptors are stored at MAX_GRF - 1.  */
> +    gdb_assert (regset_ranges[intelgt::REGSET_GRF].end > 1);
> +    return regset_ranges[intelgt::REGSET_GRF].end - 1;
> +  }

... and while we have a mixed bag of styles, I think that putting the
methods before the data members is common.

> +/* 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?

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

> +/* Convert a DWARF register number to a GDB register number.  This
> +   function requires the register listing in the target description to
> +   be in the same order in each regset as the intended DWARF numbering
> +   order.  Currently this always holds true when gdbserver generates
> +   the target description.  */
> +
> +static int
> +intelgt_dwarf_reg_to_regnum (gdbarch *gdbarch, int num)
> +{
> +  constexpr int ip = 0;
> +  constexpr int ce = 1;
> +
> +  /* Register sets follow this format: [START, END), where START is
> +     inclusive and END is exclusive.  */
> +  constexpr regnum_range dwarf_nums[intelgt::REGSET_COUNT] = {
> +    [intelgt::REGSET_SBA] = { 5, 12 },
> +    [intelgt::REGSET_GRF] = { 16, 272 },
> +    [intelgt::REGSET_ADDR] = { 272, 288 },
> +    [intelgt::REGSET_FLAG] = { 288, 304 },
> +    [intelgt::REGSET_ACC] = { 304, 320 },
> +    [intelgt::REGSET_MME] = { 320, 336 },
> +  };
> +
> +  /* Number of SBA registers.  */
> +  constexpr size_t sba_dwarf_len = dwarf_nums[intelgt::REGSET_SBA].end
> +    - dwarf_nums[intelgt::REGSET_SBA].start;
> +
> +  /* Map the DWARF register numbers of SBA registers to their names.
> +     Base number is dwarf_nums[intelgt::REGSET_SBA].start.  */
> +  constexpr const char* sba_dwarf_reg_order[sba_dwarf_len] {

Space before *.

> +    "btbase",
> +    "scrbase",
> +    "genstbase",
> +    "sustbase",
> +    "blsustbase",
> +    "blsastbase",
> +    "scrbase2"
> +  };
> +
> +  intelgt_gdbarch_tdep *data
> +    = gdbarch_tdep<intelgt_gdbarch_tdep> (gdbarch);
> +
> +  if (num == ip)
> +    return intelgt_pseudo_register_num (gdbarch, "ip");
> +  if (num == ce)
> +    return data->ce_regnum;
> +
> +  for (int regset = 0; regset < intelgt::REGSET_COUNT; ++regset)
> +    if (num >= dwarf_nums[regset].start && num < dwarf_nums[regset].end)
> +      {
> +	if (regset == intelgt::REGSET_SBA)
> +	  {
> +	    /* For SBA registers we first find out the name of the
> +	       register out of DWARF register number and then find the
> +	       register number corresponding to the name.  */
> +	    int sba_num = num - dwarf_nums[intelgt::REGSET_SBA].start;
> +	    const char* name = sba_dwarf_reg_order [sba_num];

Space before *, no space before [.

> +
> +	    return user_reg_map_name_to_regnum (gdbarch, name, -1);
> +	  }
> +	else
> +	  {
> +	    int candidate = data->regset_ranges[regset].start + num
> +	      - dwarf_nums[regset].start;

Formatting:

	    int candidate = (data->regset_ranges[regset].start + num
			     - dwarf_nums[regset].start);

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

> +/* The memory_insert_breakpoint gdbarch method.  */
> +
> +static int
> +intelgt_memory_insert_breakpoint (gdbarch *gdbarch, bp_target_info *bp)
> +{
> +  intelgt_debug_printf ("req ip: %s", paddress (gdbarch,
> +						bp->reqstd_address));
> +
> +  /* Ensure that we have enough space in the breakpoint.  */
> +  static_assert (intelgt::MAX_INST_LENGTH <= BREAKPOINT_MAX);
> +
> +  gdb_byte inst[intelgt::MAX_INST_LENGTH];
> +  int err = target_read_memory (bp->reqstd_address, inst,
> +				intelgt::MAX_INST_LENGTH);
> +  if (err != 0)
> +    {
> +      /* We could fall back to reading a full and then a compacted
> +	 instruction but I think we should rather allow short reads than
> +	 having the caller try smaller and smaller sizes.  */

Please avoid the first person in comments (there is at least one other
instance).

> +/* 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?

> +/* The "read_pc" gdbarch method.  */
> +
> +static CORE_ADDR
> +intelgt_read_pc (readable_regcache *regcache)
> +{
> +  gdbarch *arch = regcache->arch ();
> +  intelgt_gdbarch_tdep *data = gdbarch_tdep<intelgt_gdbarch_tdep> (arch);
> +
> +  /* Instruction pointer is stored in CR0.2.  */
> +  uint32_t ip;
> +  intelgt_read_register_part (regcache, data->cr0_regnum,
> +			      sizeof (uint32_t) * 2,
> +			      gdb::make_array_view ((gdb_byte *) &ip,
> +						    sizeof (uint32_t)),
> +			      _("Cannot compute PC."));

Even though we know the host will likely always be little-endian, this
is not technically correct.  There should be an extract_unsigned_integer
call (or equivalent) to extract the target bytes into the host variable.
Same for other functions that read or update the PC using
make_array_view.

> +/* Called by tdesc_use_registers each time a new regnum
> +   is assigned.  Used to track down assigned numbers for
> +   any important regnums.  */
> +
> +static int
> +intelgt_unknown_register_cb (gdbarch *arch, tdesc_feature *feature,
> +			     const char *reg_name, int possible_regnum)
> +{
> +  intelgt_gdbarch_tdep *data = gdbarch_tdep<intelgt_gdbarch_tdep> (arch);
> +
> +  /* First, check if this a beginning of a not yet tracked regset
> +     assignment.  */
> +
> +  for (int regset = 0; regset < intelgt::REGSET_COUNT; ++regset)
> +    {
> +      if (data->regset_ranges[regset].start == -1
> +	  && feature->name == intelgt::DWARF_REGSET_FEATURES[regset])
> +	{
> +	  data->regset_ranges[regset].start = possible_regnum;
> +	  data->regset_ranges[regset].end
> +	      = feature->registers.size () + possible_regnum;
> +	  break;
> +	}
> +    }

You can get rid of the braces for the "for".

> +/* Architecture initialization.  */
> +
> +static gdbarch *
> +intelgt_gdbarch_init (gdbarch_info info, gdbarch_list *arches)
> +{
> +  /* If there is already a candidate, use it.  */
> +  arches = gdbarch_list_lookup_by_info (arches, &info);
> +  if (arches != nullptr)
> +    return arches->gdbarch;
> +
> +  const target_desc *tdesc = info.target_desc;
> +  gdbarch_up gdbarch_u
> +    (gdbarch_alloc (&info,
> +		    gdbarch_tdep_up (new intelgt_gdbarch_tdep)));
> +  gdbarch *gdbarch = gdbarch_u.get ();
> +  intelgt_gdbarch_tdep *data
> +    = gdbarch_tdep<intelgt_gdbarch_tdep> (gdbarch);
> +
> +  /* Initialize register info.  */
> +  set_gdbarch_num_regs (gdbarch, 0);
> +  set_gdbarch_register_name (gdbarch, tdesc_register_name);
> +
> +  if (tdesc_has_registers (tdesc))
> +    {
> +      tdesc_arch_data_up tdesc_data = tdesc_data_alloc ();
> +
> +      /* First assign register numbers to all registers.  The
> +	 callback function will record any relevant metadata
> +	 about it in the intelgt_gdbarch_data instance to be
> +	 inspected after.  */
> +
> +      tdesc_use_registers (gdbarch, tdesc, std::move (tdesc_data),
> +			   intelgt_unknown_register_cb);

Might as well inline the call to tdesc_data_alloc.

> +
> +      /* Now check the collected metadata to ensure that all
> +	 mandatory pieces are in place.  */
> +
> +      if (data->ce_regnum == -1)
> +	error (_("Debugging requires $ce provided by the target"));

Missing "to be".

> +      if (data->retval_regnum == -1)
> +	error (_("Debugging requires return value register to be provided "
> +		 "by the target"));
> +      if (data->cr0_regnum == -1)
> +	error (_("Debugging requires control register to be provided by "
> +		 "the target"));
> +      if (data->sr0_regnum == -1)
> +	error (_("Debugging requires state register to be provided by "
> +		 "the target"));
> +
> +      /* 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);

Opening parenthesis on next line.

Simon