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

[Guinevere Larsen <guinevere@redhat.com>]

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

On 6/15/26 4:49 AM, Schimpe, Christina wrote:
>
> *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 <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
>
> HTML Version:
>
>
> Intel Deutschland GmbH
>
> Registered Address: Dornacher Straße 1, 85622 Feldkirchen, Germany
> Tel: +49 89 991 430, www.intel.de <//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
>

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