Skip to content

[libcxx] implement LWG4148: unique_ptr::operator* should not allow dangling references #128213

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

[libcxx] implement LWG4148: unique_ptr::operator* should not allow dangling references #128213

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
2 changes: 1 addition & 1 deletion libcxx/docs/Status/Cxx2cIssues.csv
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -103,7 +103,7 @@
"`LWG4142 <https://wg21.link/LWG4142>`__","``format_parse_context::check_dynamic_spec`` should require at least one type","2024-11 (Wrocław)","","",""
"`LWG4144 <https://wg21.link/LWG4144>`__","Disallow ``unique_ptr<T&, D>``","2024-11 (Wrocław)","","",""
"`LWG4147 <https://wg21.link/LWG4147>`__","Precondition on ``inplace_vector::emplace``","2024-11 (Wrocław)","","",""
"`LWG4148 <https://wg21.link/LWG4148>`__","``unique_ptr::operator*`` should not allow dangling references","2024-11 (Wrocław)","","",""
"`LWG4148 <https://wg21.link/LWG4148>`__","``unique_ptr::operator*`` should not allow dangling references","2024-11 (Wrocław)","|Complete|","21",""
"`LWG4153 <https://wg21.link/LWG4153>`__","Fix extra ""-1"" for ``philox_engine::max()``","2024-11 (Wrocław)","","",""
"`LWG4154 <https://wg21.link/LWG4154>`__","The Mandates for ``std::packaged_task``'s constructor from a callable entity should consider decaying","2024-11 (Wrocław)","","",""
"`LWG4157 <https://wg21.link/LWG4157>`__","The resolution of LWG3465 was damaged by P2167R3","2024-11 (Wrocław)","","20",""
Expand Down
7 changes: 7 additions & 0 deletions libcxx/include/__memory/unique_ptr.h
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -262,6 +262,13 @@ class _LIBCPP_UNIQUE_PTR_TRIVIAL_ABI _LIBCPP_TEMPLATE_VIS unique_ptr {

_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 __add_lvalue_reference_t<_Tp> operator*() const
_NOEXCEPT_(_NOEXCEPT_(*std::declval<pointer>())) {
// TODO(LLVM-21): Remove this workaround
#if !defined(_LIBCPP_CLANG_VER) || _LIBCPP_CLANG_VER != 1800
// TODO: use reference_converts_from_temporary_v once implemented.
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is a C++23 type trait? Can we use it?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is not implemented yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

But reference_converts_from_temporary_v shouldn't be available before C++23. If you decide to add the static_assert in old modes, it may be more consistent (and less verbose) to solely use the intrinsic.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

In general we tend to add our own private declaration instead of directly using an intrinsic. This private declaration can be used.

Copy link
Contributor

@H-G-Hristov H-G-Hristov Feb 27, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elhewaty FYI made at attempt at implementing the builtin to help with this PR but it won't be usable without workarounds due to Clang bugs and until we update the unsupported platforms Android, Apple, etc.
#128649 which makes it pointless for now.

Copy link
Contributor

@H-G-Hristov H-G-Hristov Mar 8, 2025 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elhewaty #128649 has been merged. The private __reference_converts_from_temporary_v has been removed as per the code review but you can restore it and use it.
a1e77d2#diff-cfa44dc4db9caf101a253c6f6db926764176ac7ff62744a7776949b83c1ad26dL24

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@H-G-Hristov If we remove the private should we use the libcxx public one?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

The removal of the private variable was requested during the review because it wasn't used. If you decide to use it, you can just copy the implementation from my branch (if it is still available) and reintroduce it. Please note that Apple Clang doesn't support the intrinsic yet, which complicates things a bit. Please check the tests about which platforms in the current CI don't have proper support yet.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@elhewaty Please check: a1e77d2#diff-cfa44dc4db9caf101a253c6f6db926764176ac7ff62744a7776949b83c1ad26dL24 You can revert the revert in this commit.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Please not that __has_builtin(__reference_converts_from_temporary) does not return correct value in clang 19.0 and 19.1.0, which also affect Clang 20.0 on Android (Clang on Android doesn't match official Clang releases) in NDK r29. Also AppleClang 17 (released in XCode 16.3, which also doesn't match to any official Clang release) doesn't support the intrinsic.

 #  if __has_builtin(__reference_converts_from_temporary) ||                                                            \
       (defined(_LIBCPP_CLANG_VER) &&                                                                                   \
        ((!defined(__ANDROID__) && _LIBCPP_CLANG_VER >= 1901) || (defined(__ANDROID__) && _LIBCPP_CLANG_VER >= 2000)))

static_assert(
!__reference_converts_from_temporary(__add_lvalue_reference_t<_Tp>, decltype(*std::declval<pointer>())),
"Reference type _Tp must not convert from a temporary object");
#endif
return *__ptr_;
}
_LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer operator->() const _NOEXCEPT { return __ptr_; }
Expand Down
26 changes: 26 additions & 0 deletions libcxx/test/libcxx/memory/unique_ptr.verify.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
@@ -0,0 +1,26 @@
//===----------------------------------------------------------------------===//
//
// 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: std-at-least-c++11

// <memory>

#include <memory>

struct deleter {
using pointer = long*;
void operator()(pointer) const {}
};

int main(int, char**) {
long l = 0;
std::unique_ptr<const int, deleter> p(&l);
int i =
*p; // expected-error@*:* {{static assertion failed due to requirement '!__reference_converts_from_temporary(const int &, long &)': Reference type _Tp must not convert from a temporary object}}
return 0;
}
Loading