Skip to content

Conversation

higher-performance
Copy link
Contributor

This should fix #112234.

@higher-performance higher-performance requested a review from a team as a code owner October 16, 2024 20:53
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Oct 16, 2024
@llvmbot
Copy link
Member

llvmbot commented Oct 16, 2024

@llvm/pr-subscribers-libcxx

Author: None (higher-performance)

Changes

This should fix #112234.


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

1 Files Affected:

  • (modified) libcxx/include/string (+1-1)
diff --git a/libcxx/include/string b/libcxx/include/string
index e8c9bcee53e3df..f6dd7293de90ca 100644
--- a/libcxx/include/string
+++ b/libcxx/include/string
@@ -1213,7 +1213,7 @@ public:
       __alloc_traits::deallocate(__alloc(), __get_long_pointer(), __get_long_cap());
   }
 
-  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 operator __self_view() const _NOEXCEPT {
+  _LIBCPP_HIDE_FROM_ABI _LIBCPP_CONSTEXPR_SINCE_CXX20 operator __self_view() const _NOEXCEPT _LIBCPP_LIFETIMEBOUND {
     return __self_view(typename __self_view::__assume_valid(), data(), size());
   }
 

@higher-performance higher-performance force-pushed the string-view-lifetimebound branch from ed5f234 to 554d5ec Compare October 16, 2024 20:54
@higher-performance higher-performance changed the title Add lifetimebound to std::basic_string::opeator std::string_view Add lifetimebound to std::basic_string::operator std::string_view Oct 16, 2024
@usx95
Copy link
Contributor

usx95 commented Oct 16, 2024

I feel this could be dealt with the generic GSL owner-to-pointer conversion. Also, we prefer not to annotate the library and instead inferLifetimebound. This allows clang to diagnose cases irrespective of the underlying library.

That said, I don't think we need any of that to fix this bug. For example, we already do a good job when GSL pointers and lifetimebound interact as in the bug: https://godbolt.org/z/9M9PsEGs7

I feel we need to fix the analysis for assignment operator here which would fix cases beyond std::string_view.
cc: @Xazax-hun @hokein

@frederick-vs-ja
Copy link
Contributor

We may consider adding a new *.verify.cpp to libcxx/strings/basic.string/string.access. Even if no product code change is needed, the verification for warning can still be helpful.

@higher-performance
Copy link
Contributor Author

In principle I would probably wish to see both implemented, since this would similarly work with compilers other than Clang (assuming _LIBCPP_LIFETIMEBOUND is redefined appropriately).

But I do see why the built-in solution is preferable currently. I just implemented it this way because the fix was trivial and we would have it working immediately. If you don't mind delaying having a solution then feel free not to merge - up to you.

@higher-performance
Copy link
Contributor Author

I'm closing this since it will either be superseded by the compiler fix, or by #112751.

@higher-performance higher-performance deleted the string-view-lifetimebound branch October 17, 2024 17:27
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

No dangling assignment involving lifetimebound argument
4 participants