From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Yw/NCF0TO2mmRjUAWB0awg (envelope-from ) for ; Thu, 11 Dec 2025 13:54:21 -0500 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1765479261; bh=5g4ByUfQ6NrjEV61tuzCNaFAT2dM5eGO/m5vvRBSgWI=; h=Date:From:Subject:To:References:In-Reply-To:List-Id: List-Unsubscribe:List-Archive:List-Post:List-Help:List-Subscribe: From; b=wmUsBFMkDjesTBhSw8BrGg6Y8DXE/NdmmZdpaQwUfpsryPPtnb/HUqBL907TGPtPW Ta/Q59n85KpQAo19+2cKYVfXv2QZCheBHzpD7CniEpI3t6iJSLMxWbW7RBsEkCocBo Ip6Hj0P3MF4FjkfadMH0JgjhjWtev7qPK/OHNSP0= Received: by simark.ca (Postfix, from userid 112) id 0A5551E0B6; Thu, 11 Dec 2025 13:54:21 -0500 (EST) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-2.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,MAILING_LIST_MULTI, RCVD_IN_DNSWL_MED,RCVD_IN_VALIDITY_CERTIFIED_BLOCKED, RCVD_IN_VALIDITY_RPBL_BLOCKED,RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=egQ16S+E; dkim-atps=neutral Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id 187F21E08D for ; Thu, 11 Dec 2025 13:54:19 -0500 (EST) Received: from vm01.sourceware.org (localhost [127.0.0.1]) by sourceware.org (Postfix) with ESMTP id 8ABDB4BA2E32 for ; Thu, 11 Dec 2025 18:54:18 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8ABDB4BA2E32 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=simark.ca header.i=@simark.ca header.a=rsa-sha256 header.s=mail header.b=egQ16S+E Received: from simark.ca (simark.ca [158.69.221.121]) by sourceware.org (Postfix) with ESMTPS id 168C84BA2E05 for ; Thu, 11 Dec 2025 18:53:50 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 168C84BA2E05 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=simark.ca Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=simark.ca ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 168C84BA2E05 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=158.69.221.121 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1765479230; cv=none; b=iNx/c8CyeJxlnzW9OHbMK3G0c1Nj1baj/BFFNSDqe2p3nHx1O+GiYhaRC1TNk5LUAHweS51SQL1deCAHan6QiaWRL/9j3l7nDVZAt2rJHQqhN01r8uzyfwsebmMNQU5ODjM0VhjuxgwUI53HvaXX7dqtH9K7OoovkOCkaQq1o48= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1765479230; c=relaxed/simple; bh=5g4ByUfQ6NrjEV61tuzCNaFAT2dM5eGO/m5vvRBSgWI=; h=DKIM-Signature:Message-ID:Date:MIME-Version:From:Subject:To; b=KUmaiAUIM4AbFVMap4KBOMl5XyK+Mcjvr1e9dBe0bDKcHmxxx9c4ajFzoqjNzOuSmTjVMFD3wcKez3PSI4rqRD9Lm2Oc2hhujrVzXxAmH0YzMpUzAm1l6SmhgjtPq3v2JEjIavY9Yioxlw6DbdxR9OuoAC1KdtW+lt0mdQ/+W1Q= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 168C84BA2E05 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/simple; d=simark.ca; s=mail; t=1765479229; bh=5g4ByUfQ6NrjEV61tuzCNaFAT2dM5eGO/m5vvRBSgWI=; h=Date:From:Subject:To:References:In-Reply-To:From; b=egQ16S+EQC2bmMpSfFjUYkSDyGNyA5VvXQ55cbHf/umFJLk8xxortZX1VpTEWa3WY GZrogOZD4454eRQI+QMfxcc03O7Z2+ZLp/Ao1y0zloRXtqi1u0Ly3103kHB9szCtBJ LkF2T/hXJ4RpHXslyyHZ8MjEVxyz70duv2kwD368= Received: by simark.ca (Postfix) id 86D801E08D; Thu, 11 Dec 2025 13:53:49 -0500 (EST) Message-ID: <77802a4e-b72e-47fd-bb63-c084bf162331@simark.ca> Date: Thu, 11 Dec 2025 13:53:49 -0500 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird From: Simon Marchi Subject: Re: [PATCH v3 07/44] gdb, intelgt: add the target-dependent definitions for the Intel GT architecture To: Tankut Baris Aktemur , gdb-patches@sourceware.org, Markus Metzger References: <20250801-upstream-intelgt-mvp-v3-0-59ce0f87075b@intel.com> <20250801-upstream-intelgt-mvp-v3-7-59ce0f87075b@intel.com> Content-Language: en-US In-Reply-To: <20250801-upstream-intelgt-mvp-v3-7-59ce0f87075b@intel.com> Content-Type: text/plain; charset=UTF-8 Content-Transfer-Encoding: 7bit X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org 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 > Co-authored-by: Natalia Saiapova > Co-authored-by: Mihails Strasuns > Co-authored-by: Mohamed Bouhaouel Only minor comments noted below. Approved-By: Simon Marchi > 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 . */ > + > +#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 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 (®set_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 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 (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 (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 (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 (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