[libc++] Add [[clang::lifetimebound]] attribute to std::forward and friends

[philnik]

Summary

This allows clang to catch lifetime bugs through these functions.
As a drive-by, replace _LIBCPP_INLINE_VISIBILITY with _LIBCPP_HIDE_FROM_ABI.
Fixes #59900

[ldionne]

Thanks! This is a nice improvement IMO.

However, the CI issue you're seeing is actually a very interesting and slightly deeper issue. Indeed, remember when Clang decided to define forward and move as builtins in D123345? One impact of that change is that we don't control our std::forward and std::move anymore, so we'd need to make the change in this patch inside Clang, not inside libc++. An obvious follow-up question is then do we make the change in Clang and strive to keep libc++ in synchronization with how Clang defines std::forward? Or do we not care about keeping them in sync? Should we have a way to ensure that both definitions stay in sync?

CC Clang folks @rnk @rsmith @aaron.ballman

[philnik]

In D141321#4040624, @ldionne wrote:

Thanks! This is a nice improvement IMO.

However, the CI issue you're seeing is actually a very interesting and slightly deeper issue. Indeed, remember when Clang decided to define forward and move as builtins in D123345? One impact of that change is that we don't control our std::forward and std::move anymore, so we'd need to make the change in this patch inside Clang, not inside libc++. An obvious follow-up question is then do we make the change in Clang and strive to keep libc++ in synchronization with how Clang defines std::forward? Or do we not care about keeping them in sync? Should we have a way to ensure that both definitions stay in sync?

CC Clang folks @rnk @rsmith @aaron.ballman

I'm not sure how clang handles this, but the warnings get emitted. It's just that I forgot to guard std::forward_like pre-C++23. I guess it just adds the attributes to the builtin declaration magically.

[philnik]

Try to fix CI

[Mordante]

I like this idea. I think it would be useful for "views" like string_view and span too.

libcxx/include/__config

1059

Why not [[clang::lifetimebound]] ?

libcxx/include/__utility/forward_like.h

37–38

Should this extension be documented, like we do with [[nodiscard]]?

[philnik]

libcxx/include/__config

1059

For the same reason that we _Uglify all other names: macros. Attributes are not immune to them, so we have to _Uglify them too. https://godbolt.org/z/MqW68b9PY

libcxx/include/__utility/forward_like.h

37–38

I'm not convinced we should list the [[nodiscard]] decorated functions. As we extend the use of these kinds of macros the lists become long and I don't see why anybody who is actually interested in that list. Users are interested in getting warnings from the compiler, not which functions have some specific attribute. We also don't document which symbols are marked [[clang::preferred_name]].

[aaron.ballman]

In D141321#4040624, @ldionne wrote:

Thanks! This is a nice improvement IMO.

However, the CI issue you're seeing is actually a very interesting and slightly deeper issue. Indeed, remember when Clang decided to define forward and move as builtins in D123345? One impact of that change is that we don't control our std::forward and std::move anymore, so we'd need to make the change in this patch inside Clang, not inside libc++. An obvious follow-up question is then do we make the change in Clang and strive to keep libc++ in synchronization with how Clang defines std::forward? Or do we not care about keeping them in sync? Should we have a way to ensure that both definitions stay in sync?

CC Clang folks @rnk @rsmith @aaron.ballman

I think trying to stay in sync is probably the right approach, but I don't have good ideas about how to automatically catch differences. However, I don't expect to make a lot of changes to these signatures, so maybe we don't need something to catch differences. I suspect the correct way to do this in Clang is to modify Builtins.def to add a new encoding for lifetime bound to the list of attributes, and then update ASTContext::GetBuiltinType()/Sema::ActOnFunctionDeclarator() to properly add the attribute where needed.

[ldionne]

In D141321#4048177, @aaron.ballman wrote:

I think trying to stay in sync is probably the right approach, but I don't have good ideas about how to automatically catch differences. However, I don't expect to make a lot of changes to these signatures, so maybe we don't need something to catch differences. I suspect the correct way to do this in Clang is to modify Builtins.def to add a new encoding for lifetime bound to the list of attributes, and then update ASTContext::GetBuiltinType()/Sema::ActOnFunctionDeclarator() to properly add the attribute where needed.

Actually, I think we do have a way to make sure that they stay in sync, at least for the aspects of it that we care about. We have our tests, and they will fail if somehow Clang doesn't do the right thing! It seems to do the right thing right now, since the tests are passing. IDK how that works, but oh well.

libcxx/include/__utility/forward_like.h

37–38

Yeah, I'd agree with dropping the list of [[nodiscard]] extensions in the docs, but please make sure they are all marked with _LIBCPP_NODISCARD_EXT so we can find them. Having the actual list in the documentation has little value and it will only get out of sync.

That should be done in a separate patch, obv.

libcxx/include/__config

 #  if __has_cpp_attribute(fallthrough)
 #    define _LIBCPP_FALLTHROUGH() [[fallthrough]]
 #  elif __has_attribute(__fallthrough__)
 #    define _LIBCPP_FALLTHROUGH() __attribute__((__fallthrough__))
 #  else
 #    define _LIBCPP_FALLTHROUGH() ((void)0)
 #  endif
 
+#  if __has_cpp_attribute(_Clang::__lifetimebound__)
+#    define _LIBCPP_LIFETIMEBOUND [[_Clang::__lifetimebound__]]
+#  else
+#    define _LIBCPP_LIFETIMEBOUND
+#  endif
+
 #  if __has_attribute(__nodebug__)
 #    define _LIBCPP_NODEBUG __attribute__((__nodebug__))
 #  else
 #    define _LIBCPP_NODEBUG
 #  endif
 
 #  if __has_attribute(__standalone_debug__)
 #    define _LIBCPP_STANDALONE_DEBUG __attribute__((__standalone_debug__))

libcxx/include/__utility/forward.h

 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp>
-_LIBCPP_NODISCARD_EXT inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR _Tp&&
-forward(__libcpp_remove_reference_t<_Tp>& __t) _NOEXCEPT {
+_LIBCPP_NODISCARD_EXT inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _Tp&&
+forward(_LIBCPP_LIFETIMEBOUND __libcpp_remove_reference_t<_Tp>& __t) _NOEXCEPT {
   return static_cast<_Tp&&>(__t);
 }
 
 template <class _Tp>
-_LIBCPP_NODISCARD_EXT inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR _Tp&&
-forward(__libcpp_remove_reference_t<_Tp>&& __t) _NOEXCEPT {
+_LIBCPP_NODISCARD_EXT inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR _Tp&&
+forward(_LIBCPP_LIFETIMEBOUND __libcpp_remove_reference_t<_Tp>&& __t) _NOEXCEPT {
   static_assert(!is_lvalue_reference<_Tp>::value, "cannot forward an rvalue as an lvalue");
   return static_cast<_Tp&&>(__t);
 }
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___UTILITY_FORWARD_H

libcxx/include/__utility/forward_like.h

 
 template <class _Ap, class _Bp>
 using _OverrideRef = _If<is_rvalue_reference_v<_Ap>, remove_reference_t<_Bp>&&, _Bp&>;
 
 template <class _Ap, class _Bp>
 using _ForwardLike = _OverrideRef<_Ap&&, _CopyConst<remove_reference_t<_Ap>, remove_reference_t<_Bp>>>;
 
 template <class _Tp, class _Up>
-[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto forward_like(_Up&& __ux) noexcept -> _ForwardLike<_Tp, _Up> {
+[[nodiscard]] _LIBCPP_HIDE_FROM_ABI constexpr auto forward_like(_LIBCPP_LIFETIMEBOUND _Up&& __ux) noexcept
+    -> _ForwardLike<_Tp, _Up> {
   return static_cast<_ForwardLike<_Tp, _Up>>(__ux);
 }
 
 #endif // _LIBCPP_STD_VER > 20
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___UTILITY_FORWARD_LIKE_H

libcxx/include/__utility/move.h

 
 #if !defined(_LIBCPP_HAS_NO_PRAGMA_SYSTEM_HEADER)
 #  pragma GCC system_header
 #endif
 
 _LIBCPP_BEGIN_NAMESPACE_STD
 
 template <class _Tp>
-_LIBCPP_NODISCARD_EXT inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR __libcpp_remove_reference_t<_Tp>&&
-move(_Tp&& __t) _NOEXCEPT {
+_LIBCPP_NODISCARD_EXT inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR __libcpp_remove_reference_t<_Tp>&&
+move(_LIBCPP_LIFETIMEBOUND _Tp&& __t) _NOEXCEPT {
   typedef _LIBCPP_NODEBUG __libcpp_remove_reference_t<_Tp> _Up;
   return static_cast<_Up&&>(__t);
 }
 
 template <class _Tp>
 using __move_if_noexcept_result_t =
     __conditional_t<!is_nothrow_move_constructible<_Tp>::value && is_copy_constructible<_Tp>::value, const _Tp&, _Tp&&>;
 
 template <class _Tp>
-_LIBCPP_NODISCARD_EXT inline _LIBCPP_INLINE_VISIBILITY _LIBCPP_CONSTEXPR_SINCE_CXX14 __move_if_noexcept_result_t<_Tp>
-move_if_noexcept(_Tp& __x) _NOEXCEPT {
-  return _VSTD::move(__x);
+_LIBCPP_NODISCARD_EXT inline _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX14 __move_if_noexcept_result_t<_Tp>
+move_if_noexcept(_LIBCPP_LIFETIMEBOUND _Tp& __x) _NOEXCEPT {
+  return std::move(__x);
 }
 
 _LIBCPP_END_NAMESPACE_STD
 
 #endif // _LIBCPP___UTILITY_MOVE_H

libcxx/test/libcxx/utilities/utility/forward/lifetimebound.verify.cpp

+//===----------------------------------------------------------------------===//
+//
+// 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
+//
+//===----------------------------------------------------------------------===//
+
+// UNSUPPORTED: c++03
+// ADDITIONAL_COMPILE_FLAGS: -Wno-pessimizing-move -Wno-unused-variable
+
+#include <utility>
+
+#include "test_macros.h"
+
+struct S {
+  const int& func() [[clang::lifetimebound]];
+};
+
+void func() {
+  auto&& v1 = std::move(int{});                              // expected-warning {{temporary bound to local reference 'v1' will be destroyed at the end of the full-expression}}
+  auto&& v2 = std::forward<int&&>(int{});                    // expected-warning {{temporary bound to local reference 'v2' will be destroyed at the end of the full-expression}}
+  auto&& v3 = std::forward<const int&>(S{}.func());          // expected-warning {{temporary bound to local reference 'v3' will be destroyed at the end of the full-expression}}
+  auto&& v4 = std::move_if_noexcept<const int&>(S{}.func()); // expected-warning {{temporary bound to local reference 'v4' will be destroyed at the end of the full-expression}}
+#if TEST_STD_VER >= 23
+  auto&& v5 = std::forward_like<int&&>(int{});               // expected-warning {{temporary bound to local reference 'v5' will be destroyed at the end of the full-expression}}
+#endif
+}