From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id 3H+YCe8R6WkvHzQAWB0awg (envelope-from ) for ; Wed, 22 Apr 2026 14:22:39 -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=RtNiCyCs; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 161E81E0C3; Wed, 22 Apr 2026 14:22:39 -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 E0CC51E093 for ; Wed, 22 Apr 2026 14:22:37 -0400 (EDT) Received: from vm01.sourceware.org (localhost [127.0.0.1]) by sourceware.org (Postfix) with ESMTP id 3BA044422C70 for ; Wed, 22 Apr 2026 18:22:37 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3BA044422C70 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=RtNiCyCs Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 307C44BBCDC6 for ; Wed, 22 Apr 2026 18:11:12 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 307C44BBCDC6 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 307C44BBCDC6 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1776881472; cv=none; b=Z8SwMfdqj/DjgEAaEDz/T9bJl4ZMiN881QHgECx8oP77UMc1ximDzQsN6pmjjCQzsLYjsPnecqonrcFJ0Aa0BurQ3qiIxPO6S93SF6IAxI4xiMlM1FLD81xTThsfHIGvakde/8TVCsoWSJAn3w+hkbSAWaajs8HYOo8pHY1slG8= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1776881472; c=relaxed/simple; bh=q9Dt7jWo+8JOoknoykYOqVulNVkXbgVix3T1vDu3na8=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=eO3vuk1du7Tos6eG0T/wmNKF1lmuuq4MLnGifKzWpil0C322N1eVDSqvWJ9vHuNrBaff4utGEE//CgQsnYSqeXv6qAK+FurtE7Aqy6+Ak6Pca4gaJ0/ORnuZ7+zP2TD0czMYn3FC0qaGlzjv4nWVY3dGy/EMW7YdYUbWluq4PjA= ARC-Authentication-Results: i=1; server2.sourceware.org DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 307C44BBCDC6 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1776881471; 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=6y36RnWNC1MiWpk2uF6+MU/SZcoyucKptwdKY65DIQk=; b=RtNiCyCsE/DFfwumEgVS0oGu9aXhxvJxdRQDigvGHGIekNadC9okMGSW24qmEQpqJLno6H pCnsJxfbg108nvugHyNK9yQrddzdP3a5A59ZTBtXdjgiODXN1y616v8KDqpasGh2ENY4Ki k6du/4de+aZY86q4ywcjNKjMJIqUscg= Received: from mail-ua1-f70.google.com (mail-ua1-f70.google.com [209.85.222.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-258-ApKDHUwjM-KJ3dpYqCgGLA-1; Wed, 22 Apr 2026 14:11:10 -0400 X-MC-Unique: ApKDHUwjM-KJ3dpYqCgGLA-1 X-Mimecast-MFC-AGG-ID: ApKDHUwjM-KJ3dpYqCgGLA_1776881470 Received: by mail-ua1-f70.google.com with SMTP id a1e0cc1a2514c-953a7e2c35fso2744066241.3 for ; Wed, 22 Apr 2026 11:11:10 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1776881470; x=1777486270; 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=6y36RnWNC1MiWpk2uF6+MU/SZcoyucKptwdKY65DIQk=; b=BdJ0nwXz+zFv02FIxuz/PfIUJ07ajic2ZY76HlxT/who4G80aqcoDdLHeH1eloeGdq L4V7lJnD4bKSeGaRHp7IBV95v5F37n9A8IB1w4RMXcEgMjzQRjmrs5nPtFM1UKUxEmiW zpszeN/FcpXfT7Lnxtwg7SZvPiVXAt8h0k01SDh+uimPmRClJBm0phuiZ7NAlGNaG28v zoJJvXnnLCX2RtG+nK+WkWa/UhILgQq2jmMPb2dGPhY8dzR0naBrO6DaS85FAbfUQAGr jXz3L7Rq62L7IiNl+TFDeo63rsqXaI7TrMLcn1jVT3fbQEbpv6+82xrt+tcFFG+TCfsr O66w== X-Gm-Message-State: AOJu0YyEY3pLlXeIK7OeoabmbiSQ/0RN7xQ0vDl142VZA4aoWZ5i+srz xC0uel5no9WwInv8tDdx3IAzGv4CVx3Fm47teW7kAoQyQ9mg1NjkkvcjXPOwGue6UrkQ/4Uienh 8lFe1dBx3yjwnLcU/LW9yOKOYMo4/c3Oinp+pxWoltiZQh0EXMFEDs/tC0zEehdweWjuzk3Y= X-Gm-Gg: AeBDieucGKTX1/fBAx5DaK7V2UxGXdejeE5dKT1ucdS6WXHRcrTrdacRZGVL36OWXvd cSnPnELMcyDBRYIjEm7z2AepLkMB9wRXwGcTj7UIqIZM0GnT5V/hHCV/eME3LpDt212hOTLypxb wI3clczQ/geNIz+e9mELVTslY03Wts3yQzaNYOIeZruuzzC6THJO3H+XsXkagNppv0iGC07L1pK Ocp63CRnce7tX3ynUTcJpPlA7k2Gm1qZOqZIAJpr61VpqdzqHwzuHJtst20iZuipHRoVLUJlfqT prLRiHOX9O7eR/l0Qw40+t8AoYW7bneWWGY7RpR51ClazUUV4pTs/PofMSmGVvfQo9llXysL4k0 R1/Aete+sgI/Bg+H0S7dDW0zceD+eNA2r9JF+u0T+PQ== X-Received: by 2002:a05:6122:a20f:b0:56f:1c32:bcfa with SMTP id 71dfb90a1353d-56fa59dcc4dmr9377144e0c.11.1776881469240; Wed, 22 Apr 2026 11:11:09 -0700 (PDT) X-Received: by 2002:a05:6122:a20f:b0:56f:1c32:bcfa with SMTP id 71dfb90a1353d-56fa59dcc4dmr9377076e0c.11.1776881468530; Wed, 22 Apr 2026 11:11:08 -0700 (PDT) Received: from ?IPV6:2804:14d:8084:993e::75d? ([2804:14d:8084:993e::75d]) by smtp.gmail.com with ESMTPSA id a1e0cc1a2514c-9589097ec5csm8192111241.4.2026.04.22.11.11.06 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 22 Apr 2026 11:11:08 -0700 (PDT) Message-ID: Date: Wed, 22 Apr 2026 15:11:03 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH 3/6] gdb/record: c++ify internal structures of record-full.c To: Thiago Jung Bauermann Cc: gdb-patches@sourceware.org References: <20260415185836.2732968-1-guinevere@redhat.com> <20260415185836.2732968-4-guinevere@redhat.com> <87bjfhaq3m.fsf@linaro.org> From: Guinevere Larsen In-Reply-To: <87bjfhaq3m.fsf@linaro.org> X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: 4-RiaOSHsmJPiQuXRZMbmAo0aKPejtm8YZl_8OA7o6E_1776881470 X-Mimecast-Originator: redhat.com Content-Language: en-US Content-Type: text/plain; charset=UTF-8; format=flowed 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 4/18/26 12:45 AM, Thiago Jung Bauermann wrote: > 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. Yeah, fair point. I will update 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. Fixed. > 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. Huh, turns out, the regcache is not even needed in this function, I must have not looked closely at the code I copy pasted > >> + { >> + /* 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. Yeah, I think this makes sense. Went with this option. > > 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. fixed > >> + 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. fixed > >> + 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. fixed > >> + 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. this is the opposite, no gdbarch needed :) > >> + { >> + 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. Good point, fixed > >> + 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. I have considered it. The reason I didn't go with it is because polymorphism would need pointers as opposed to references or just variables, and so the effects vector in record_full_instruction would need to hold pointers to the base class, still requiring us to manually handle memory and things. So in the end it felt like it would net more complexity rather than less... or maybe a similar amount of complexity, but one that would be more confusing to follow in my opinion. > >> +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. good point, I've implemented it. > >> 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. done too. >> >> 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. also done (non-virtual for the previously explained reasons) > >> @@ -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. this seems like a non-trivial change, so considering how there should be no way to get that wrong with the current code I'll pass on it. > >> >> record_full_arch_list_add (rec); >> -- Cheers, Guinevere Larsen It/she