From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id BIndDIdQF2q7ByEAWB0awg (envelope-from ) for ; Wed, 27 May 2026 16:13:59 -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=e0IqC34q; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id 211081E0A3; Wed, 27 May 2026 16:13:59 -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 E72691E024 for ; Wed, 27 May 2026 16:13:53 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 3099B4BA2E2C for ; Wed, 27 May 2026 20:13:53 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 3099B4BA2E2C 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=e0IqC34q 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 348464BA2E2A for ; Wed, 27 May 2026 20:13:22 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 348464BA2E2A 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 348464BA2E2A Authentication-Results: sourceware.org; arc=none smtp.remote-ip=170.10.129.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1779912802; cv=none; b=ar7n76sFFha/8bl3lrpJP59rz/bLlFvGkpBLF5gi9VlnMZozJcm0lZS6k4PAvn+deuRpcZ+sjjGks9kgkYSYejd256vtajEreSt/tzYfCqe2pfBd3moy9R1r+c/h22uF8rNIZhwP/IYMVr/snO26WYF3YG5KOTXPeWONo00ok18= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1779912802; c=relaxed/simple; bh=B7tpcDhBH7M/ikQkSv/p5zccicKEWF/wWO19dToWsv0=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=Tb532faDrPryj0wazy0f/kcm90gR/GZ9q5x5szycjXAR0CpYFBhIS1OjIeSmePOD4YS7BNES4eUYlZUDP7O3e99RRfTMDQBbbbaxJxhJUMsDQyUVxDIkmP+nOoLQtU+oEmK8syaeIrsVp4MxE5zWY31Kz5Nxm2q1NkH/GRLnG3A= ARC-Authentication-Results: i=1; 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=e0IqC34q DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 348464BA2E2A DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1779912801; 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=XvqM96HHCO9pApK1eUz8Yt9EoYQHlNSpPWkU62tBXZE=; b=e0IqC34qHr/1kmIHQeDHxrVcaXfehWcyVxH7Ur664LBNR5Bb2mZ6xMg1B+bkPsl54xlrql kCrN/XEuYgcDIPYXfwi93sQqtGtjNvOd7TUJi+HAbo8R5UoYPh/9zlyfHWYwlqD+s0xUyx 5dX/LtkoeQoM8MVREbEjKGrti4jdzzk= Received: from mail-vs1-f71.google.com (mail-vs1-f71.google.com [209.85.217.71]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-691-ZkOsSDQvOYWueIE4lh06zA-1; Wed, 27 May 2026 16:13:20 -0400 X-MC-Unique: ZkOsSDQvOYWueIE4lh06zA-1 X-Mimecast-MFC-AGG-ID: ZkOsSDQvOYWueIE4lh06zA_1779912800 Received: by mail-vs1-f71.google.com with SMTP id ada2fe7eead31-6332db4182dso15408619137.0 for ; Wed, 27 May 2026 13:13:20 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1779912799; x=1780517599; 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=XvqM96HHCO9pApK1eUz8Yt9EoYQHlNSpPWkU62tBXZE=; b=OfUa5ioz7w649VE/Zq3xy1djY6tCAuVW8O8imLM2ZSjBB4r24saj24491AUUqF9wAj aeSmhmstq+x6jTD8WP1MnTkx8hINC3YWHgp6FL5QVeR9vSa40q1h64qrfwQVZB/FvRZM qnUnorfRC5hi4kp43a59YX7PpUR9soIs4OL9WWKm8igPiu+lcKkp23OZk2rPtEQ6cjrs 718AmoR3618gqH8z7qgdxuhOrcQJSIwd9Bs/o7fP5Lqlm57KTlJt/vfPI66zON8Qb0Kl coysMSny8i3mo51mJ63/+Ns8Ych0uLWI5ida2tZ5kgYHVy+Wv8DwyDJ4U6fJ70TBHZef 0HiQ== X-Forwarded-Encrypted: i=1; AFNElJ/4NqHHzEdHf2m5eeoZz7SiATYmGGuFkcDOxeY3Knf2E8aBMWMifPNxfWD7JvTd3TmkN79ADILeNg0Wqg==@sourceware.org X-Gm-Message-State: AOJu0YwbctR4O3zwZJzAE2XR8mUMYDTF3A6ahyGs1+xkOK7Jcoiii5xZ KTJl2GqG0cgmU80dRKknzhyEXXGGXuE5MZejtCYrXo0rHxu7KNGqBOaOoSzOSUWvbJcJWT+kZCx kDiUK6ySYG14Vp1L88AkCqqcpk+/TIoqjENc6FZdr9LA4DFobXW8eaLEVI0FiaydPpEm1SEk= X-Gm-Gg: Acq92OGlQ36jdLSwAFLxmn/lR5ERQBvAcY5msOEFnH03zQ8bGR2lLTnQ8WUrMZ9kcjP V+auoJ4ZxCzOkpZojkwPZtIVspyv1Wj1VU3gIeeyBOwj/SIkJYszmaPHWM5Asb4soxIw0yGyO/O +xlrL64tYZDci/NhQeReg8J2oFYozlTGBgFEP8VVzL03yvAbMxNfYuLZPxsZHzWhkCV0YHbq9oq wqGpOWUqd3e0DvpaaPkwWlVDMMNFWYogjLeZc/kyUAEGCFZ40U0BxbjtDLFxdigqTFx6tQ1pQbs SBSJW5H9JIwMII9hcqWrEZ/kn9IZ/5crzkAqcXhNth3LJjq3bqUZIWBVPjksAoWegkbDz1gydQG qmtYwJha3ymdDDpx4bfeGVS7Kcae/2EkuFcj6rhPr5/Xr0TNn2knHVKersRYmj/aLprG6GYA5F5 YsAFA= X-Received: by 2002:a05:6102:fa2:b0:633:d7ec:153e with SMTP id ada2fe7eead31-67c83a8a4d3mr13605650137.28.1779912799494; Wed, 27 May 2026 13:13:19 -0700 (PDT) X-Received: by 2002:a05:6102:fa2:b0:633:d7ec:153e with SMTP id ada2fe7eead31-67c83a8a4d3mr13605643137.28.1779912798905; Wed, 27 May 2026 13:13:18 -0700 (PDT) Received: from ?IPV6:2804:14d:8084:993e:2d5d:7adb:2b6:a50e? ([2804:14d:8084:993e:2d5d:7adb:2b6:a50e]) by smtp.gmail.com with ESMTPSA id a1e0cc1a2514c-961738b2babsm18406374241.7.2026.05.27.13.13.16 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Wed, 27 May 2026 13:13:18 -0700 (PDT) Message-ID: <4ea1a202-e336-44fa-a9e2-2b71a66d9e88@redhat.com> Date: Wed, 27 May 2026 17:13:13 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 3/7] gdb/record: c++ify internal structures of record-full.c To: "Schimpe, Christina" , "gdb-patches@sourceware.org" Cc: Thiago Jung Bauermann References: <20260515163706.3355686-1-guinevere@redhat.com> <20260515163706.3355686-4-guinevere@redhat.com> From: Guinevere Larsen In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: yIIx111dEMAKBNahp8DmAH80QUxJ77mG-KnaBHpdATo_1779912800 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 5/26/26 6:00 AM, Schimpe, Christina wrote: >> -----Original Message----- >> From: Guinevere Larsen >> Sent: Freitag, 15. Mai 2026 18:37 >> To: gdb-patches@sourceware.org >> Cc: Guinevere Larsen ; Thiago Jung Bauermann >> >> Subject: [PATCH v3 3/7] gdb/record: c++ify internal structures of record-full.c >> >> 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. >> >> Reviewed-by: Thiago Jung Bauermann >> --- >> gdb/record-full.c | 601 +++++++++++++++++++++++++--------------------- >> 1 file changed, 321 insertions(+), 280 deletions(-) >> >> diff --git a/gdb/record-full.c b/gdb/record-full.c index >> e4dd07dc300..24e94eaf5da 100644 >> --- a/gdb/record-full.c >> +++ b/gdb/record-full.c >> @@ -51,6 +51,7 @@ >> #include >> #include >> #include >> +#include >> >> /* This module implements "target record-full", also known as "process >> record and replay". This target sits on top of a "normal" target @@ -93,12 >> +94,113 @@ 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_accessible; > Shouldn't we update the comment then, too ? Yes, good point! > Nit: you could consider moving this into a separate patch. I would make the review a bit easier IMO. > > The same applies for each structure/class that you refactor in this patch. > But this is rather an optional suggestion, too. 😊 Thiago suggested that, since I'm already messing with the class, I may as well invert the name and logic of the variable. I guess my thinking was that, since he already seen my previous version this wouldn't be much of a change to review, but you make a good point, will keep it in mind for the future > >> union >> { >> gdb_byte *ptr; >> gdb_byte buf[sizeof (gdb_byte *)]; >> } u; >> + >> + record_full_mem_entry () : addr (0), len (0) { } > Any specific reason why you don't init mem_entry_accessible and u here ? Mainly because I wanted to treat "len == 0" as effectively uninitialized, so it didn't seem important, but I see that I didn't actually follow through with it that much so I suppose I should at least initialize mem_entry_accessible. u seems unnecessary to setup, since we'll do 0-sized reads (which will return the local buffer and not a random pointer).... > >> + >> + 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_accessible = true; >> + } >> + >> + record_full_mem_entry (record_full_mem_entry &&other) { >> + addr = other.addr; >> + len = other.len; >> + memcpy (u.buf, other.u.buf, sizeof (u.buf)); >> + mem_entry_accessible = other.mem_entry_accessible; >> + /* With len == 0, OTHER is guaranteed to not try to free the >> + memory when it is destructed. */ >> + other.len = 0; >> + } >> + >> + ~record_full_mem_entry () >> + { >> + if (len > sizeof (u.buf)) >> + delete[] u.ptr; >> + } >> + >> + record_full_mem_entry &operator= (record_full_mem_entry &&other) { >> + addr = other.addr; >> + len = other.len; >> + memcpy (u.buf, other.u.buf, sizeof (u.buf)); > Shouldn't we check for self-assignment here ? I don't think it would be possible to self-assign here, since this is a move operator. We shouldn't be able to create a copy anywhere, so I think it shouldn't be a large thing. I can add an assert, since if it ever happens this is a large bug. > > Also, like the destructor, I believe we must delete u.ptr here: > ~~~ > if (len > sizeof (u.buf)) > delete[] u.ptr; > ~~~ > > Same feedback for record_full_mem_entry &operator= The idea of a move operation (constructor or assignment) is that we won't need to allocate new memory, we'll just pass the pointer to the new holder and that's it. If the pointer is deleted here, since it was copied over in the memcpy call, when the next object calls its destructor, we'll get a double free. > >> + mem_entry_accessible = other.mem_entry_accessible; >> + /* With len == 0, OTHER is guaranteed to not try to free the >> + memory when it is destructed. */ >> + other.len = 0; >> + return *this; >> + } >> + >> + DISABLE_COPY_AND_ASSIGN (record_full_mem_entry); >> + >> + gdb_byte *get_loc () > I don't think this is too important, but since I saw it: > This should be const I think. > > IIUC we should then also use a const reference in loops which call get_loc (), > like this one: > > for (auto &entry : to_print->effects) > > And the same applies for type, reg and mem. There's a good amount of places that would need changing for adding const to get_loc, I think, including bfdcore_read and bfdcore_write, so it feels like its too much for this patch. reg and mem throw errors about the get discarding qualifiers. I did add const to "type" though, thanks for pointing that out. > >> + { >> + if (len > sizeof (u.buf)) >> + return u.ptr; >> + else >> + return u.buf; >> + } >> + >> + bool execute (gdbarch *gdbarch) >> + { >> + /* Nothing to do if the memory is flagged not_accessible. */ >> + if (!mem_entry_accessible) >> + return false; >> + >> + gdb::byte_vector buf (len); >> + >> + if (record_debug > 1) >> + 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_accessible = false; >> + else >> + { >> + if (target_write_memory (addr, >> + get_loc (), >> + len)) >> + { >> + mem_entry_accessible = false; >> + 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 >> @@ -110,6 +212,70 @@ 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]; >> + } >> + >> + record_full_reg_entry (record_full_reg_entry &&other) { >> + num = other.num; >> + len = other.len; >> + memcpy (u.buf, other.u.buf, sizeof (u.buf)); >> + /* With len == 0, OTHER is guaranteed to not try to free the >> + memory when it is destructed. */ >> + other.len = 0; >> + } >> + >> + ~record_full_reg_entry () >> + { >> + if (len > sizeof (u.buf)) >> + delete[] u.ptr; >> + } >> + >> + record_full_reg_entry &operator=(record_full_reg_entry &&other) { >> + num = other.num; >> + len = other.len; >> + memcpy (u.buf, other.u.buf, sizeof (u.buf)); >> + /* With len == 0, OTHER is guaranteed to not try to free the >> + memory when it is destructed. */ >> + other.len = 0; >> + return *this; >> + } >> + >> + DISABLE_COPY_AND_ASSIGN (record_full_reg_entry); >> + >> + gdb_byte *get_loc () >> + { >> + if (len > sizeof (u.buf)) >> + return u.ptr; >> + else >> + return u.buf; >> + } >> + >> + bool execute (regcache *regcache) >> + { >> + 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); >> + regcache->cooked_write (num, get_loc ()); >> + memcpy (get_loc (), buf.data (), len); >> + return false; >> + } >> }; >> >> enum record_full_type >> @@ -118,16 +284,88 @@ enum record_full_type >> record_full_mem >> }; >> >> -struct record_full_entry >> +class record_full_entry >> { >> - enum record_full_type type; >> - union >> + std::variant entry; >> + >> +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_entry (record_full_entry &&other) >> + : entry (std::move (other.entry)) >> + { >> + } >> + >> + DISABLE_COPY_AND_ASSIGN (record_full_entry); >> + >> + record_full_reg_entry& reg () >> + { >> + gdb_assert (type () == record_full_reg); >> + return std::get (entry); } >> + >> + record_full_mem_entry& mem () >> + { >> + gdb_assert (type () == record_full_mem); >> + return std::get (entry); } >> + >> + 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 (regcache *regcache) >> + { >> + switch (type ()) >> + { >> + case record_full_reg: >> + return reg ().execute (regcache); >> + case record_full_mem: >> + return mem ().execute (regcache->arch ()); >> + } >> + return false; >> + } >> }; >> >> /* This is the main structure that comprises the execution log. >> @@ -384,91 +622,12 @@ 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 record_full_entry >> -record_full_reg_init (struct regcache *regcache, int regnum) -{ >> - 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. */ >> - >> -static inline void >> -record_full_reg_cleanup (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); >> -} >> - >> -/* Init a record_full_mem record entry. */ >> - >> -static inline record_full_entry >> -record_full_mem_init (CORE_ADDR addr, int len) -{ >> - 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. */ >> - >> -static inline void >> -record_full_mem_cleanup (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); >> -} >> - >> -/* Free one record entry, any type. */ >> - >> -static inline void >> -record_full_entry_cleanup (record_full_entry rec) -{ >> - >> - switch (rec.type) { >> - case record_full_reg: >> - record_full_reg_cleanup (rec); >> - break; >> - case record_full_mem: >> - record_full_mem_cleanup (rec); >> - break; >> - } >> -} >> - >> static void >> record_full_reset_history () >> { >> record_full_insn_count = 0; >> record_full_next_insn = 0; >> >> - for (auto &insn : record_full_list) >> - { >> - for (auto &entry : insn.effects) >> - record_full_entry_cleanup (entry); >> - } >> - >> record_full_list.clear (); >> } >> >> @@ -476,11 +635,7 @@ static void >> record_full_list_release_following (int index) { >> for (int i = record_full_list.size () - 1; i > index; i--) >> - { >> - for (auto &entry : record_full_list[i].effects) >> - record_full_entry_cleanup (entry); >> - record_full_list.pop_back (); >> - } >> + 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; >> @@ -509,9 +664,6 @@ record_full_list_release_first (void) >> if (record_full_list.empty ()) >> return; >> >> - for (auto &entry : record_full_list[0].effects) >> - record_full_entry_cleanup (entry); >> - >> record_full_list.pop_front (); >> --record_full_next_insn; >> } >> @@ -521,28 +673,7 @@ record_full_list_release_first (void) static void >> record_full_arch_list_add (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_full_incomplete_instruction.effects.push_back (std::move >> + (rec)); >> } >> >> /* Record the value of a register NUM to record_full_arch_list. */ @@ -550,7 >> +681,7 @@ record_full_get_loc (struct record_full_entry *rec) int >> record_full_arch_list_add_reg (struct regcache *regcache, int regnum) { >> - record_full_entry rec; >> + record_full_entry rec (record_full_reg, regcache->arch (), regnum); >> >> if (record_debug > 1) >> gdb_printf (gdb_stdlog, >> @@ -558,9 +689,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 ()); >> >> record_full_arch_list_add (rec); >> >> @@ -573,7 +702,7 @@ record_full_arch_list_add_reg (struct regcache >> *regcache, int regnum) int record_full_arch_list_add_mem (CORE_ADDR >> addr, int len) { >> - record_full_entry rec; >> + record_full_entry rec (record_full_mem, addr, len); >> >> if (record_debug > 1) >> gdb_printf (gdb_stdlog, >> @@ -584,14 +713,9 @@ record_full_arch_list_add_mem (CORE_ADDR addr, >> int len) >> if (!addr) /* FIXME: Why? Some arch must permit it... */ >> return 0; >> >> - rec = record_full_mem_init (addr, len); >> - >> if (record_read_memory (current_inferior ()->arch (), addr, >> - record_full_get_loc (&rec), len)) >> - { >> - record_full_mem_cleanup (rec); >> - return -1; >> - } >> + rec.get_loc (), len)) >> + return -1; >> >> record_full_arch_list_add (rec); >> >> @@ -719,98 +843,14 @@ record_full_gdb_operation_disable_set (void) >> static enum target_stop_reason record_full_stop_reason >> = TARGET_STOPPED_BY_NO_REASON; >> >> -/* Execute one instruction from the record log. Each instruction in >> - the log will be represented by an arbitrary sequence of register >> - entries and memory entries, followed by an 'end' entry. */ >> - >> -static inline void >> -record_full_exec_entry (regcache *regcache, >> - gdbarch *gdbarch, >> - record_full_entry *entry) >> -{ >> - switch (entry->type) >> - { >> - case record_full_reg: /* reg */ >> - { >> - gdb::byte_vector reg (entry->u.reg.len); >> - >> - if (record_debug > 1) >> - gdb_printf (gdb_stdlog, >> - "Process record: record_full_reg %s to " >> - "inferior num = %d.\n", >> - host_address_to_string (entry), >> - entry->u.reg.num); >> - >> - regcache->cooked_read (entry->u.reg.num, reg.data ()); >> - regcache->cooked_write (entry->u.reg.num, record_full_get_loc >> (entry)); >> - memcpy (record_full_get_loc (entry), reg.data (), entry->u.reg.len); >> - } >> - break; >> - >> - case record_full_mem: /* mem */ >> - { >> - /* Nothing to do if the entry is flagged not_accessible. */ >> - if (!entry->u.mem.mem_entry_not_accessible) >> - { >> - gdb::byte_vector mem (entry->u.mem.len); >> - >> - if (record_debug > 1) >> - gdb_printf (gdb_stdlog, >> - "Process record: record_full_mem %s to " >> - "inferior addr = %s len = %d.\n", >> - host_address_to_string (entry), >> - paddress (gdbarch, entry->u.mem.addr), >> - entry->u.mem.len); >> - >> - if (record_read_memory (gdbarch, >> - entry->u.mem.addr, mem.data (), >> - entry->u.mem.len)) >> - entry->u.mem.mem_entry_not_accessible = 1; >> - else >> - { >> - if (target_write_memory (entry->u.mem.addr, >> - record_full_get_loc (entry), >> - entry->u.mem.len)) >> - { >> - entry->u.mem.mem_entry_not_accessible = 1; >> - if (record_debug) >> - warning (_("Process record: error writing memory at " >> - "addr = %s len = %d."), >> - paddress (gdbarch, entry->u.mem.addr), >> - entry->u.mem.len); >> - } >> - else >> - { >> - memcpy (record_full_get_loc (entry), mem.data (), >> - entry->u.mem.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 (), >> - entry->u.mem.addr, entry->u.mem.len)) >> - record_full_stop_reason = >> TARGET_STOPPED_BY_WATCHPOINT; >> - } >> - } >> - } >> - } >> - break; >> - } >> -} >> - >> static inline void >> record_full_exec_insn (regcache *regcache, >> gdbarch *gdbarch, >> record_full_instruction &insn) { >> for (auto &entry : insn.effects) >> - record_full_exec_entry (regcache, gdbarch, &entry); >> + if (entry.execute (regcache)) >> + record_full_stop_reason = TARGET_STOPPED_BY_WATCHPOINT; >> } >> >> static void record_full_restore (struct bfd &cbfd); @@ -2185,21 +2225,19 >> @@ record_full_read_entry_from_bfd (bfd *cbfd, asection *osec, int >> *bfd_offset) >> bfdcore_read (cbfd, osec, ®num, sizeof (regnum), bfd_offset); >> regnum = netorder32 (regnum); >> >> - record_full_entry rec; >> - >> - rec = record_full_reg_init (cache, regnum); >> + record_full_entry rec (record_full_reg, cache->arch (), regnum); >> >> /* Get val. */ >> - bfdcore_read (cbfd, osec, record_full_get_loc (&rec), >> - rec.u.reg.len, bfd_offset); >> + bfdcore_read (cbfd, osec, rec.get_loc (), >> + rec.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, >> + rec.reg ().num, >> (unsigned long) sizeof (regnum), >> - rec.u.reg.len); >> + rec.reg ().len); >> >> record_full_arch_list_add (rec); >> break; >> @@ -2216,19 +2254,17 @@ record_full_read_entry_from_bfd (bfd *cbfd, >> asection *osec, int *bfd_offset) >> bfd_offset); >> addr = netorder64 (addr); >> >> - record_full_entry rec; >> - rec = record_full_mem_init (addr, len); >> + record_full_entry rec (record_full_mem, addr, len); >> >> /* Get val. */ >> - bfdcore_read (cbfd, osec, record_full_get_loc (&rec), len, >> - bfd_offset); >> + bfdcore_read (cbfd, osec, rec.get_loc (), len, bfd_offset); >> >> if (record_debug) >> gdb_printf (gdb_stdlog, >> " Reading memory %s (1 plus " >> "%lu plus %lu plus %d bytes)\n", >> paddress (get_current_arch (), >> - rec.u.mem.addr), >> + rec.mem ().addr), >> (unsigned long) sizeof (addr), >> (unsigned long) sizeof (len), >> len); >> @@ -2380,52 +2416,56 @@ record_full_write_entry_to_bfd >> (record_full_entry &entry, >> uint32_t regnum, len; >> uint64_t addr; >> >> - type = entry.type; >> + type = entry.type (); >> bfdcore_write (obfd.get (), osec, &type, sizeof (type), bfd_offset); >> >> - switch (entry.type) >> + switch (type) >> { >> case record_full_reg: /* reg */ >> - if (record_debug) >> - gdb_printf (gdb_stdlog, >> - " Writing register %d (1 " >> - "plus %lu plus %d bytes)\n", >> - entry.u.reg.num, >> - (unsigned long) sizeof (regnum), >> - entry.u.reg.len); >> - >> - /* Write regnum. */ >> - regnum = netorder32 (entry.u.reg.num); >> - bfdcore_write (obfd.get (), osec, ®num, sizeof (regnum), bfd_offset); >> - >> - /* Write regval. */ >> - bfdcore_write (obfd.get (), osec, record_full_get_loc (&entry), >> - entry.u.reg.len, bfd_offset); >> - break; >> + { >> + auto ® = entry.reg (); >> + if (record_debug) >> + gdb_printf (gdb_stdlog, >> + " Writing register %d (1 " >> + "plus %lu plus %d bytes)\n", >> + reg.num, >> + (unsigned long) sizeof (regnum), >> + reg.len); >> + >> + /* Write regnum. */ >> + regnum = netorder32 (reg.num); >> + bfdcore_write (obfd.get (), osec, ®num, sizeof (regnum), >> +bfd_offset); >> + >> + /* Write regval. */ >> + bfdcore_write (obfd.get (), osec, entry.get_loc (), reg.len, bfd_offset); >> + break; >> + } >> >> case record_full_mem: /* mem */ >> - if (record_debug) >> - gdb_printf (gdb_stdlog, >> - " Writing memory %s (1 plus " >> - "%lu plus %lu plus %d bytes)\n", >> - paddress (gdbarch, >> - entry.u.mem.addr), >> - (unsigned long) sizeof (addr), >> - (unsigned long) sizeof (len), >> - entry.u.mem.len); >> - >> - /* Write memlen. */ >> - len = netorder32 (entry.u.mem.len); >> - bfdcore_write (obfd.get (), osec, &len, sizeof (len), bfd_offset); >> - >> - /* Write memaddr. */ >> - addr = netorder64 (entry.u.mem.addr); >> - bfdcore_write (obfd.get (), osec, &addr, sizeof (addr), bfd_offset); >> - >> - /* Write memval. */ >> - bfdcore_write (obfd.get (), osec, record_full_get_loc (&entry), >> - entry.u.mem.len, bfd_offset); >> - break; >> + { >> + auto &mem = entry.mem (); >> + if (record_debug) >> + gdb_printf (gdb_stdlog, >> + " Writing memory %s (1 plus " >> + "%lu plus %lu plus %d bytes)\n", >> + paddress (gdbarch, mem.addr), >> + (unsigned long) sizeof (addr), >> + (unsigned long) sizeof (len), >> + mem.len); >> + >> + /* Write memlen. */ >> + len = netorder32 (mem.len); >> + bfdcore_write (obfd.get (), osec, &len, sizeof (len), bfd_offset); >> + >> + /* Write memaddr. */ >> + addr = netorder64 (mem.addr); >> + bfdcore_write (obfd.get (), osec, &addr, sizeof (addr), bfd_offset); >> + >> + /* Write memval. */ >> + bfdcore_write (obfd.get (), osec, entry.get_loc (), mem.len, >> + bfd_offset); >> + break; >> + } >> } >> >> } >> @@ -2473,13 +2513,13 @@ record_full_base_target::save_record (const >> char *recfilename) >> /* Number of effects of an instruction. */ >> save_size += sizeof (uint32_t) + sizeof (uint8_t) + sizeof (uint32_t); >> for (auto &entry : record_full_list[i].effects) >> - switch (entry.type) >> + switch (entry.type ()) >> { >> case record_full_reg: >> - save_size += 1 + 4 + entry.u.reg.len; >> + save_size += 1 + 4 + entry.reg ().len; >> break; >> case record_full_mem: >> - save_size += 1 + 4 + 8 + entry.u.mem.len; >> + save_size += 1 + 4 + 8 + entry.mem ().len; >> break; >> } >> } >> @@ -2612,18 +2652,18 @@ maintenance_print_record_instruction (const >> char *args, int from_tty) >> >> gdbarch *arch = current_inferior ()->arch (); >> >> - for (auto entry : to_print->effects) >> + for (auto &entry : to_print->effects) >> { >> - switch (entry.type) >> + switch (entry.type ()) >> { >> case record_full_reg: >> { >> - type *regtype = gdbarch_register_type (arch, entry.u.reg.num); >> + type *regtype = gdbarch_register_type (arch, entry.reg ().num); >> value *val >> = value_from_contents (regtype, >> - record_full_get_loc (&entry)); >> + entry.get_loc ()); >> gdb_printf ("Register %s changed: ", >> - gdbarch_register_name (arch, entry.u.reg.num)); >> + gdbarch_register_name (arch, entry.reg ().num)); >> struct value_print_options opts; >> get_user_print_options (&opts); >> opts.raw = true; >> @@ -2633,11 +2673,12 @@ maintenance_print_record_instruction (const >> char *args, int from_tty) >> } >> case record_full_mem: >> { >> - gdb_byte *b = record_full_get_loc (&entry); >> + record_full_mem_entry& mem = entry.mem (); >> + gdb_byte *b = entry.get_loc (); >> gdb_printf ("%d bytes of memory at address %s changed from:", >> - entry.u.mem.len, >> - print_core_address (arch, entry.u.mem.addr)); >> - for (int i = 0; i < entry.u.mem.len; i++) >> + mem.len, >> + print_core_address (arch, mem.addr)); >> + for (int i = 0; i < mem.len; i++) >> gdb_printf (" %02x", b[i]); >> gdb_printf ("\n"); >> break; >> -- >> 2.54.0 >> > Christina > Intel Deutschland GmbH > Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany > Tel: +49 89 991 430, www.intel.de > Managing Directors: Harry Demas, Jeffrey Schneiderman, Yin Chong Sorrell > Chairperson of the Supervisory Board: Nicole Lau > Registered Seat: Munich > Commercial Register: Amtsgericht Muenchen HRB 186928 > -- Cheers, Guinevere Larsen It/she