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

Aktemur, Tankut Baris
2025-07-18T17:44:27Z

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