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

elhewaty · 2025-02-21T18:27:54Z

Fixes: #118362

llvmbot · 2025-02-21T18:28:27Z

@llvm/pr-subscribers-libcxx

Author: None (elhewaty)

Changes

Fixes: #118362

Full diff: https://github.com/llvm/llvm-project/pull/128213.diff

2 Files Affected:

(modified) libcxx/include/__memory/unique_ptr.h (+4-1)
(added) libcxx/test/libcxx/memory/unique_ptr.compile.fail.cpp (+28)

diff --git a/libcxx/include/__memory/unique_ptr.h b/libcxx/include/__memory/unique_ptr.h
index e8ef386f7f9fc..5e8335e971f6d 100644
--- a/libcxx/include/__memory/unique_ptr.h
+++ b/libcxx/include/__memory/unique_ptr.h
@@ -261,7 +261,10 @@ 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>())) {
+      _NOEXCEPT_(_NOEXCEPT_(*std::declval<pointer>()))
+    // TODO: use reference_converts_from_temporary_v once implemented.
+    requires(!__reference_converts_from_temporary(__add_lvalue_reference_t<_Tp>, decltype(*declval<pointer>())))
+  {
     return *__ptr_;
   }
   _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX23 pointer operator->() const _NOEXCEPT { return __ptr_; }
diff --git a/libcxx/test/libcxx/memory/unique_ptr.compile.fail.cpp b/libcxx/test/libcxx/memory/unique_ptr.compile.fail.cpp
new file mode 100644
index 0000000000000..633247da3a5a4
--- /dev/null
+++ b/libcxx/test/libcxx/memory/unique_ptr.compile.fail.cpp
@@ -0,0 +1,28 @@
+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// <memory>
+
+#include <iostream>
+#include <memory>
+
+using namespace std;
+
+struct deleter {
+  using pointer = long*;
+  void operator()(pointer) const {}
+};
+
+int main(int argc, char const *argv[]) {
+  long l = 0;
+  std::unique_ptr<const int, deleter> p(&l);
+  // Error: indirection requires pointer operand ('std::unique_ptr<const int, deleter>' invalid)
+  int i = *p;
+  cout << i << endl;
+  return 0;
+}
\ No newline at end of file

github-actions · 2025-02-21T18:31:08Z

✅ With the latest revision this PR passed the C/C++ code formatter.

mordante

Thanks for working on this!

mordante · 2025-02-21T18:46:47Z

libcxx/include/__memory/unique_ptr.h

+    // TODO: use reference_converts_from_temporary_v once implemented.
+    requires(!__reference_converts_from_temporary(__add_lvalue_reference_t<_Tp>, decltype(*declval<pointer>())))


This is implemented as a constraint. That has several issues:

it's only available in C++20 and newer, this code should work in C++11

this is not always a mandates, a static_assert is the way to do this. Please add a useful message in the static_assert.

libcxx/test/libcxx/memory/unique_ptr.compile.fail.cpp

Zingam · 2025-02-22T11:08:58Z

libcxx/include/__memory/unique_ptr.h

@@ -261,7 +261,10 @@ 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>())) {
+      _NOEXCEPT_(_NOEXCEPT_(*std::declval<pointer>()))
+    // TODO: use reference_converts_from_temporary_v once implemented.


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

It is not implemented yet.

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.

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

@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.

@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

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

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.

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

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)))

elhewaty · 2025-02-22T20:18:18Z

@mordante The crash in the CI is the same as the one I mentioned on discord

AmrDeveloper

Should you add a release note for implementing this paper?

https://github.com/llvm/llvm-project/blob/main/libcxx/docs/ReleaseNotes/21.rst

Zingam · 2025-02-23T04:50:44Z

Should you add a release note for implementing this paper?

https://github.com/llvm/llvm-project/blob/main/libcxx/docs/ReleaseNotes/21.rst

We don't do that for LWG issues.

elhewaty · 2025-02-25T18:58:33Z

@mordante can you take a look?

mordante · 2025-02-25T19:36:29Z

libcxx/test/libcxx/memory/unique_ptr.verify.cpp

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


Suggested change

*p; // expect-error {{static assertion failed due to requirement '!__reference_converts_from_temporary(const int &, long &)': Reference type _Tp must not convert from a temporary object}}

*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}}

@mordante Why it crashes during the build?

@elhewaty Can you apply my suggestion, that should fix some of your CI issues.

@elhewaty are you still interested in pursuing this patch?

@mordante, I am sorry for being late, I had a ton of things to take care of in the past few weeks, I couldn't even open my pc.

I think I am some how confused, was the builtin reference_converts_from_temporary_v fully removed?

@elhewaty Please check my answer above. No it wasn't just the private variable.

…ngling references

elhewaty requested a review from a team as a code owner February 21, 2025 18:27

llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Feb 21, 2025

mordante self-assigned this Feb 21, 2025

mordante requested changes Feb 21, 2025

View reviewed changes

Zingam reviewed Feb 22, 2025

View reviewed changes

elhewaty force-pushed the implement-LWG4148 branch 3 times, most recently from 0553825 to 21bcffd Compare February 22, 2025 18:15

elhewaty requested review from mordante and frederick-vs-ja February 22, 2025 18:20

elhewaty force-pushed the implement-LWG4148 branch from 21bcffd to ebdf97e Compare February 22, 2025 19:42

AmrDeveloper reviewed Feb 22, 2025

View reviewed changes

H-G-Hristov mentioned this pull request Feb 25, 2025

[libc++][type_traits] Implements "A type trait to detect reference binding to temporary" #128649

Merged

2 tasks

mordante reviewed Feb 25, 2025

View reviewed changes

elhewaty force-pushed the implement-LWG4148 branch from ebdf97e to d8f59f3 Compare March 29, 2025 13:11

[libcxx] implement LWG4148: unique_ptr::operator* should not allow da…

b031f46

…ngling references

elhewaty force-pushed the implement-LWG4148 branch from d8f59f3 to b031f46 Compare March 29, 2025 13:14

		// TODO: use reference_converts_from_temporary_v once implemented.
		requires(!__reference_converts_from_temporary(__add_lvalue_reference_t<_Tp>, decltype(*declval<pointer>())))

	*p; // expect-error {{static assertion failed due to requirement '!__reference_converts_from_temporary(const int &, long &)': Reference type _Tp must not convert from a temporary object}}
	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}}

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

Are you sure you want to change the base?

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

Uh oh!

Conversation

elhewaty commented Feb 21, 2025

Uh oh!

llvmbot commented Feb 21, 2025

Uh oh!

github-actions bot commented Feb 21, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

mordante left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

H-G-Hristov Feb 27, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

H-G-Hristov Mar 8, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

elhewaty commented Feb 22, 2025

Uh oh!

AmrDeveloper left a comment

Choose a reason for hiding this comment

Uh oh!

Zingam commented Feb 23, 2025

Uh oh!

elhewaty commented Feb 25, 2025

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Uh oh!

github-actions bot commented Feb 21, 2025 •

edited

Loading

H-G-Hristov Feb 27, 2025 •

edited

Loading

H-G-Hristov Mar 8, 2025 •

edited

Loading