From mboxrd@z Thu Jan 1 00:00:00 1970 Return-Path: Received: from mail-lf1-x135.google.com (mail-lf1-x135.google.com [IPv6:2a00:1450:4864:20::135]) by sourceware.org (Postfix) with ESMTPS id 5CC4F3858D29; Sat, 25 May 2024 09:59:38 +0000 (GMT) DMARC-Filter: OpenDMARC Filter v1.4.2 sourceware.org 5CC4F3858D29 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 5CC4F3858D29 Authentication-Results: server2.sourceware.org; arc=none smtp.remote-ip=2a00:1450:4864:20::135 ARC-Seal: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716631181; cv=none; b=j4F/g4dNqYqZysM+9OTKBx/tuPn6KMxT8VramTbPO3/vHaIpwS1Nwz15VXTnSVBzMO0q991eFocMH4GfkinbCtyr//K03qCjllHArVWLS6LurE3urMESVVy2hyPLORMFYsF6AYRSZRzUpiXeLRjStFqSY/Zb4siHLYlLlAKcxmw= ARC-Message-Signature: i=1; a=rsa-sha256; d=sourceware.org; s=key; t=1716631181; c=relaxed/simple; bh=AFcj37mygwr3PRgMDPa7+KGEMIVg0Y0+DG7q/+2HtiY=; h=DKIM-Signature:Message-ID:Date:MIME-Version:Subject:To:From; b=SBh5oBfLTh2oAHIUFMyznORlzay44AQGeO13MzQA3cgZN7G3UVZZ3noUALn5uVlXLpomt9lzR6PGYHglaE1veW+BHcXdsbuJ1KS2jANfNPXhIfy8UNf17vGIN36In9X4LVI2lQ3gZ/iUOtRNsVVQXL80UTaaOvq/luiO5veRSxI= ARC-Authentication-Results: i=1; server2.sourceware.org Received: by mail-lf1-x135.google.com with SMTP id 2adb3069b0e04-5295a576702so1782121e87.0; Sat, 25 May 2024 02:59:38 -0700 (PDT) DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=gmail.com; s=20230601; t=1716631176; x=1717235976; darn=gcc.gnu.org; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :from:to:cc:subject:date:message-id:reply-to; bh=HSFtpMzWLbSUJ6eSsJaQbl8rKSH+RtGTIcjgX5srUdc=; b=ho3HAglkWWUueg8go1dpfYxAyVOgGDkTLTfj+jKoiNaUexRwtot3vY/cXimMW35fyG eu0mr9KH8YtU9CLc1ztf9F9rhCjPPwjNiKHUA7j2puP72O+kiaFXfIyf/Y/zni9y/IQ7 rsvb2vr6IbcRnPxFns2p/xliZO+aHwgDqCH4QwaSCnOFZJ/hAz5Z4pUIFBmcGEYPriST olJjFVedY+SpiA6wMaCryszj6L1a1pHtiX6k0wPG4LpEnDMlry87xVJ4I5Q59CbMqODI pcd7RnVQmKeUNDPCdHW0xljq2+mqqFOVIpeBbyHhTV5Rz9HLwDuGHZCxXXgeBQp1p7VU xHjw== X-Google-DKIM-Signature: v=1; a=rsa-sha256; c=relaxed/relaxed; d=1e100.net; s=20230601; t=1716631176; x=1717235976; h=content-transfer-encoding:in-reply-to:from:content-language :references:cc:to:subject:user-agent:mime-version:date:message-id :x-gm-message-state:from:to:cc:subject:date:message-id:reply-to; bh=HSFtpMzWLbSUJ6eSsJaQbl8rKSH+RtGTIcjgX5srUdc=; b=BQO0gjL2/QtkBWcRFg3VyS/6+IXpQnDl04vZWN29SeIC8EVwZBFD3iErAhTsuahuEP WHeWDJs/GNdk6xza5NGDc6SxhU+MWT14yurAMwqtf3x9qwIJkCQ5RoEXtDXme5FGXVWq w02fyQqQEGMCksAMu+HSK+PaI4yMnd+a+gEz6Yw3WrrvYvJo/2PabRj9s3slaVfKJWFX 3j/3c7fS9dAzG/HHPA59aD9rRXf53YTVbEJmgWdVOVEMxqdVxsiV8MytY851jiTugHgX 45besJQpxp8hKbAxQB/in+hvrAmG+5OlobJNfGGTXWI841NOPKcfGbAHULS6Od1n1ifN 6I+g== X-Forwarded-Encrypted: i=1; AJvYcCUL3Q9MxkNt5b8oSBvQktkU09u7O0sFXaU+joOjlWdQ2OCsMvJ6+3jqWtSKjXlstzglCwEwEMf89Mngu1pyJ5bvf35kuq2/0g== X-Gm-Message-State: AOJu0Ywd5/4iHqGMxFND7sfksuBr1bEQz2J9pn4lIjfRV6Ld68h48Gwj a4nViJURh4q7meMAChwn/I9ii/vvRRblC47vs8qfDK1eqlQ2lZYC X-Google-Smtp-Source: AGHT+IF3Dgd/o6mitaVDV2cDhQcXrESh/4zCqIreollEpkWMAVAAbhL3KtuhZ+OW2dQOdo6JMYWIvw== X-Received: by 2002:ac2:58e4:0:b0:528:ce56:96d with SMTP id 2adb3069b0e04-529666d710amr2245387e87.50.1716631176068; Sat, 25 May 2024 02:59:36 -0700 (PDT) Received: from ?IPV6:2a01:e0a:1dc:b1c0:7716:f7d0:2d6f:3241? ([2a01:e0a:1dc:b1c0:7716:f7d0:2d6f:3241]) by smtp.gmail.com with ESMTPSA id 5b1f17b1804b1-42100fadd72sm77238415e9.31.2024.05.25.02.59.35 (version=TLS1_3 cipher=TLS_AES_128_GCM_SHA256 bits=128/128); Sat, 25 May 2024 02:59:35 -0700 (PDT) Message-ID: Date: Sat, 25 May 2024 11:59:34 +0200 MIME-Version: 1.0 User-Agent: Mozilla Thunderbird Subject: Re: [PATCH] Avoid vector -Wfree-nonheap-object warnings To: Jonathan Wakely Cc: libstdc++ , gcc-patches References: <305e3002-4ebb-4f07-91f3-10a29ec7fc70@gmail.com> Content-Language: en-US From: =?UTF-8?Q?Fran=C3=A7ois_Dumont?= In-Reply-To: Content-Type: text/plain; charset=UTF-8; format=flowed Content-Transfer-Encoding: 8bit X-Spam-Status: No, score=-11.2 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 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 24/05/2024 16:17, Jonathan Wakely wrote: > On Thu, 23 May 2024 at 18:38, François Dumont wrote: >> >> On 23/05/2024 15:31, Jonathan Wakely wrote: >>> On 23/05/24 06:55 +0200, François 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 already >>> uses the RAII guard :-( >>> https://gcc.gnu.org/bugzilla/show_bug.cgi?id=115016 >> 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-object >> warn. But _M_realloc_append is already doing potentially throwing >> operations before assigning this->_M_impl so it must be something else. >> >> 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 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. >> (_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 = _M_storage; >>>> + _M_storage = 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 = 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 __last) >>>> { >>>> - pointer __result = 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 = _M_allocate(_S_check_init_len( >>>> - static_cast(__n), _M_get_Tp_allocator())); >>>> - this->_M_impl._M_end_of_storage = >>>> - 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 = 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 need a >>> local __start later anyway. So maybe like this: >>> >>> template >>> void >>> _M_initialize_dispatch(_Integer __ni, _Integer __value, __true_type) >>> { >>> const size_type __n = static_cast(__ni); >>> pointer __start = _M_allocate(_S_check_init_len(__n), >>> _M_get_Tp_allocator()); >>> _Guard __guard(__start, __n, *this); >>> this->_M_impl._M_start = __start; >>> _M_fill_initialize(__n, __value); >>> this->_M_impl._M_end_of_storage = __start + __n; >>> (void) __guard._M_release(); >>> } >>> >>> Or inline the __uninitialized_fill_n_a call if you want to (but then >>> 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. >> Ok to commit ? > OK for trunk, thanks! > There are test failures in C++98, working on it.