From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lj1-x234.google.com (mail-lj1-x234.google.com [IPv6:2a00:1450:4864:20::234]) by sourceware.org (Postfix) with ESMTPS id 4042B3858432; Wed, 29 May 2024 09:35:51 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 4042B3858432 Authentication-Results: sourceware.org; dmarc=pass (p=none dis=none) header.from=gmail.com Authentication-Results: sourceware.org; spf=pass smtp.mailfrom=gmail.com ARC-Filter: OpenARC Filter v1.0.0 sourceware.org 4042B3858432 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::234 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716975353; cv=none; b=PBSJIwSXqkbWML5w2eLIar5ywTy/0zYVh5Ya2ORSs9dF4dprY6/y1lxbFNKpCgIAYAyFS+Hak+qb0INWYQinG6J19oH+ataqUOP3KkqqRBmwwVpf8IJwq//oCH1Dk5f5F216nYcpF8t1YCwIqGA8idEFmfKsCj6FlP4XAzrcAfI= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716975353; c=relaxed/simple; bh=EZVqOCzg54kBMslbUKd8iVLMpEcUQmjiemrZ66wKC+w=; h=DKIM-Signature:MIME-Version:From:Date:Message-ID:Subject:To; b=lE+ECtjfb21o/GCaXL0s/T4fjL/qYCxB/KlRmoGmzHup2hL9vh8b2kmKzXrDtULSVh1XUDU3l4AxpXh3RE7jIZZaHyA7KIeHaTB2uuUOl1EHPpwB8OSXVBaTYXhiMg6CIlb5A1HkB/wpbaHmwXAxAFAmHpE/nvF5851RglkZpo0= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lj1-x234.google.com with SMTP id 38308e7fff4ca-2e6f51f9de4so25489721fa.3; Wed, 29 May 2024 02:35:51 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1716975349; x=1717580149; darn=gcc.gnu.org; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:from:to:cc:subject:date :message-id:reply-to; bh=1Oa8jx7U7LU8xt3PTRiGxED4/Y6hjat3/K+15gaxIUs=; b=ANF6zcxedfFWfpFKiomIP9rq+imSKzKpjosz9mgP9VhmUpHaOcV7VDQHsFeb6KimOn TkfiDVM3jv8P6UH+n02NsdOKeOi4ifIBePrrd1JbxFZZ0PYX4rg1KuF1pxKGKqMZ4DO8 L3fqQo3bA9ipVNu1LMunHCaX+hAUtr9/OkFeArPe1C+e1iyQNFVmsGvaenXGLC5fA2iH mLpI/Cj76H6yAXwTWMjz/MhEXLyrD4mMupqRTzCQchMW06GJh2RW2MacYn4mnNw9FrvR aZmjN5jJHJyDGuU6wgR6+nFn9FqQi+0ogeve/QTL3U3eLXNd1htK1y7JFKBaNBkHSkXH XGUA== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716975349; x=1717580149; h=content-transfer-encoding:cc:to:subject:message-id:date:from :in-reply-to:references:mime-version:x-gm-message-state:from:to:cc :subject:date:message-id:reply-to; bh=1Oa8jx7U7LU8xt3PTRiGxED4/Y6hjat3/K+15gaxIUs=; b=tDG7cF5oqdpN4B3/vpzbjJthVeMhMfnZbpjeYw1Qe0roIF/i67U3NcZyj9jplqVMrj ysoWfFMgxVdsviA7ggQIRo7wGVq4hDXrW05TFk6OXGLm4+aE6fgGbmx0s7RUrrg554dD qH4gr2Ysy9ov27226LcP4+uPO1yjn29QBy5I+H8IRa4SCGmv1x91djGI5gKyl4co/Ve6 0MAY5mVMCP5QOF9DAA/F1X5hbL+rBCrXy8r4ZX+9o675VdnyNZazyBAnMA01m96MGY47 3EWcjwifR2TjKEIBtO5PjfhKYo5t4jsKtIvbH/oMZthO1fjgXkfY/4FpIo0F65hwczKA D2Ag== X-Forwarded-Encrypted: i=1; AJvYcCXWhJZHXivkQby3Bo5+B51cdNJYWgBdnmLsLtWJ55O8rpAgn04mNhLYVrBP/z4EtpIWnDfFMmTulSRq4JBNRm57OHPQZx3+d/MyEublMywO3BzG2hBM+6h0v0EMM2Re X-Gm-Message-State: AOJu0YygdxTpVeWtxql/rXLImIyPMCBtQeW/D5ljM+MbBNcvZiMl1NUS O2+GxnTN1yye9Nn7T7/UR4tk6cQwholKGIlXJwJLlrtMGK4z5jj4pRoKmkoXgN9P+hWaANEKIP4 wK4VBSTK+PPtXOp7UKSMVLG9ZUy7nEA== X-Google-Smtp-Source: AGHT+IH9+7cevLeVZCMykrKIl9afI2vhOsABLmgbA5atLEo36J/ZO6PTGxFcGiy3R3PWPpTWbGhVG16erFrQLaVJfHU= X-Received: by 2002:a2e:3514:0:b0:2ea:7e39:bd38 with SMTP id 38308e7fff4ca-2ea7e39c68dmr5855271fa.34.1716975349226; Wed, 29 May 2024 02:35:49 -0700 (PDT) MIME-Version: 1.0 References: <305e3002-4ebb-4f07-91f3-10a29ec7fc70@gmail.com> <610e3afa-8a8a-400d-882e-41ec186d67f6@gmail.com> <0634b907-5c27-4b8d-a47c-a899e637acac@gmail.com> In-Reply-To: <0634b907-5c27-4b8d-a47c-a899e637acac@gmail.com> From: Jonathan Wakely Date: Wed, 29 May 2024 10:35:38 +0100 Message-ID: Subject: Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings To: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= Cc: Jonathan Wakely , "libstdc++" , gcc-patches Content-Type: text/plain; charset="UTF-8" Content-Transfer-Encoding: quoted-printable X-Spam-Status: No, score=-6.4 required=5.0 tests=BAYES_00,DKIM_SIGNED,DKIM_VALID,DKIM_VALID_AU,DKIM_VALID_EF,FREEMAIL_FROM,GIT_PATCH_0,KAM_SHORT,RCVD_IN_DNSWL_NONE,SPF_HELO_NONE,SPF_PASS,TXREP,T_SCC_BODY_TEXT_LINE autolearn=ham autolearn_force=no version=3.4.6 X-Spam-Checker-Version: SpamAssassin 3.4.6 (2021-04-09) on server2.sourceware.org List-Id: On Tue, 28 May 2024 at 21:55, Fran=C3=A7ois Dumont w= rote: > > I can indeed restore _M_initialize_dispatch as it was before. It was not > fixing my initial problem. I simply kept the code simplification. > > libstdc++: Use RAII to replace try/catch blocks > > Move _Guard into std::vector declaration and use it to guard all > calls to > vector _M_allocate. > > Doing so the compiler has more visibility on what is done with the > pointers > and do not raise anymore the -Wfree-nonheap-object warning. > > libstdc++-v3/ChangeLog: > > * include/bits/vector.tcc (_Guard): Move all the nested > duplicated class... > * include/bits/stl_vector.h (_Guard_alloc): ...here and rena= me. > (_M_allocate_and_copy): Use latter. > (_M_initialize_dispatch): Small code simplification. > (_M_range_initialize): Likewise and set _M_finish first > from the result > of __uninitialize_fill_n_a that can throw. > > Tested under Linux x86_64. > > Ok to commit ? OK, thanks > > Fran=C3=A7ois > > On 28/05/2024 12:30, Jonathan Wakely wrote: > > On Mon, 27 May 2024 at 05:37, Fran=C3=A7ois Dumont wrote: > >> Here is a new version working also in C++98. > > Can we use a different solution that doesn't involve an explicit > > template argument list for that __uninitialized_fill_n_a call? > > > > -+ this->_M_impl._M_finish =3D std::__uninitialized_fill_n_a > > ++ this->_M_impl._M_finish =3D > > ++ std::__uninitialized_fill_n_a > > + (__start, __n, __value, _M_get_Tp_allocator()); > > > > Using _M_fill_initialize solves the problem :-) > > > > > > > >> Note that I have this failure: > >> > >> FAIL: 23_containers/vector/types/1.cc -std=3Dgnu++98 (test for excess= errors) > >> > >> but it's already failing on master, my patch do not change anything. > > Yes, that's been failing for ages. > > > >> Tested under Linux x64, > >> > >> still ok to commit ? > >> > >> Fran=C3=A7ois > >> > >> On 24/05/2024 16:17, Jonathan Wakely wrote: > >>> On Thu, 23 May 2024 at 18:38, Fran=C3=A7ois Dumont wrote: > >>>> On 23/05/2024 15:31, Jonathan Wakely wrote: > >>>>> On 23/05/24 06:55 +0200, Fran=C3=A7ois Dumont wrote: > >>>>>> As explained in this email: > >>>>>> > >>>>>> https://gcc.gnu.org/pipermail/libstdc++/2024-April/058552.html > >>>>>> > >>>>>> I experimented -Wfree-nonheap-object because of my enhancements on > >>>>>> algos. > >>>>>> > >>>>>> So here is a patch to extend the usage of the _Guard type to other > >>>>>> parts of vector. > >>>>> Nice, that fixes the warning you were seeing? > >>>> Yes ! I indeed forgot to say so :-) > >>>> > >>>> > >>>>> We recently got a bug report about -Wfree-nonheap-object in > >>>>> std::vector, but that is coming from _M_realloc_append which alread= y > >>>>> uses the RAII guard :-( > >>>>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=3D115016 > >>>> Note that I also had to move call to __uninitialized_copy_a before > >>>> assigning this->_M_impl._M_start so get rid of the -Wfree-nonheap-ob= ject > >>>> warn. But _M_realloc_append is already doing potentially throwing > >>>> operations before assigning this->_M_impl so it must be something el= se. > >>>> > >>>> Though it made me notice another occurence of _Guard in this method.= Now > >>>> replaced too in this new patch. > >>>> > >>>> libstdc++: Use RAII to replace try/catch blocks > >>>> > >>>> Move _Guard into std::vector declaration and use it to guard = all > >>>> calls to > >>>> vector _M_allocate. > >>>> > >>>> Doing so the compiler has more visibility on what is done wit= h the > >>>> pointers > >>>> and do not raise anymore the -Wfree-nonheap-object warning. > >>>> > >>>> libstdc++-v3/ChangeLog: > >>>> > >>>> * include/bits/vector.tcc (_Guard): Move all the nest= ed > >>>> duplicated class... > >>>> * include/bits/stl_vector.h (_Guard_alloc): ...here. > >>>> (_M_allocate_and_copy): Use latter. > >>>> (_M_initialize_dispatch): Likewise and set _M_finish = first > >>>> from the result > >>>> of __uninitialize_fill_n_a that can throw. > >>>> (_M_range_initialize): Likewise. > >>>> > >>>>>> diff --git a/libstdc++-v3/include/bits/stl_vector.h > >>>>>> b/libstdc++-v3/include/bits/stl_vector.h > >>>>>> index 31169711a48..4ea74e3339a 100644 > >>>>>> --- a/libstdc++-v3/include/bits/stl_vector.h > >>>>>> +++ b/libstdc++-v3/include/bits/stl_vector.h > >>>>>> @@ -1607,6 +1607,39 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > >>>>>> clear() _GLIBCXX_NOEXCEPT > >>>>>> { _M_erase_at_end(this->_M_impl._M_start); } > >>>>>> > >>>>>> + private: > >>>>>> + // RAII guard for allocated storage. > >>>>>> + struct _Guard > >>>>> If it's being defined at class scope instead of locally in a member > >>>>> function, I think a better name would be good. Maybe _Ptr_guard or > >>>>> _Dealloc_guard or something. > >>>> _Guard_alloc chosen. > >>>>>> + { > >>>>>> + pointer _M_storage; // Storage to deallocate > >>>>>> + size_type _M_len; > >>>>>> + _Base& _M_vect; > >>>>>> + > >>>>>> + _GLIBCXX20_CONSTEXPR > >>>>>> + _Guard(pointer __s, size_type __l, _Base& __vect) > >>>>>> + : _M_storage(__s), _M_len(__l), _M_vect(__vect) > >>>>>> + { } > >>>>>> + > >>>>>> + _GLIBCXX20_CONSTEXPR > >>>>>> + ~_Guard() > >>>>>> + { > >>>>>> + if (_M_storage) > >>>>>> + _M_vect._M_deallocate(_M_storage, _M_len); > >>>>>> + } > >>>>>> + > >>>>>> + _GLIBCXX20_CONSTEXPR > >>>>>> + pointer > >>>>>> + _M_release() > >>>>>> + { > >>>>>> + pointer __res =3D _M_storage; > >>>>>> + _M_storage =3D 0; > >>>>> I don't think the NullablePointer requirements include assigning 0, > >>>>> only from nullptr, which isn't valid in C++98. > >>>>> > >>>>> https://en.cppreference.com/w/cpp/named_req/NullablePointer > >>>>> > >>>>> Please use _M_storage =3D pointer() instead. > >>>> I forgot about user fancy pointer, fixed. > >>>> > >>>> > >>>>>> + return __res; > >>>>>> + } > >>>>>> + > >>>>>> + private: > >>>>>> + _Guard(const _Guard&); > >>>>>> + }; > >>>>>> + > >>>>>> protected: > >>>>>> /** > >>>>>> * Memory expansion handler. Uses the member allocation > >>>>>> function to > >>>>>> @@ -1618,18 +1651,10 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > >>>>>> _M_allocate_and_copy(size_type __n, > >>>>>> _ForwardIterator __first, _ForwardIterator __la= st) > >>>>>> { > >>>>>> - pointer __result =3D this->_M_allocate(__n); > >>>>>> - __try > >>>>>> - { > >>>>>> - std::__uninitialized_copy_a(__first, __last, __result, > >>>>>> - _M_get_Tp_allocator()); > >>>>>> - return __result; > >>>>>> - } > >>>>>> - __catch(...) > >>>>>> - { > >>>>>> - _M_deallocate(__result, __n); > >>>>>> - __throw_exception_again; > >>>>>> - } > >>>>>> + _Guard __guard(this->_M_allocate(__n), __n, *this); > >>>>>> + std::__uninitialized_copy_a > >>>>>> + (__first, __last, __guard._M_storage, _M_get_Tp_allocator= ()); > >>>>>> + return __guard._M_release(); > >>>>>> } > >>>>>> > >>>>>> > >>>>>> @@ -1642,13 +1667,15 @@ _GLIBCXX_BEGIN_NAMESPACE_CONTAINER > >>>>>> // 438. Ambiguity in the "do the right thing" clause > >>>>>> template > >>>>>> void > >>>>>> - _M_initialize_dispatch(_Integer __n, _Integer __value, __true= _type) > >>>>>> + _M_initialize_dispatch(_Integer __int_n, _Integer __value, > >>>>>> __true_type) > >>>>>> { > >>>>>> - this->_M_impl._M_start =3D _M_allocate(_S_check_init_len( > >>>>>> - static_cast(__n), _M_get_Tp_allocator())); > >>>>>> - this->_M_impl._M_end_of_storage =3D > >>>>>> - this->_M_impl._M_start + static_cast(__n); > >>>>>> - _M_fill_initialize(static_cast(__n), __value); > >>>>> Please fix the comment on _M_fill_initialize if you're removing the > >>>>> use of it here. > >>>> Already done in this initial patch proposal, see below. > >>>> > >>>>>> + const size_type __n =3D static_cast(__int_n); > >>>>>> + _Guard __guard(_M_allocate(_S_check_init_len( > >>>>>> + __n, _M_get_Tp_allocator())), __n, *this); > >>>>> I think this would be easier to read if the _S_check_init_len call = was > >>>>> done first, and maybe the allocation too, since we are going to nee= d a > >>>>> local __start later anyway. So maybe like this: > >>>>> > >>>>> template > >>>>> void > >>>>> _M_initialize_dispatch(_Integer __ni, _Integer __value, __tru= e_type) > >>>>> { > >>>>> const size_type __n =3D static_cast(__ni); > >>>>> pointer __start =3D _M_allocate(_S_check_init_len(__n), > >>>>> _M_get_Tp_allocator()); > >>>>> _Guard __guard(__start, __n, *this); > >>>>> this->_M_impl._M_start =3D __start; > >>>>> _M_fill_initialize(__n, __value); > >>>>> this->_M_impl._M_end_of_storage =3D __start + __n; > >>>>> (void) __guard._M_release(); > >>>>> } > >>>>> > >>>>> Or inline the __uninitialized_fill_n_a call if you want to (but the= n > >>>>> fix the comment on _M_fill_initialize). Inlining it does make this > >>>>> function more consistent with the next one, which calls > >>>>> __uninitialized_copy_a directly. > >>>> Yes, this is why I called __uninitialized_fill_n_a instead and also = to > >>>> do so *before* assigning _M_impl._M_start. > >>>> > >>>> > >>>>>> - // Called by the first initialize_dispatch above and by the > >>>>>> - // vector(n,value,a) constructor. > >>>>>> + // Called by the vector(n,value,a) constructor. > >>>> See, it's here :-) > >>> Doh! Sorry, I'm not sure how I missed that. > >>> > >>>> Ok to commit ? > >>> OK for trunk, thanks! > >>>