From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id EaXuAFrN4mnhwyEAWB0awg (envelope-from ) for ; Fri, 17 Apr 2026 20:16:26 -0400 Authentication-Results: simark.ca; dkim=pass (2048-bit key; unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=jS7MqHsJ; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id D5F281E0B1; Fri, 17 Apr 2026 20:16:25 -0400 (EDT) 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 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 966091E04F for ; Fri, 17 Apr 2026 20:16:23 -0400 (EDT) Received: from vm01.sourceware.org (localhost [127.0.0.1]) by sourceware.org (Postfix) with ESMTP id B555D4B920BC for ; Sat, 18 Apr 2026 00:16:22 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org B555D4B920BC Authentication-Results: sourceware.org; dkim=pass (2048-bit key, unprotected) header.d=linaro.org header.i=@linaro.org header.a=rsa-sha256 header.s=google header.b=jS7MqHsJ Received: from mail-vk1-xa33.google.com (mail-vk1-xa33.google.com [IPv6:2607:f8b0:4864:20::a33]) by sourceware.org (Postfix) with ESMTPS id EBDDE4B920B4 for ; Sat, 18 Apr 2026 00:15:56 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org EBDDE4B920B4 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=linaro.org Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=linaro.org ARC-Filter: OpenARC Filter v1.0.0 sourceware.org EBDDE4B920B4 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::a33 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1776471357; cv=none; b=d6YijBsQz1e/mRpABmZwHMZXV9pQiM35OOvG5OTSIqJ5EalZ7SMAdEc625RonNXF0SDkBgxFv8K9GdYGUtRQ4mTelLkQ76wKlbg6rxhF6BRkLD3HLF81gXfhxZqS+4p31vfxYLzt19VWYV4WrC3mTLD5TLnQ7YNFjY/ScKZ58xE= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1776471357; c=relaxed/simple; bh=XZOKoSkBcT6T0vnGwQeqkxg8o5XrYPuASrJCVio19qE=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=BCTFn7RKvaQ2goH6y6/OfuVm8Hahl4qZGENzrXTaMQLzlTEo5B0xE21sqGNIa5mxoev9g7ZsmFembWdmn6b9vGChxIT9IL9aFWAZahgLJeA2T1mCFqY9Lc59qAjsR/8fh8o0oB8FGb2mC/bn7kmTdNZQ6hqnin7l++QF6Kl3d+0= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org EBDDE4B920B4 Received: by mail-vk1-xa33.google.com with SMTP id 71dfb90a1353d-56d89f35940so435647e0c.2 for ; Fri, 17 Apr 2026 17:15:56 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1776471356; x=1777076156; darn=sourceware.org; h=content-transfer-encoding:mime-version:message-id:date:user-agent :references:in-reply-to:subject:cc:to:from:from:to:cc:subject:date :message-id:reply-to; bh=YnKPJKCVjTPfYc+ZT1luxmI6RLarIaQZPBZWBLYmdpo=; b=jS7MqHsJRfX8k3BEdhBidhhrM8BA3m6veid+Sa3ete+XoIxlRzqNt7oZeByiasdTsI FuGn1YdAxv7a7nlVG9EjWg9DibylmIErSiK3cm5EZxTh/N4tRyornpl9P+hozxGBpLx2 dQWZ8xW78LAikSI+6vZ0ZoCWcMbCtuR0Qt3SIAZx4zMOROs0vSYABwCP7apN8qc9Np91 cBadoVomQN7DpySICBxl23IgnszQStpLzkT+e+kh28OuatDYmUxkV0AMbQjt2WFRmGQ7 FQszASx056SHtsvDgp0M4veCdpQs52exow6MtJyWYT8M1o8LTKsB2MPwDlPrVHggJlbX Hb/w== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1776471356; x=1777076156; h=content-transfer-encoding:mime-version:message-id:date:user-agent :references:in-reply-to:subject:cc:to:from:x-gm-gg :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=YnKPJKCVjTPfYc+ZT1luxmI6RLarIaQZPBZWBLYmdpo=; b=mxczpd1ZAOhRIvD7prjwvIeRWpBZ/A0UkicvVKkaQf53o9VcPdVl3CG2Q1NExFK6yN gbmUlUqHy0VP9liNnEL3ybEc9UQublI+qTdDyjhCjgHFwdnWCD8Cgud4iJbf6Gqqkue9 7bkqOKD1Y03hQazWZ7Q/AOT7JeZ8uBlaneZ3oOhNqIHs5pG9nappPAzPvDLz9+382ke1 aKB8yTMzI7ZBPudHWE8zlVqKXHEmso8EVdL3HCe0iv9UN7EF5a21XtCstJHpune4WIaw n3cjRflPKPFozuWw3f19/d3pArdq331p4lZ26LV1F+miMY7Khwad9vBySowW50/p6qty Ea/Q== X-Gm-Message-State: AOJu0YyU2Fg+R5ayveswVcZ/3vDO/7tuQANpdvI8HWlFKJVBzn1O5Y+w /nvHAiDLCjeWZU8NegtlFeYAbEqZsEr4lczx7BjBJ6gIy/DsABKxP2Zpx9fFJhjOxhzLNM6lNEF CpuoF X-Gm-Gg: AeBDiesJBBT1Xkm+HlsjQYzCwgZqp1YXQ/tDLLLOGj6eSz95wC9p7pOrAoN/DKoSPaY v4yA3Jrd3SZzHg7UEvFGsIobyojaZCR3EhgFhGSge4MAaZuU0XoxAFw0v0ArwU7Vm3MFafPfcQz qussSHpQp0pwO7X43DUH996XnRuUTzxAQf+hrH3Tj4T5URwwtDGz/L1OCTEN5yq9ATVZtSIvNml 1M1LrswsiYN5br6k1nPZ+RGmdjdLuoGAmz3EXokVpLRnv5pW1kX4fBBUmCa7FCqEm1JXxqXs4c1 lWwzPJtKSiIkNMqXjhT5tWOetSmv7E7fDxgeT85nNXGFrDTgtu1epoOJ1242zRYPUrOh7JJWkfZ CfYAIPIngytN1EnhFmBasaQwdTl9xW7iyzb1ahred0qiuzW9C/JaWZhitPkNdBQ0S36ZiYBZjp+ 1uqQFT0gjTFDgLbykATmQoCm+EdeL48yN9mcz2NBZNzZB+ X-Received: by 2002:a05:6122:3d41:b0:56f:189b:7b99 with SMTP id 71dfb90a1353d-56fa57d8343mr2950672e0c.1.1776471356152; Fri, 17 Apr 2026 17:15:56 -0700 (PDT) Received: from localhost ([2804:14d:7e39:8083:c4f1:ef88:5168:2d26]) by smtp.gmail.com with ESMTPSA id 71dfb90a1353d-56fa933a54dsm1881085e0c.16.2026.04.17.17.15.54 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 Apr 2026 17:15:55 -0700 (PDT) From: Thiago Jung Bauermann To: Guinevere Larsen Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 1/6] gdb/record: Refactor record history In-Reply-To: <20260415185836.2732968-2-guinevere@redhat.com> (Guinevere Larsen's message of "Wed, 15 Apr 2026 15:58:31 -0300") References: <20260415185836.2732968-1-guinevere@redhat.com> <20260415185836.2732968-2-guinevere@redhat.com> User-Agent: mu4e 1.14.0; emacs 30.2 Date: Fri, 17 Apr 2026 21:15:51 -0300 Message-ID: <87tst9cedk.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain; charset=utf-8 Content-Transfer-Encoding: quoted-printable 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 Hello Guinevere, Nice improvement! Just a few comments, mostly nits. Guinevere Larsen writes: > This is the first step in a large refactor in how GDB keeps execution > history. Rather than using a linked list where multiple entries can > describe a single instruction, the history will now be stored in an > std::deque, each instruction being one entry in the deque. > > The choice was initially to use an std::vector, but it would become > unwieldy because it needs all the memory to be consecutive, which is > hard for 200 thousand entries. Deque was picked because it was a nice > midpoint between vector (maximum cache cohesion) and linked list > (maximum ease of finding space to store more). > > Each instruction in memory will be now one record_full_instruction > entry, which for this commit just contains a vector of > record_full_entry for the effects of the instruction, and the data that > was stored in the record_full_end entry (that is, the instruction number > and the signal, if any). > > This change introduced a minimal performance improvement (what's > important is that it isn't a degradation) and a reduction in the total > memory footprint of roughly 20% if the entire history is used. Awesome! > --- > gdb/aarch64-tdep.c | 2 - > gdb/amd64-linux-tdep.c | 3 - > gdb/arm-tdep.c | 2 - > gdb/i386-linux-tdep.c | 3 - > gdb/i386-tdep.c | 4 - > gdb/loongarch-tdep.c | 2 - > gdb/moxie-tdep.c | 2 - > gdb/ppc-linux-tdep.c | 3 - > gdb/record-full.c | 1193 ++++++++++++++++------------------------ > gdb/record-full.h | 1 - > gdb/riscv-tdep.c | 3 - > gdb/rs6000-tdep.c | 4 - > gdb/s390-linux-tdep.c | 3 - > gdb/s390-tdep.c | 2 - > 14 files changed, 482 insertions(+), 745 deletions(-) > > diff --git a/gdb/aarch64-tdep.c b/gdb/aarch64-tdep.c > index 4befaa2720d..a532992b1d8 100644 > --- a/gdb/aarch64-tdep.c > +++ b/gdb/aarch64-tdep.c > @@ -6210,8 +6210,6 @@ aarch64_process_record (struct gdbarch *gdbarch, st= ruct regcache *regcache, > aarch64_record.aarch64_mems[rec_no].len)) > ret =3D -1; >=20=20 > - if (record_full_arch_list_add_end ()) > - ret =3D -1; > } >=20=20 > deallocate_reg_mem (&aarch64_record); =E2=8B=AE > diff --git a/gdb/arm-tdep.c b/gdb/arm-tdep.c > index 08cce9dbad8..1ce0927f26b 100644 > --- a/gdb/arm-tdep.c > +++ b/gdb/arm-tdep.c > @@ -14911,8 +14911,6 @@ arm_process_record (struct gdbarch *gdbarch, stru= ct regcache *regcache, > } > } >=20=20 > - if (record_full_arch_list_add_end ()) > - ret =3D -1; > } >=20=20 > FWIW the aarch64-tdep.c and arm-tdep.c changes are obvious. =E2=8B=AE > /* Free one record entry, any type. > Return entry->type, in case caller wants to know. */ The "Return ..." sentence can be deleted, since the function returns void n= ow. > -static inline enum record_full_type > -record_full_entry_release (struct record_full_entry *rec) > +static inline void > +record_full_entry_cleanup (struct record_full_entry rec) > { > - enum record_full_type type =3D rec->type; >=20=20 > - switch (type) { > + switch (rec.type) { > case record_full_reg: > - record_full_reg_release (rec); > + record_full_reg_cleanup (rec); > break; > case record_full_mem: > - record_full_mem_release (rec); > - break; > - case record_full_end: > - record_full_end_release (rec); > + record_full_mem_cleanup (rec); > break; > } > - return type; > } =E2=8B=AE > -/* Free all record entries forward of the given list position. */ > - > static void > -record_full_list_release_following (struct record_full_entry *rec) > +record_full_list_release_following (int index) > { > - struct record_full_entry *tmp =3D rec->next; > - > - rec->next =3D NULL; > - while (tmp) > + for (int i =3D record_full_list.size (); i >=3D index; i--) Shouldn't this loop start with record_full_list.size () - 1, to avoid accessing member record_full_list[record_full_list.size ()] below, which AFAIK isn't valid? Also, I'll just point out one thing which I don't know if it's intentional or not: before this patch, this function preserved the rec entry provided as argument. After this patch, it removes the entry corresponding to the index argument. > { > - rec =3D tmp->next; > - if (record_full_entry_release (tmp) =3D=3D record_full_end) > - { > - record_full_insn_num--; > - record_full_insn_count--; > - } > - tmp =3D rec; > + for (auto &entry : record_full_list[i].effects) > + record_full_entry_cleanup (entry); > + record_full_list.pop_back (); > } > + /* Set the next instruction to be past the end of the log so we > + start recording if the user moves forward again. */ > + record_full_next_insn =3D index; > +} > + > +static void > +record_full_save_instruction () > +{ > + ++record_full_insn_count; > + record_full_incomplete_instruction.insn_num =3D record_full_insn_count; > + record_full_incomplete_instruction.effects.shrink_to_fit (); > + record_full_list.push_back (record_full_incomplete_instruction); > + record_full_next_insn ++; Extra space before '++'. > @@ -746,7 +648,7 @@ record_full_message (struct regcache *regcache, enum = gdb_signal signal) > the user says something different, like "deliver this signal" > during the replay mode). >=20=20 > - User should understand that nothing he does during the replay > + User should understand that nothing they do during the replay > mode will change the behavior of the child. If he tries, s/he tries/they try/ > then that is a user error. > =E2=8B=AE > @@ -1322,21 +1223,6 @@ record_full_wait_1 (struct target_ops *ops, > record_full_stop_reason =3D TARGET_STOPPED_BY_NO_REASON; > status->set_stopped (GDB_SIGNAL_0); >=20=20 > - /* Check breakpoint when forward execute. */ > - if (execution_direction =3D=3D EXEC_FORWARD) > - { > - tmp_pc =3D regcache_read_pc (regcache); > - if (record_check_stopped_by_breakpoint (aspace, tmp_pc, > - &record_full_stop_reason)) > - { > - if (record_debug) > - gdb_printf (gdb_stdlog, > - "Process record: break at %s.\n", > - paddress (gdbarch, tmp_pc)); > - goto replay_out; > - } > - } > - > /* If GDB is in terminal_inferior mode, it will not get the > signal. And in GDB replay mode, GDB doesn't need to be > in terminal_inferior mode, because inferior will not The commit message mentions only a change in the data structure used to store recorded instructions, but this hunk looks like an unrelated change in the logic: why is it ok to remove this check of whether a breakpoint has been hit? I suppose it's correct because the are no testsuite regressions (there actually are for this patch, but not for the series as a whole). This change should either be explained in the commit message, or moved to a separate patch (assuming that's feasible). Or maybe I'm wrong and this is just part of the big code reorg in this function? If so, please disregard this comment. =E2=8B=AE > - replay_out: > + if (record_full_next_insn < 0) > + { > + gdb_assert (execution_direction =3D=3D EXEC_REVERSE); > + record_full_next_insn =3D 0; > + } > + else if (record_full_next_insn > record_full_list.size ()) > + { > + gdb_assert (execution_direction =3D=3D EXEC_FORWARD); > + record_full_next_insn =3D record_full_list.size (); > + } > + /* Reset the current instruciton to point to the one to be replayed > + moving forward. */ > + else if (execution_direction =3D=3D EXEC_REVERSE) > + record_full_next_insn++; > + > + //replay_out: This commented-out label should be removed. > if (status->kind () =3D=3D TARGET_WAITKIND_STOPPED) > { > + int insn =3D (execution_direction =3D=3D EXEC_FORWARD) > + ? record_full_next_insn - 1 : record_full_next_insn; > if (record_full_get_sig) > status->set_stopped (GDB_SIGNAL_INT); > - else if (record_full_list->u.end.sigval !=3D GDB_SIGNAL_0) > - /* FIXME: better way to check */ > - status->set_stopped (record_full_list->u.end.sigval); > + else if (record_full_list[insn].sigval.has_value ()) > + status->set_stopped > + (record_full_list[insn].sigval.value ()); > else > status->set_stopped (GDB_SIGNAL_TRAP); > } =E2=8B=AE > @@ -2339,19 +2157,98 @@ netorder32 (uint32_t input) > return ret; > } >=20=20 > +static void > +record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int *bfd_off= set) > +{ > + uint8_t rectype; > + uint32_t regnum, len; > + uint64_t addr; > + regcache *cache =3D get_thread_regcache (inferior_thread ()); > + > + bfdcore_read (cbfd, osec, &rectype, sizeof (rectype), > + bfd_offset); This line fits in 80 columns. Also some others below, but they don't make it to the final version of the function so not that important. > @@ -2379,124 +2276,50 @@ record_full_restore (struct bfd &cbfd) > "RECORD_FULL_FILE_MAGIC (0x%s)\n", > phex_nz (netorder32 (magic), 4)); >=20=20 > - /* Restore the entries in recfd into record_full_arch_list_head and > - record_full_arch_list_tail. */ > - record_full_arch_list_head =3D NULL; > - record_full_arch_list_tail =3D NULL; > record_full_insn_num =3D 0; >=20=20 > try > { > - regcache *regcache =3D get_thread_regcache (inferior_thread ()); > This empty line should also be removed. > - while (1) > + while (bfd_offset < osec_size) > { > - uint8_t rectype; > - uint32_t regnum, len, signal, count; > - uint64_t addr; >=20=20 This empty line should also be removed. > - /* We are finished when offset reaches osec_size. */ > - if (bfd_offset >=3D osec_size) > - break; > - bfdcore_read (&cbfd, osec, &rectype, sizeof (rectype), &bfd_offset); > + record_full_reset_incomplete (); > + uint32_t eff_count =3D 0; > + uint8_t sigval; > + uint32_t insn_num; >=20=20 > - switch (rectype) > - { > - case record_full_reg: /* reg */ > - /* Get register number to regnum. */ > - bfdcore_read (&cbfd, osec, ®num, sizeof (regnum), > - &bfd_offset); > - regnum =3D netorder32 (regnum); > - > - rec =3D record_full_reg_alloc (regcache, regnum); > - > - /* Get val. */ > - bfdcore_read (&cbfd, osec, record_full_get_loc (rec), > - rec->u.reg.len, &bfd_offset); > - > - if (record_debug) > - gdb_printf (gdb_stdlog, > - " Reading register %d (1 " > - "plus %lu plus %d bytes)\n", > - rec->u.reg.num, > - (unsigned long) sizeof (regnum), > - rec->u.reg.len); > - break; > + /* First read the generic information for an instruction. */ > + bfdcore_read (&cbfd, osec, &sigval, > + sizeof (uint8_t), &bfd_offset); > + bfdcore_read (&cbfd, osec, &eff_count, sizeof (uint32_t), > + &bfd_offset); > + bfdcore_read (&cbfd, osec, &insn_num, > + sizeof (uint32_t), &bfd_offset); The first and third bfdcore_read calls above fit in 80 columns. Actually, there are many bfdcore_read and bfdcore_write calls in this and other patches which are unnecessarily broken into two lines. I will stop pointing them out. :) --=20 Thiago