From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Q/Q/F/Z65mmgLi0AWB0awg (envelope-from ) for ; Mon, 20 Apr 2026 15:13:58 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=SxxYJKwB; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id DF7411E0C3; Mon, 20 Apr 2026 15:13:57 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,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 8EE321E093 for ; Mon, 20 Apr 2026 15:13:55 -0400 (EDT) Received: from vm01.sourceware.org (localhost [127.0.0.1]) by sourceware.org (Postfix) with ESMTP id 01AAE4A98F11 for ; Mon, 20 Apr 2026 19:13:55 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 01AAE4A98F11 Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=SxxYJKwB Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.129.124]) by sourceware.org (Postfix) with ESMTP id 6EF8D4BA23CF for ; Mon, 20 Apr 2026 19:13:26 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 6EF8D4BA23CF Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 6EF8D4BA23CF Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1776712406; cv=none; b=Cfhx4unDkGibHmkSKHKpp50qPf+sLiVb34nXT3PLax+T15KiqH7rBLgsCxHKTefgqZt5nS/ARjvoggJrXD+9SW1ePHO8LUHgE7u6/sCTzDrMUI9bzhIJGZqpbUCpijN8n6rOVITe0rtxVW78KXcNadT2QjHJbrb97VF8pRuOUzo= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1776712406; c=relaxed/simple; bh=mwuP8I1S7myZKZ1F6fP9aAJ7Fj6olV5+ofbUC4eCsiA=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=RzK2uIoxYBR3QR/dnK4wItaQdOtzRy8ylLXeKyAYoO5GxYhHw9O2+6EFROIKmdjRskxudtG9gYxfLXV29NNnJfeF/mL3cAXF4QnVw69109EOXmydYoSsTS4awzgNpZRbRnwpz72E7VbcDZD3sHJV5xh7N+qWt4PJD60phzH9CK4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 6EF8D4BA23CF DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1776712406; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: content-transfer-encoding:content-transfer-encoding: in-reply-to:in-reply-to:references:references; bh=hNiV6Llks9njsglIZTBXV5FxIPmR3o94Ne5UmzVh0G8=; b=SxxYJKwBjyIn3zvzSD4DT1IUSn5EGQLdXPq0GyQxRNHCnu7kYuAx+rcRNMEuRBDKekJIWT oW6A8Ng/Sz7g9zuUnoIBoUp/euEpt93d4iBXpHYy0lbknVh7dWjuwEg8JJGa8YtNnsC0IC 8+bKi7vGtPrTEkRXwrBFUY+YQEB4szE= Received: from mail-dl1-f70.google.com (mail-dl1-f70.google.com [74.125.82.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-463-5KDfyEaHOryT2jKvFdWIaQ-1; Mon, 20 Apr 2026 15:13:24 -0400 X-MC-Unique: 5KDfyEaHOryT2jKvFdWIaQ-1 X-Mimecast-MFC-AGG-ID: 5KDfyEaHOryT2jKvFdWIaQ_1776712404 Received: by mail-dl1-f70.google.com with SMTP id a92af1059eb24-12c8de02a4dso6537180c88.1 for ; Mon, 20 Apr 2026 12:13:24 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1776712403; x=1777317203; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-gg:x-gm-message-state:from:to:cc:subject:date:message-id :reply-to; bh=hNiV6Llks9njsglIZTBXV5FxIPmR3o94Ne5UmzVh0G8=; b=iZQcc6cXLC3WVBzdvCLpiKDeewBkZYAMk3LTVkAqLQ9c+YsLtKG3Jc2hEjH/zvNemf tmocIlt+S4WueAG+6BBOPM+OAGFNEy56DGet1VHeWajgy4nvAsUGB6+kEim5B1X09HHw KwRXEp+PBopt9q6vADjYrVzUHpJPCbLfw6TWmXL3T757a0m+WVc2VvYGNQZt3k1CqbUS 999HOuRXcqNk3LrO+q2Ddi4KCTPdir38RoDjm5Vv19Lm4jEATi4z64q5fBaAp52PCHic 46W1ERjxfHTUslz5/K7FiTxMYfxWwRvypi3IvJbpaVCOUtnuJIgVPxCzudKbk22zdYYN FiAA== X-Gm-Message-State: AOJu0YwJgxU7Zf4xAPythVgQGwwEKkzPCTuwrxNnLn7A3l85PVrkV4l9 MWGlXmlv8B+v9sly/05sLMwYwapX4prwNt8qsU2uT1jpRYWUfXQmbQ8wjKuKCFW8YfJx3GnCrfA vzNnT9MnB4YQ6NMc860VcKPcEFUbZoZ1oU9AYs0a6Eaqy7f8Vp4lprN9NPfyFV4zv5a/2EWs= X-Gm-Gg: AeBDieuYqgYtQk+TyO0xXfskBTIVngcoMaBWymezXf/yxV9DbvGTvpCfYjfL35lbYu3 IvYcLYIS0hs/V3ZP0ZbF8OgE4LXt2zzjqADeorpzW+sBkYWL7V8NZEO2WcMlZaNA2eaydIxlAPq Pn7O5DnCF8NcveH5+NkRHqiR2YCAn+hQe8hPrZ+La/D9CNXMkC6Psq3XnvTqGrKy+QyyIBgAzzt aDU3b1SV5ld1OyI3Q4FMTgTWPVfFv9AmgVhHE/0oLFMXaMNXRnB9GjVp01ne+P4SkrmJ4ZPgZbb XpltBVXYUbof6r28qwWDV1EDH7NdI6f3f5aOu8vzzO9KWfe5E3fnW9oNHq9dbciD7Fkix/utxVt +8dBEkU+IZIkdsGvbDVhVc6fxxk+gOWBuDyp0YO3upg== X-Received: by 2002:a05:7022:ff45:b0:128:d2a5:709c with SMTP id a92af1059eb24-12c73fb3381mr8982961c88.33.1776712402672; Mon, 20 Apr 2026 12:13:22 -0700 (PDT) X-Received: by 2002:a05:7022:ff45:b0:128:d2a5:709c with SMTP id a92af1059eb24-12c73fb3381mr8982933c88.33.1776712401987; Mon, 20 Apr 2026 12:13:21 -0700 (PDT) Received: from ?IPV6:2804:14d:8084:993e::75d? ([2804:14d:8084:993e::75d]) by smtp.gmail.com with ESMTPSA id a92af1059eb24-12c749d29cdsm16383260c88.6.2026.04.20.12.13.20 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Mon, 20 Apr 2026 12:13:21 -0700 (PDT) Message-ID: Date: Mon, 20 Apr 2026 16:13:17 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 1/6] gdb/record: Refactor record history To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org References: <20260415185836.2732968-1-guinevere@redhat.com> <20260415185836.2732968-2-guinevere@redhat.com> <87tst9cedk.fsf@linaro.org> From: Guinevere Larsen In-Reply-To: <87tst9cedk.fsf@linaro.org> X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: i6X36zN8mpgp_sT0_o_CKSUi3_kqkb7kajjDWARnGR8_1776712404 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit 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 4/17/26 9:15 PM, Thiago Jung Bauermann wrote: > Hello Guinevere, > > Nice improvement! Just a few comments, mostly nits. Thanks for properly reviewing this beast of a change! > > 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, struct regcache *regcache, >> aarch64_record.aarch64_mems[rec_no].len)) >> ret = -1; >> >> - if (record_full_arch_list_add_end ()) >> - ret = -1; >> } >> >> deallocate_reg_mem (&aarch64_record); > ⋮ >> 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, struct regcache *regcache, >> } >> } >> >> - if (record_full_arch_list_add_end ()) >> - ret = -1; >> } >> >> > FWIW the aarch64-tdep.c and arm-tdep.c changes are obvious. > > ⋮ >> /* 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 now. fixed! > >> -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 = rec->type; >> >> - 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; >> } > ⋮ >> -/* 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 = rec->next; >> - >> - rec->next = NULL; >> - while (tmp) >> + for (int i = record_full_list.size (); i >= 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? oh, nice catch. Not only is it invalid, it then makes the code leak memory since we're never calling the cleanups. Fixed > > 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. Huh. Yeah, all the more reason to modernize the code, this had totally escaped me. Will update to keep the previous behavior. > >> { >> - rec = tmp->next; >> - if (record_full_entry_release (tmp) == record_full_end) >> - { >> - record_full_insn_num--; >> - record_full_insn_count--; >> - } >> - tmp = 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 = index; >> +} >> + >> +static void >> +record_full_save_instruction () >> +{ >> + ++record_full_insn_count; >> + record_full_incomplete_instruction.insn_num = 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 '++'. fixed >> @@ -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). >> >> - 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/ fixed > >> then that is a user error. >> > ⋮ >> @@ -1322,21 +1223,6 @@ record_full_wait_1 (struct target_ops *ops, >> record_full_stop_reason = TARGET_STOPPED_BY_NO_REASON; >> status->set_stopped (GDB_SIGNAL_0); >> >> - /* Check breakpoint when forward execute. */ >> - if (execution_direction == EXEC_FORWARD) >> - { >> - tmp_pc = 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. I thought this was just an unnecessary piece of code, and that I was going to need to rework it because of the refactor, and decided to just remove it. Now that the refactor is done, I realize I was wrong... and considering Linaro CI found some regressions, I'm guessing these might be related. I'll explore this a little bit, but most likely I'll re-add this for v2. > > ⋮ >> - replay_out: >> + if (record_full_next_insn < 0) >> + { >> + gdb_assert (execution_direction == EXEC_REVERSE); >> + record_full_next_insn = 0; >> + } >> + else if (record_full_next_insn > record_full_list.size ()) >> + { >> + gdb_assert (execution_direction == EXEC_FORWARD); >> + record_full_next_insn = record_full_list.size (); >> + } >> + /* Reset the current instruciton to point to the one to be replayed >> + moving forward. */ >> + else if (execution_direction == EXEC_REVERSE) >> + record_full_next_insn++; >> + >> + //replay_out: > This commented-out label should be removed. fixed. > >> if (status->kind () == TARGET_WAITKIND_STOPPED) >> { >> + int insn = (execution_direction == 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 != 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); >> } > ⋮ >> @@ -2339,19 +2157,98 @@ netorder32 (uint32_t input) >> return ret; >> } >> >> +static void >> +record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int *bfd_offset) >> +{ >> + uint8_t rectype; >> + uint32_t regnum, len; >> + uint64_t addr; >> + regcache *cache = get_thread_regcache (inferior_thread ()); >> + >> + bfdcore_read (cbfd, osec, &rectype, sizeof (rectype), >> + bfd_offset); > This line fits in 80 columns. fixed > > 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)); >> >> - /* Restore the entries in recfd into record_full_arch_list_head and >> - record_full_arch_list_tail. */ >> - record_full_arch_list_head = NULL; >> - record_full_arch_list_tail = NULL; >> record_full_insn_num = 0; >> >> try >> { >> - regcache *regcache = get_thread_regcache (inferior_thread ()); >> > This empty line should also be removed. fixed. > >> - while (1) >> + while (bfd_offset < osec_size) >> { >> - uint8_t rectype; >> - uint32_t regnum, len, signal, count; >> - uint64_t addr; >> > This empty line should also be removed. fixed. > >> - /* We are finished when offset reaches osec_size. */ >> - if (bfd_offset >= osec_size) >> - break; >> - bfdcore_read (&cbfd, osec, &rectype, sizeof (rectype), &bfd_offset); >> + record_full_reset_incomplete (); >> + uint32_t eff_count = 0; >> + uint8_t sigval; >> + uint32_t insn_num; >> >> - switch (rectype) >> - { >> - case record_full_reg: /* reg */ >> - /* Get register number to regnum. */ >> - bfdcore_read (&cbfd, osec, ®num, sizeof (regnum), >> - &bfd_offset); >> - regnum = netorder32 (regnum); >> - >> - rec = 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. :) > Fixed these, and will take a look at future calls as well :) These went through so many iterations, no wonder I forgot to keep reformatting them lol, but thanks for checking! -- Cheers, Guinevere Larsen It/she