I’ve been out for a couple of days, so I only saw this now. Thanks, I am glad that some of my review was helpful. 😊 Christina From: Guinevere Larsen Sent: Donnerstag, 25. Juni 2026 22:41 To: Schimpe, Christina ; gdb-patches@sourceware.org Subject: Re: [PATCH v4 4/8] gdb/record: c++ify internal structures of record-full.c On 6/15/26 4:49 AM, Schimpe, Christina wrote: From: Guinevere Larsen Sent: Donnerstag, 11. Juni 2026 19:17 To: Schimpe, Christina ; gdb-patches@sourceware.org Cc: Thiago Jung Bauermann 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. Huh, ok. Turns out I was mistaken here in one specific situation. In aarch64 (and possibly other architectures, I would need test it more thoroughly), call instructions end up recording the PC multiple times, which was causing GDB to assert. So I will implement your suggestion for now, and (hopefully) fix the double-recording issues in the near future. Thanks for raising this, turns out my code may have been leaking memory all along, or at least had that chance! -- Cheers, Guinevere Larsen it/its she/her (deprecated) 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 -- 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 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 HTML Version: Intel Deutschland GmbH Registered Address: Dornacher Straße 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 München 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