From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from simark.ca by simark.ca with LMTP id Zh2iLkXBGWpKRycAWB0awg (envelope-from ) for ; Fri, 29 May 2026 12:39:33 -0400 Authentication-Results: simark.ca; dkim=pass (1024-bit key; unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=LlKTQdDG; dkim-atps=neutral Received: by simark.ca (Postfix, from userid 112) id AEE9B1E062; Fri, 29 May 2026 12:39:33 -0400 (EDT) X-Spam-Checker-Version: SpamAssassin 4.0.1 (2024-03-25) on simark.ca X-Spam-Level: X-Spam-Status: No, score=-3.4 required=5.0 tests=ARC_SIGNED,ARC_VALID,BAYES_00, DKIMWL_WL_HIGH,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,HTML_MESSAGE, MAILING_LIST_MULTI,RCVD_IN_DNSWL_MED, RCVD_IN_VALIDITY_CERTIFIED_BLOCKED,RCVD_IN_VALIDITY_RPBL_BLOCKED, RCVD_IN_VALIDITY_SAFE_BLOCKED autolearn=ham autolearn_force=no version=4.0.1 Received: from vm01.sourceware.org (vm01.sourceware.org [38.145.34.32]) (using TLSv1.3 with cipher TLS_AES_256_GCM_SHA384 (256/256 bits) key-exchange x25519 server-signature ECDSA (prime256v1) server-digest SHA256) (No client certificate requested) by simark.ca (Postfix) with ESMTPS id E2E6F1E062 for ; Fri, 29 May 2026 12:39:32 -0400 (EDT) Received: from vm01.sourceware.org (localhost [IPv6:::1]) by sourceware.org (Postfix) with ESMTP id 58C664BA23DD for ; Fri, 29 May 2026 16:39:32 +0000 (GMT) DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 58C664BA23DD Authentication-Results: sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=LlKTQdDG Received: from us-smtp-delivery-124.mimecast.com (us-smtp-delivery-124.mimecast.com [170.10.133.124]) by sourceware.org (Postfix) with ESMTP id 8A6B74BA23F1 for ; Fri, 29 May 2026 16:39:02 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 8A6B74BA23F1 Authentication-Results: sourceware.org; dmarc=pass (p=quarantine dis=none) header.from=redhat.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=redhat.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 8A6B74BA23F1 Authentication-Results: sourceware.org; arc=none smtp.remote-ip=170.10.133.124 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1780072742; cv=none; b=JrNsRPDbfHY6CTZwBj3ypWwJp06+gVL3AFCDjiNhORGZOJQ0yyfs4zGD7jEcg7Gbttebsy5OpmLfdWLWu9cdwYeREeKeMcRU8bYlo09N3z96KS3NLdJpIQlQGr4q23na5u3OOPjAOtgLzAMy9neun+kIcQ/xfj/t/ry+qVe90/E= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1780072742; c=relaxed/simple; bh=/6k1aa7jRuvrxSOsGAqEzo8vUt5tfNh1GVqh6rjVUjA=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=VgDoO+6/nNGyyxr2S6yRdEoN7EunG9moaugajGlbi08aKmMwvHz4b1hTg5hHSmwdoANCVPoPBp5n746qczzd/Ky3CSAHs6z0u0QoY08hUBS3tVPnlg4NSsVRMfhlpzMaU1KwU/SAh5MZ9z4LOLcKHZRs+rw+pqAlR1O+jKYgSUM= ARC-Authentication-Results: i=1; sourceware.org; dkim=pass (1024-bit key, unprotected) header.d=redhat.com header.i=@redhat.com header.a=rsa-sha256 header.s=mimecast20190719 header.b=LlKTQdDG DKIM-Filter: OpenDKIM Filter v2.11.0 sourceware.org 8A6B74BA23F1 DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=redhat.com; s=mimecast20190719; t=1780072742; h=from:from:reply-to:subject:subject:date:date:message-id:message-id: to:to:cc:cc:mime-version:mime-version:content-type:content-type: in-reply-to:in-reply-to:references:references; bh=w2C3WGvm8eyYN9SBC7DvVolmyD5rhil5tGDcnjYdaZY=; b=LlKTQdDGqMM6/rvBcg4xlDqBiX59D6Ety3kNPlwn4HEVlG5wZKk5/+BagcIc9p5BHIUGyb +GXgUHsmVzMZQtKQmpp6PyTZgFx5n0lZUnrxRM2Hkb6KQpnfyJyajcj5WvDC3hDJC9bqox tlp7yqFOKPJb4KH8/6rZNG2wIg1aqfs= Received: from mail-qv1-f70.google.com (mail-qv1-f70.google.com [209.85.219.70]) by relay.mimecast.com with ESMTP with STARTTLS (version=TLSv1.3, cipher=TLS_AES_256_GCM_SHA384) id us-mta-524-gG3mdOJgNhKHmVOTyj3O4Q-1; Fri, 29 May 2026 12:39:00 -0400 X-MC-Unique: gG3mdOJgNhKHmVOTyj3O4Q-1 X-Mimecast-MFC-AGG-ID: gG3mdOJgNhKHmVOTyj3O4Q_1780072740 Received: by mail-qv1-f70.google.com with SMTP id 6a1803df08f44-8ccd719a2f2so39167776d6.0 for ; Fri, 29 May 2026 09:39:00 -0700 (PDT) X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20251104; t=1780072740; x=1780677540; h=in-reply-to:from:content-language:references:cc:to:subject :user-agent:mime-version:date:message-id:x-gm-gg:x-gm-message-state :from:to:cc:subject:date:message-id:reply-to; bh=w2C3WGvm8eyYN9SBC7DvVolmyD5rhil5tGDcnjYdaZY=; b=GSVB8Ti85aQZtd9mO/M1xTDNT+veWGwnM1yAwjogLIovDPirWI8ECARJhAo76LV9rb DDt45NoMwZ/q3j/qVWeYEb7qAPiwbS5MP0a5yrzErPMjRc3KqiqIlqG2aDg0GgtL5iSP 8lgM6XKja8RjxG0S+X3v0DYXuwaMA5d2P3MjeJD7nZG6rvhvjXkdawNcNC9rur1Ptpaz UdpDHGVPVgvIiHobl0MgHPStXFec9LPAq1gEoZHIXRcx0tyJTIVXe+sE1lyBoaRoBZWE gpIKiFusJS0W/Um1ZCGOVLeKkdhzuE06gfFp99qvgpXuEmNYzRN+tUEz08cfWDpvcJ2l yL6A== X-Forwarded-Encrypted: i=1; AFNElJ9ACYROTlqkIWj2LLYRRCy8lCfSuzBBKFOgtT2iFyu1fEKxlas4vtFSMsNw6+K3AqlNSWabwNkHPsPnWw==@sourceware.org X-Gm-Message-State: AOJu0Yx94kqGEo0Hhn+p1Qodd5I64w/7R/E9qxXxIfif4Eq6xpbhSnxc ZPEcJLSK1ruVW22fiu8J9KHC3BKLkU8KORnjARqbbNibfpgtTuiJwfK6fQxGtklfyctFh0PbaXn 81HEsZqUCzx4tmC+YXwYSr0eMaCiLkLeSYbnZttXOmszEusak6dnZbdP+ZY64MKE= X-Gm-Gg: Acq92OGeoOPQQcDVFeCpPb+8vY17Psvfx/m/ePegCVkDYPWE1H+YnX2EM2ca9OyUX0r 3QfpUwoXv8H6bgLIk/8I/Jmrl/ovY9XPMUywmMh0iafdf5Cz7Q4oIAma/dVwJuSnTt+LG3Yi3uA 7mCjIaJKiAfbxbkUDLzxB0EEGgy/6oeZtPRwcXH2axUmhjU8BunOC1Ka/HNlHLnKgI5Q6hkeqwi NH0WhXkw2HtP78h2NtqJP44RB3BbRvLaPX6x51Uvf4hJDYMcYZhujN4u7qcgdeNC19kyGBRTz0i DsAr1sEFtsYe/GExHIHys9DDpISXtRKVeYgXHcOPnUC2HBOTvbR1B6zDColyGLPmCmtSLhGI0U8 guJ0Sz4+W05ojydiWb000k/jVMoPWD76cayWIBGl1QRuJqHutTpvNQNp4YNryVzP806KM2E7gBL OAyYo= X-Received: by 2002:a05:620a:224c:10b0:8cf:c272:9721 with SMTP id af79cd13be357-9153d93aab6mr55870285a.6.1780072739913; Fri, 29 May 2026 09:38:59 -0700 (PDT) X-Received: by 2002:a05:620a:224c:10b0:8cf:c272:9721 with SMTP id af79cd13be357-9153d93aab6mr55866585a.6.1780072739470; Fri, 29 May 2026 09:38:59 -0700 (PDT) Received: from ?IPV6:2804:14d:8084:993e:2d5d:7adb:2b6:a50e? ([2804:14d:8084:993e:2d5d:7adb:2b6:a50e]) by smtp.gmail.com with ESMTPSA id af79cd13be357-91532650e85sm232447785a.42.2026.05.29.09.38.56 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Fri, 29 May 2026 09:38:58 -0700 (PDT) Message-ID: <761c5249-d8c3-43c4-b25b-69ddef1f69f6@redhat.com> Date: Fri, 29 May 2026 13:38:54 -0300 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH v3 3/7] gdb/record: c++ify internal structures of record-full.c To: "Schimpe, Christina" , "gdb-patches@sourceware.org" Cc: Thiago Jung Bauermann References: <20260515163706.3355686-1-guinevere@redhat.com> <20260515163706.3355686-4-guinevere@redhat.com> <4ea1a202-e336-44fa-a9e2-2b71a66d9e88@redhat.com> From: Guinevere Larsen In-Reply-To: X-Mimecast-Spam-Score: 0 X-Mimecast-MFC-PROC-ID: -etSqp2YURNgrMu5uwQZbuKzC0Y4rifUUCj-9Ig4uDM_1780072740 X-Mimecast-Originator: redhat.com Content-Type: multipart/alternative; boundary="------------gZPajqDe29F0aeulNQtpvFOT" Content-Language: en-US X-BeenThere: gdb-patches@sourceware.org X-Mailman-Version: 2.1.30 Precedence: list List-Id: Gdb-patches mailing list List-Unsubscribe: , List-Archive: List-Post: List-Help: List-Subscribe: , Errors-To: gdb-patches-bounces~public-inbox=simark.ca@sourceware.org This is a multi-part message in MIME format. --------------gZPajqDe29F0aeulNQtpvFOT Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 7bit 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 --------------gZPajqDe29F0aeulNQtpvFOT Content-Type: text/html; charset=UTF-8 Content-Transfer-Encoding: 7bit
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
--------------gZPajqDe29F0aeulNQtpvFOT--