Re: [PATCH v3 06/44] gdb, arch, intelgt: add intelgt arch definitions

[Simon Marchi <simark@simark.ca>]

On 8/1/25 5:37 AM, Tankut Baris Aktemur wrote:
> From: Markus Metzger <markus.t.metzger@intel.com>
> 
> Provide Intel GT architecture-specific definitions that can be used by
> both the low target at the server side and tdep at the GDB side.
> 
> Other than, for example, IA, Intel GT does not have a dedicated
> breakpoint instruction.  Instead, it has a breakpoint bit in each
> instruction.  We define arch methods for dealing with instruction
> breakpoint bits.
> 
> Co-authored-by: Tankut Baris Aktemur <tankut.baris.aktemur@intel.com>
> Co-authored-by: Mihails Strasuns <mihails.strasuns@intel.com>
> Co-authored-by: Natalia Saiapova <natalia.saiapova@intel.com>
> Reviewed-By: Thiago Jung Bauermann <thiago.bauermann@linaro.org>

This LGTM, see nits below.

Approved-By: Simon Marchi <simon.marchi@efficios.com>

> +#include "gdbsupport/tdesc.h"
> +#include <string>
> +#include <vector>

clangd tells me that all these includes are unused.

> +
> +namespace intelgt {

Good idea to use a namespace here, we don't do that enough in GDB.

> +
> +/* Various arch constants.  */
> +
> +enum breakpoint_kind
> +{
> +  BP_INSTRUCTION = 1,
> +};

enum class, for new stuff?  You can get rid of the prefix then.

> +
> +/* The length of a full and compact IntelGT instruction in bytes.  */
> +
> +constexpr int MAX_INST_LENGTH = 16;
> +constexpr int COMPACT_INST_LENGTH = 8;
> +
> +/* Feature names.
> +
> +   They correspond to register sets defined in zet_intel_gpu_debug.h.  We
> +   declare feature names in the order used in that header.
> +
> +   The SBA register set consists of a set of base registers in the order
> +   defined in that header file.
> +
> +   Not all registers have DWARF numbers.  See DWARF_REGSETS below for a
> +   list of features that do.  */
> +constexpr const char *FEATURE_GRF = "org.gnu.gdb.intelgt.grf";
> +constexpr const char *FEATURE_ADDR = "org.gnu.gdb.intelgt.addr";
> +constexpr const char *FEATURE_FLAG = "org.gnu.gdb.intelgt.flag";
> +constexpr const char *FEATURE_CE = "org.gnu.gdb.intelgt.ce";
> +constexpr const char *FEATURE_SR = "org.gnu.gdb.intelgt.sr";
> +constexpr const char *FEATURE_CR = "org.gnu.gdb.intelgt.cr";
> +constexpr const char *FEATURE_TDR = "org.gnu.gdb.intelgt.tdr";
> +constexpr const char *FEATURE_ACC = "org.gnu.gdb.intelgt.acc";
> +constexpr const char *FEATURE_MME = "org.gnu.gdb.intelgt.mme";
> +constexpr const char *FEATURE_SP = "org.gnu.gdb.intelgt.sp";
> +constexpr const char *FEATURE_SBA = "org.gnu.gdb.intelgt.sba";
> +constexpr const char *FEATURE_DBG = "org.gnu.gdb.intelgt.dbg";
> +constexpr const char *FEATURE_FC = "org.gnu.gdb.intelgt.fc";
> +constexpr const char *FEATURE_DEBUGGER = "org.gnu.gdb.intelgt.debugger";
> +
> +/* Register sets/groups needed for DWARF mapping.  Used for
> +   declaring static arrays for various mapping tables.  */
> +
> +enum dwarf_regsets : int
> +{
> +  REGSET_SBA = 0,
> +  REGSET_GRF,
> +  REGSET_ADDR,
> +  REGSET_FLAG,
> +  REGSET_ACC,
> +  REGSET_MME,
> +  REGSET_COUNT
> +};
> +
> +/* Map of dwarf_regset values to the target description
> +   feature names.  */
> +
> +constexpr const char *DWARF_REGSET_FEATURES[REGSET_COUNT] = {
> +  FEATURE_SBA,
> +  FEATURE_GRF,
> +  FEATURE_ADDR,
> +  FEATURE_FLAG,
> +  FEATURE_ACC,
> +  FEATURE_MME
> +};
> +
> +/* The encoding for XE version enumerates follows this pattern, which is

enumerates -> enumerators?

> +   aligned with the IGA encoding.  */

What is IGA?  Around here it's a grocery store (https://www.iga.net). :)

> +
> +#define XE_VERSION(MAJ, MIN) (((MAJ) << 24) | (MIN))

With my modern C++ hat on: I am wondering if this can be a (constexpr?)
function, just because.

> +/* Supported GDB XE platforms.  */
> +
> +enum xe_version
> +{
> +  XE_INVALID = 0,
> +  XE_HP = XE_VERSION (1, 1),
> +  XE_HPG = XE_VERSION (1, 2),
> +  XE_HPC = XE_VERSION (1, 4),
> +  XE2 = XE_VERSION (2, 0),
> +  XE3 = XE_VERSION (3, 0),
> +};
> +
> +/* Helper function to translate the device id to a device version.  */
> +
> +extern xe_version get_xe_version (uint32_t device_id);

I would remove this "extern" (and all the unnecessary uses of "extern"
in the project actually).

Simon