[libc++] Mark std::unique_ptr::release() as nodiscard #110847
Conversation
It's more often than not a bug to call release() while discarding the value, since the resource would leak. While it's technically possible for the user to purposefully do that, I think [[nodiscard]] in that location will do far more good than harm. Folks can always explicitly discard the result of the expression or use [[maybe_unused]] if the discarding is intended. Fixes llvm#30246
llvmbot
added
the
libc++
|
@llvm/pr-subscribers-libcxx Author: Louis Dionne (ldionne) ChangesIt's more often than not a bug to call release() while discarding the value, since the resource would leak. While it's technically possible for the user to purposefully do that, I think [[nodiscard]] in that location will do far more good than harm. Folks can always explicitly discard the result of the expression or use [[maybe_unused]] if the discarding is intended. Fixes #30246 Full diff: https://github.com/llvm/llvm-project/pull/110847.diff 5 Files Affected:
diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index 11215dc111e36a..38375518551b4d 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/libcxx/include/__memory/unique_ptr.h
@@ -278,7 +278,7 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {
return __ptr_ != nullptr;
}
- _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer release() _NOEXCEPT {
+ [[__nodiscard__]] _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer release() _NOEXCEPT {
pointer __t = __ptr_;
__ptr_ = pointer();
return __t;
diff --git a/libcxx/include/deque b/libcxx/include/deque
index bab0526629f0f8..481837260cbd60 100644
--- a/libcxx/include/deque
+++ b/libcxx/include/deque
@@ -2187,7 +2187,7 @@ void deque<_Tp, _Allocator>::__add_back_capacity() {
typedef __allocator_destructor<_Allocator> _Dp;
unique_ptr<pointer, _Dp> __hold(__alloc_traits::allocate(__a, __block_size), _Dp(__a, __block_size));
__buf.push_back(__hold.get());
- __hold.release();
+ (void)__hold.release();
for (__map_pointer __i = __map_.end(); __i != __map_.begin();)
__buf.push_front(*--__i);
diff --git a/libcxx/include/locale b/libcxx/include/locale
index 573910a85bef54..90265d49ebc328 100644
--- a/libcxx/include/locale
+++ b/libcxx/include/locale
@@ -2459,7 +2459,7 @@ _LIBCPP_HIDE_FROM_ABI void __double_or_nothing(unique_ptr<_Tp, void (*)(void*)>&
if (__t == 0)
__throw_bad_alloc();
if (__owns)
- __b.release();
+ (void)__b.release();
__b = unique_ptr<_Tp, void (*)(void*)>(__t, free);
__new_cap /= sizeof(_Tp);
__n = __b.get() + __n_off;
diff --git a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.nodiscard.verify.cpp b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.nodiscard.verify.cpp
new file mode 100644
index 00000000000000..93131a09f2b64f
--- /dev/null
+++ b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.nodiscard.verify.cpp
@@ -0,0 +1,21 @@
+//===----------------------------------------------------------------------===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+
+// REQUIRES: stdlib=libc++
+
+// <memory>
+//
+// unique_ptr
+//
+// constexpr pointer release() noexcept; // nodiscard as an extension
+
+#include <memory>
+
+void f(std::unique_ptr<int> p) {
+ p.release(); // expected-warning {{ignoring return value of function declared with 'nodiscard' attribute}}
+}
diff --git a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.pass.cpp b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.pass.cpp
index 7aedac99030cba..0bb906a13dd3df 100644
--- a/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.pass.cpp
+++ b/libcxx/test/std/utilities/smartptr/unique.ptr/unique.ptr.class/unique.ptr.modifiers/release.pass.cpp
@@ -7,10 +7,10 @@
//===----------------------------------------------------------------------===//
// <memory>
-
+//
// unique_ptr
-
-// test release
+//
+// constexpr pointer release() noexcept;
#include <memory>
#include <cassert>
|
|
CC @llvm/libcxx-vendors since this may cause new diagnostics to be emitted and I'm not sure on what scale we're talking about. @alexfh Would you be able to take this change for a spin just to see if we get tons of spurious diagnostics? If this causes too many spurious diagnostics, I'd also be OK with closing #30246 as "wontfix". |
|
In (Linux) Chromium we hit 116 instances |
|
For the instances in BoringSSL, the context is that we need to interface with C APIs, and our C APIs often have this kinda annoying pattern: In C++, one would probably write this as taking The best strategy I've come up with for that is: Definitely not great, but works OK and does the right thing in both cases. But I don't mind adding a |
|
Spot-checking another random one, it seems Skia has the same sort of C/C++ boundary to contend with: |
|
Ack. Just Chromium hitting 116 issues of this tells me that this is going to be way too noisy. We're aiming to add I was trying to close a few easy issues and I thought this one would be a good candidate, but looks like this should be closed as NTBF instead. Thanks for the engagement folks. |
For the cases I've looked at, it's the default deleter. Like in @davidben's examples above, we call .release() because the ownership has been handed over to some other object already. We did find at least one bug though: https://chromium-review.googlesource.com/c/chromium/src/+/4289548/13/chrome/browser/ui/views/bookmarks/saved_tab_groups/saved_tab_group_bar.cc#197 So marking it nodiscard certainly has value, and would probably have made perfect sense if it was there from the beginning. As to whether the signal-to-noise ratio is high enough now, I don't have a strong opinion. If we do decide to apply it, I would ask for a macro to make it easier to roll out. |
Interesting. How does the C function call |
|
In the BoringSSL case, it is indeed a custom deleter. I suspect that's a decent heuristic for the C API boundary case, though there's definitely also other scenarios going on like weird "self-deleting" objects. (Which I would argue is actually badly modelled ownership transfer but nonetheless is a thing.) Still the custom deleter heuristic might help a bit. (Like I said, I'm also quite open to finding a better pattern for the BoringSSL stuff.) |
Thanks! I missed that. In the linked Skia code it seems like the default deleter though. |
|
I could try running it on the larger codebase to see how frequently this fires, but we definitely would see all issues from Chromium, Skia and boringssl mentioned above. That would already be a show stopper for us, I'm afraid =\ However, if [[nodiscard]] doesn't affect ABI (which I believe should be the case), this could be an opt-in "hardening". Did you consider this option? |
6 participants
It's more often than not a bug to call release() while discarding the value, since the resource would leak. While it's technically possible for the user to purposefully do that, I think [[nodiscard]] in that location will do far more good than harm.
Folks can always explicitly discard the result of the expression or use [[maybe_unused]] if the discarding is intended.
Fixes #30246