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. 

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.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

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