RE: [PATCH v4 4/8] gdb/record: c++ify internal structures of record-full.c

["Schimpe, Christina" <christina.schimpe@intel.com>]

[-- Attachment #1: Type: text/plain, Size: 2936 bytes --]

From: Guinevere Larsen <guinevere@redhat.com>
Sent: Donnerstag, 11. Juni 2026 19:17
To: Schimpe, Christina <christina.schimpe@intel.com>; gdb-patches@sourceware.org
Cc: Thiago Jung Bauermann <thiago.bauermann@linaro.org>
Subject: Re: [PATCH v4 4/8] gdb/record: c++ify internal structures of record-full.c

On 6/11/26 4:21 AM, Schimpe, Christina wrote:

It seems that you did not change the code as discussed here:

https://sourceware.org/pipermail/gdb-patches/2026-May/227699.html



Would you mind sharing the reason for that?

In my opinion this is necessary.

I blame the weekend. Leave on friday thinking "I'll do it on monday", arrive

on monday thinking "I did it last friday".



I made sure to make the change now. Added the asserts that the length is

0 in the move operator, which means we won't need to worry about freeing

the memory.

But isn't it possible that "len > sizeof (u.buf)" or at least "len > 0" at this point ?
Not really. Since we can't have copying of entries, if an actual entry was stored in the variable that is receiving a move, we lose information on an entry or we forgot to clear the incomplete instruction (which means we never added it to the history). The code is built for this to be the case, so I am asserting that the length is 0 to catch these mistakes and not let execution information be lost.

In any case, I believe a good comment explaining this might be helpful, since
freeing the original memory is something one would expect at this specific code
location. The same applies for the other assert you already added:
“gdb_assert (this != &other);”.

Besides that, I only found one further nit:
the line “ record_full_reg_entry &operator=(record_full_reg_entry &&other)” misses
a space before the bracket “(“.

I believe I am now done with the review of this patch, but like patch #1, please
take my review with a grain of salt, too. ;)

Reviewed-By: Christina Schimpe <christina.schimpe@intel.com>

Christina

--

Cheers,

Guinevere Larsen

it/its

she/her (deprecated)





So before this code:



+    addr = other.addr;

+    len = other.len;

+    memcpy (u.buf, other.u.buf, sizeof (u.buf));

Christina

Intel Deutschland GmbH



Registered Address: Dornacher Strasse 1, 85622 Feldkirchen, Germany

Tel: +49 89 991 430, www.intel.de<http://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
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

[-- Attachment #2: Type: text/html, Size: 8347 bytes --]