From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 3PKlLoD+4mkaRyMAWB0awg (envelope-from ) for ; Fri, 17 Apr 2026 23:46:08 -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=uIevTs66; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id A724E1E0B1; Fri, 17 Apr 2026 23:46:08 -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 C4C0F1E04F for ; Fri, 17 Apr 2026 23:46:06 -0400 (EDT) Received: from vm01.sourceware.org (localhost [127.0.0.1]) by sourceware.org (Postfix) with ESMTP id F000A4AA51EE for ; Sat, 18 Apr 2026 03:46:05 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org F000A4AA51EE 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=uIevTs66 Received: from mail-dl1-x122e.google.com (mail-dl1-x122e.google.com [IPv6:2607:f8b0:4864:20::122e]) by sourceware.org (Postfix) with ESMTPS id C63364AA54C3 for ; Sat, 18 Apr 2026 03:45:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org C63364AA54C3 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 C63364AA54C3 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2607:f8b0:4864:20::122e ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1776483939; cv=none; b=P4nvOFmkvnm3T0UG+2LChbEP8ssG1lybmkRkskN2jGVByWasdbMGqKm05bUa/CJ1lwwU3O175zBUQU6xrx4eTOthklPH5dHbstpfR27FU6eQMexEiuQggO02pz8aMSXLYPE3/7+aGvO/DE9wUQUIhQ7xAmCDGX8d1ld+hqoLJ2o= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1776483939; c=relaxed/simple; bh=dEoG1XlFUshngVZZHlcz75P1JkxSJnPl8zhoHS5hwt0=; h=DKIM-Signature:From:To:Subject:Date:Message-ID:MIME-Version; b=kJzNCXaJBggEZW/L8nxuO+nGbaCl/z+/16l1R3YV5zKQ3T04A//AKteVt6+OYPAr98c9mIYvEU0XS+4zWGsZa8+JzS30ZURoR52WoqUm0OZErEJCNWxa5bIrZytsRWx4Eo7GKiKHkn7x2CChJC2B8gHbSw9EtLCAriNSgztBxs4= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org C63364AA54C3 Received: by mail-dl1-x122e.google.com with SMTP id a92af1059eb24-12c726f46baso1807805c88.1 for ; Fri, 17 Apr 2026 20:45:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=linaro.org; s=google; t=1776483937; x=1777088737; darn=sourceware.org; h=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=JpMHZv3xDqKX356WBeOcKpHyACM7aRxHbZEIBqx7S+U=; b=uIevTs66gIdMtQ88/VZ18nA/yWq1a7sSNEmhEOYHAwtURziArlM7+2O9u9ryKOofv/ ojn4SLy/dU8Dd8152ORtVrsXrO0stWac78UEUlcR6KMGEddMbV8lOnSbpYFoFFzRdTXX HH0erHGgZRYMvIy9bTBS7Kl5FgxFowZECTqcKXb4sJslch44kH6KGsqC5C2QuXhFlQq9 jslqu9grzk2eMEBW+XAhpCsO5dgBLdWXZ84WelDSsujATk1IIh7xSa9xhzgqMMrwj+4h mLQTv+Iw7gyXw1RhgGj/gMPnB4q4/wHn7OQ3wOV2yHjOpefIHo/2fWUqkc8l2PduUuO4 1kOA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1776483937; x=1777088737; h=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=JpMHZv3xDqKX356WBeOcKpHyACM7aRxHbZEIBqx7S+U=; b=dSMzKvQcWl4JsXA1BRjFU/i6HcpJT4Wj81NUIdO5PGoklsIaARTUgAaAz7e7T6YMuG 4avjKdnuXwc4eXmeKsd7ADuSXdXPT70iaWGZ+vfUUUJmJYinhRAoS+4mJNqpFAobJVLm qmrjP+f1KOrvlQB4RihctFZcodghH3VliwXrxUrPXUml/K9wr+LCUvEzF+4eNXNDp1no ry5nlfs8LEYxFLne4/L9eH9B02SM159oVwJH5piXcHsTl9Tn4HNn7XqQKmanmSa0NuOm S9XcEFB/MKZgMS1KsoiTM+C0sixijwuGHABTsuPMN2VUAvcJkZ2lF4jQGkqKSjaph6NL vNdw== X-Gm-Message-State: AOJu0YwR+ynQ7boxnSvpTPugWQCHPFnqi/H8QCbf40T9XpQvop095YK4 MHEQeg5Sw9mL5VdKPVWnAB4VFxUJ8l/Ezpd/mdY6G/7lA444zKMHaEuOAEo/E1sT006OJokEFtm dCIfj X-Gm-Gg: AeBDievDhBjaGq3r/DjkEOEtY5fkGwe4zgNTIDKKrUADvAeFf7GWdd/MREFbgJza9r8 HOKbc1UpRF7CIjviBFuPOHkkgetYKk0AGRCJ50/edEj/xkWTt0uX4mofIBrwSvBeoomo+g+o6Cs SUHHkrxCDi9tUQyfnRYNluiM1NtLSYUSyZrEL7Jomg8JELiIOjuzN5z+bAPhy47ru99XRi7r9vH hGbOR4trkadD9mHEM4asLqR9zR8MN6hhanlhdy8SARdaqTIrscnZ74MpSKCOJHz5o4UAmZ5bauA br9ECleA13jqCLQ3EjISE9cQz9LoIntNPR74KzLYqaHLvTXHHrTvuZ9aOrsd513KhrRO8tX//Uj DqAQQNbAM4smBuKXhZfmrC/gi9LDcCRysQOVKlSjLI9Uw40c4USSeflxLHKqmlJ7ptl3SxNC4z5 xz/d2PvWd2mX8vnNSNX0lDDlwucaSv/C8/W2g5fI4yKDhp X-Received: by 2002:a05:7022:e22:b0:119:e56c:18ab with SMTP id a92af1059eb24-12c73f90bffmr2427588c88.19.1776483936699; Fri, 17 Apr 2026 20:45:36 -0700 (PDT) Received: from localhost ([2804:14d:7e39:8083:f04c:42e3:5943:38f6]) by smtp.gmail.com with ESMTPSA id a92af1059eb24-12c749c422csm5032310c88.3.2026.04.17.20.45.35 (version=TLS1_3 cipher=TLS_AES_256_GCM_SHA384 bits=256/256); Fri, 17 Apr 2026 20:45:36 -0700 (PDT) From: Thiago Jung Bauermann To: Guinevere Larsen Cc: gdb-patches@sourceware.org Subject: Re: [PATCH 3/6] gdb/record: c++ify internal structures of record-full.c In-Reply-To: <20260415185836.2732968-4-guinevere@redhat.com> (Guinevere Larsen's message of "Wed, 15 Apr 2026 15:58:33 -0300") References: <20260415185836.2732968-1-guinevere@redhat.com> <20260415185836.2732968-4-guinevere@redhat.com> User-Agent: mu4e 1.14.0; emacs 30.2 Date: Sat, 18 Apr 2026 00:45:33 -0300 Message-ID: <87bjfhaq3m.fsf@linaro.org> MIME-Version: 1.0 Content-Type: text/plain 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 Guinevere Larsen writes: > This commit adds a constructor, destructor, and some methods to the > structures record_full_entry, record_full_reg_entry and > record_full_mem_entry. This is a move to disentangle the internal > representation of the data and how record-full manipulates it for > replaying. > > Along with this change, record_full_entry is changed to use an > std::variant, since it was basically doing that already, but now we have > the stdlibc++ error checking to make sure we're only accessing elements > we're allowed to. Always nice to see these C++ification patches. Some comments below. > --- > gdb/record-full.c | 517 +++++++++++++++++++++++++--------------------- > 1 file changed, 279 insertions(+), 238 deletions(-) > > diff --git a/gdb/record-full.c b/gdb/record-full.c > index 95776679f21..f3737fdbc1f 100644 > --- a/gdb/record-full.c > +++ b/gdb/record-full.c > @@ -48,6 +48,7 @@ > #include "cli/cli-style.h" > > #include > +#include > > /* This module implements "target record-full", also known as "process > record and replay". This target sits on top of a "normal" target > @@ -90,12 +91,87 @@ struct record_full_mem_entry > int len; > /* Set this flag if target memory for this entry > can no longer be accessed. */ > - int mem_entry_not_accessible; > + bool mem_entry_not_accessible; In general I think bools that have "not" in their name are confusing. IMHO it's more straightforward to think about mem_entry_accessible than about mem_entry_not_accessible. The code was already like this before your patch, but if you agree with my point this could be a good opportunity to change it. > union > { > gdb_byte *ptr; > gdb_byte buf[sizeof (gdb_byte *)]; > } u; > + > + record_full_mem_entry () : addr (0), len (0) { } > + > + record_full_mem_entry (CORE_ADDR mem_addr, int mem_len) > + { > + addr = mem_addr; > + len = mem_len; > + if (len > sizeof (u.buf)) > + u.ptr = new gdb_byte[len]; > + mem_entry_not_accessible = false; > + } > + > + gdb_byte *get_loc () > + { > + if (len > sizeof (u.buf)) > + return u.ptr; > + else > + return u.buf; > + } > + > + bool execute (struct regcache *regcache, > + struct gdbarch *gdbarch) Two nits: The above fits in one line. Also, I suggest removing the "struct" keywords. A bit less of a nit: the regcache includes a gdbarch, so I think it's better to use it from there rather than pass it as a separate argument. > + { > + /* Nothing to do if the memory is flagged not_accessible. */ > + if (!mem_entry_not_accessible) This is a matter of personal preference, but in cases like this I like to do an early return: if (mem_entry_not_acessible) return false; Then the rest of the function doesn't need to be fully inside the if block. I think it's easier to read, and also saves one level of indentation. Also, the if condition above is a good illustration of my comment about the variable name. It took me a moment to realize what !mem_entry_not_accessible means. > + { > + gdb::byte_vector buf (len); > + > + if (record_debug > 1) This function isn't indented correctly, starting with this if which isn't in the right column. > + gdb_printf (gdb_stdlog, > + "Process record: record_full_mem %s to " > + "inferior addr = %s len = %d.\n", > + host_address_to_string (this), > + paddress (gdbarch, addr), > + len); > + > + if (record_read_memory (gdbarch, > + addr, buf.data (), > + len)) > + mem_entry_not_accessible = 1; This is a bool now, so it's better to use true/false rather than 0/1. > + else > + { > + if (target_write_memory (addr, > + get_loc (), > + len)) > + { > + mem_entry_not_accessible = 1; This is a bool now, so it's better to use true/false rather than 0/1. > + if (record_debug) > + warning (_("Process record: error writing memory at " > + "addr = %s len = %d."), > + paddress (gdbarch, addr), > + len); > + } > + else > + { > + memcpy (get_loc (), buf.data (), > + len); > + > + /* We've changed memory --- check if a hardware > + watchpoint should trap. Note that this > + presently assumes the target beneath supports > + continuable watchpoints. On non-continuable > + watchpoints target, we'll want to check this > + _before_ actually doing the memory change, and > + not doing the change at all if the watchpoint > + traps. */ > + if (hardware_watchpoint_inserted_in_range > + (current_inferior ()->aspace.get (), > + addr, len)) > + return true; > + } > + } > + } > + return false; > + } > }; > > struct record_full_reg_entry > @@ -107,6 +183,42 @@ struct record_full_reg_entry > gdb_byte *ptr; > gdb_byte buf[2 * sizeof (gdb_byte *)]; > } u; > + > + record_full_reg_entry () : num (0), len (0) { } > + > + record_full_reg_entry (gdbarch *gdbarch, int regnum) > + { > + num = regnum; > + len = register_size (gdbarch, regnum); > + if (len > sizeof (u.buf)) > + u.ptr = new gdb_byte[len]; > + } > + > + gdb_byte *get_loc () > + { > + if (len > sizeof (u.buf)) > + return u.ptr; > + else > + return u.buf; > + } > + > + bool execute (struct regcache *regcache, > + struct gdbarch *gdbarch) Same comments here as in the execute prototype for the mem entry. > + { > + gdb::byte_vector buf (len); > + > + if (record_debug > 1) > + gdb_printf (gdb_stdlog, > + "Process record: record_full_reg %s to " > + "inferior num = %d.\n", > + host_address_to_string (this), > + num); > + > + regcache->cooked_read (num, buf.data ()); It's better to pass simply "buf" here rather than "buf.data ()". This way, the cooked_read called will be the one which gets an array_view and it will check that buf is big enough to get the register contents. > + regcache->cooked_write (num, get_loc ()); > + memcpy (get_loc (), buf.data (), len); > + return false; > + } > }; > > enum record_full_type > @@ -115,16 +227,82 @@ enum record_full_type > record_full_mem > }; > > -struct record_full_entry > +class record_full_entry > { > - enum record_full_type type; > - union > + std::variant entry; Instead of having this variant and the switches in all the methods, have you considered making this a base class with virtual methods? Then record_full_reg_entry and record_full_mem_entry would implement them as appropriate. > +public: > + record_full_entry () : entry (record_full_reg_entry ()) {} > + > + /* Constructor for a register entry. Type is here to make it > + easier to recognize it in the constructor calls, it isn't > + actually important. */ > + record_full_entry (record_full_type reg_type, gdbarch *gdbarch, > + int regnum) > + : entry(record_full_reg_entry (gdbarch, regnum)) > { > - /* reg */ > - struct record_full_reg_entry reg; > - /* mem */ > - struct record_full_mem_entry mem; > - } u; > + gdb_assert (reg_type == record_full_reg); > + } > + > + record_full_entry (record_full_type mem_type, CORE_ADDR addr, int len) > + : entry(record_full_mem_entry (addr, len)) > + { > + gdb_assert (mem_type == record_full_mem); > + } > + > + record_full_reg_entry& reg () > + { > + gdb_assert (type () == record_full_reg); > + return std::get (entry); > + } For this method, record_full_mem_entry would just have an gdb_assert_not_reached. > + > + record_full_mem_entry& mem () > + { > + gdb_assert (type () == record_full_mem); > + return std::get (entry); > + } Likewise for this method, record_full_reg_entry would just have an gdb_assert_not_reached. > + record_full_type type () > + { > + switch (entry.index ()) > + { > + case 0: > + return record_full_reg; > + case 1: > + return record_full_mem; > + } > + gdb_assert_not_reached ("Impossible variant index"); > + } > + > + /* Get the pointer to the data stored by this entry. */ > + gdb_byte *get_loc () > + { > + switch (type ()) > + { > + case record_full_reg: > + return reg ().get_loc (); > + case record_full_mem: > + return mem ().get_loc (); > + } > + gdb_assert_not_reached ("Impossible entry type"); > + } > + > + /* Execute this entry, swapping the appropriate values from memory or > + register and the recorded ones. Returns TRUE if the execution was > + stopped by a watchpoint. */ > + > + bool execute (struct regcache *regcache, > + struct gdbarch *gdbarch) > + { > + switch (type ()) > + { > + case record_full_reg: > + return reg ().execute (regcache, gdbarch); > + case record_full_mem: > + return mem ().execute (regcache, gdbarch); > + } > + return false; > + } The methods above look like dynamic dispatch, but implemented by hand. :) > }; > > /* This is the main structure that comprises the execution log. > @@ -381,61 +559,30 @@ static struct cmd_list_element *record_full_cmdlist; > static void record_full_goto_insn (size_t target_insn, > enum exec_direction_kind dir); > > -/* Initialization and cleanup functions for record_full_reg and > - record_full_mem entries. */ > - > -/* Init a record_full_reg record entry. */ > - > -static inline struct record_full_entry > -record_full_reg_init (struct regcache *regcache, int regnum) > -{ > - struct record_full_entry rec; > - struct gdbarch *gdbarch = regcache->arch (); > - > - rec.type = record_full_reg; > - rec.u.reg.num = regnum; > - rec.u.reg.len = register_size (gdbarch, regnum); > - if (rec.u.reg.len > sizeof (rec.u.reg.u.buf)) > - rec.u.reg.u.ptr = (gdb_byte *) xmalloc (rec.u.reg.len); > - > - return rec; > -} > - > -/* Cleanup a record_full_reg record entry. */ > +/* Cleanup a record_full_reg_entry. This would ideally be a > + destructor for the classes, but I kept running into issues with > + double free, so this is left as a future improvement. */ Even if for now it's not a destructor, this could be a method in record_full_reg_entry. > static inline void > record_full_reg_cleanup (struct record_full_entry rec) > { > - gdb_assert (rec.type == record_full_reg); > - if (rec.u.reg.len > sizeof (rec.u.reg.u.buf)) > - xfree (rec.u.reg.u.ptr); > + gdb_assert (rec.type () == record_full_reg); > + auto reg = rec.reg (); > + if (reg.len > sizeof (reg.u.buf)) > + delete reg.u.ptr; > } > > -/* Init a record_full_mem record entry. */ > - > -static inline struct record_full_entry > -record_full_mem_init (CORE_ADDR addr, int len) > -{ > - struct record_full_entry rec; > - > - rec.type = record_full_mem; > - rec.u.mem.addr = addr; > - rec.u.mem.len = len; > - if (rec.u.mem.len > sizeof (rec.u.mem.u.buf)) > - rec.u.mem.u.ptr = (gdb_byte *) xmalloc (len); > - rec.u.mem.mem_entry_not_accessible = 0; > - > - return rec; > -} > - > -/* Cleanup a record_full_mem record entry. */ > +/* Cleanup a record_full_mem_entry. This would ideally be a > + destructor for the classes, but I kept running into issues with > + double free, so this is left as a future improvement. */ Likewise, this could be a method in record_full_reg_entry. > > static inline void > record_full_mem_cleanup (struct record_full_entry rec) > { > - gdb_assert (rec.type == record_full_mem); > - if (rec.u.mem.len > sizeof (rec.u.mem.u.buf)) > - xfree (rec.u.mem.u.ptr); > + gdb_assert (rec.type () == record_full_mem); > + auto mem = rec.mem (); > + if (mem.len > sizeof (mem.u.buf)) > + delete mem.u.ptr; > } > > /* Free one record entry, any type. > @@ -444,8 +591,8 @@ record_full_mem_cleanup (struct record_full_entry rec) > static inline void > record_full_entry_cleanup (struct record_full_entry rec) > { > - > - switch (rec.type) { > + switch (rec.type ()) > + { > case record_full_reg: > record_full_reg_cleanup (rec); > break; And this could be a (virtual?) method in record_full_entry. > @@ -518,33 +665,12 @@ record_full_arch_list_add (struct record_full_entry &rec) > record_full_incomplete_instruction.effects.push_back (rec); > } > > -/* Return the value storage location of a record entry. */ > -static inline gdb_byte * > -record_full_get_loc (struct record_full_entry *rec) > -{ > - switch (rec->type) { > - case record_full_mem: > - if (rec->u.mem.len > sizeof (rec->u.mem.u.buf)) > - return rec->u.mem.u.ptr; > - else > - return rec->u.mem.u.buf; > - case record_full_reg: > - if (rec->u.reg.len > sizeof (rec->u.reg.u.buf)) > - return rec->u.reg.u.ptr; > - else > - return rec->u.reg.u.buf; > - default: > - gdb_assert_not_reached ("unexpected record_full_entry type"); > - return NULL; > - } > -} > - > /* Record the value of a register NUM to record_full_arch_list. */ > > int > record_full_arch_list_add_reg (struct regcache *regcache, int regnum) > { > - struct record_full_entry rec; > + struct record_full_entry rec (record_full_reg, regcache->arch (), regnum); > > if (record_debug > 1) > gdb_printf (gdb_stdlog, > @@ -552,9 +678,7 @@ record_full_arch_list_add_reg (struct regcache *regcache, int regnum) > "record list.\n", > regnum); > > - rec = record_full_reg_init (regcache, regnum); > - > - regcache->cooked_read (regnum, record_full_get_loc (&rec)); > + regcache->cooked_read (regnum, rec.get_loc ()); Just a suggestion, feel free to adopt or ignore: if get_loc () returned a gdb::array_view, this cooked_read could be the one with bounds checking. > > record_full_arch_list_add (rec); > -- Thiago