Re: [PATCH v3 3/7] gdb/record: c++ify internal structures of record-full.c

[Guinevere Larsen <guinevere@redhat.com>]

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

On 5/29/26 9:09 AM, Schimpe, Christina wrote:
>>>> +    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.
> Yeah, I just noticed it since I've been looking at a sample implementation
> for the move assignment operator which does check for self-assignment:
>
> https://learn.microsoft.com/en-us/cpp/cpp/move-constructors-and-move-assignment-operators-cpp?view=msvc-170#example-complete-move-constructor-and-assignment-operator
>
> and then compared a bit with other classes which have DISABLE_COPY_AND_ASSIGN
> and the move assignment operator.
Right, makes sense. Since in this case I would see self-assignment as a 
bug, I added an assert, but if we ever have a different opinion as a 
project, it's easy enough to make it an if condition.
>
>>> 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.
> Arg, this might have been confusing.  My feedback on this is not exactly at the right line.
> I'd add this delete before those lines:
>
>>>> +    addr = other.addr;
>>>> +    len = other.len;
>>>> +    memcpy (u.buf, other.u.buf, sizeof (u.buf));
> When the object is finally destructed, I don't expect a double free, since we only free
> the data of the moved object. In the move operator we delete the data of the original
> object.
>
> In the example link I shared above, also the existing data are deleted.
> But it might be that I'm missing something here still.

Oh right, that makes a lot more sense, I thought the suggestion was to 
free the other.u.ptr lol. Sorry for the misunderstanding!

In the case of this code, I would expect that we're always assigning 
this to the "essentially uninitialized" variable, so again I think an 
assert that len == 0 is a better check. I'll run stuff locally to ensure 
that my understanding is correct, and if it isn't I'll add the free you 
suggested.

-- 
Cheers,
Guinevere Larsen
It/she

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