[libc++] Add internal checks for some basic_streambuf invariants

[ldionne]

These invariants are always expected to hold, however it's not always clear that they do. Adding explicit checks for these invariants inside non-trivial functions of basic_streambuf makes that clear.

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Louis Dionne (ldionne)

Changes

These invariants are always expected to hold, however it's not always clear that they do. Adding explicit checks for these invariants inside non-trivial functions of basic_streambuf makes that clear.

---

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

1 Files Affected:

• (modified) libcxx/include/streambuf (+40)

diff --git a/libcxx/include/streambuf b/libcxx/include/streambuf
index 585ae7af65aa8..7dc4e31cc2324 100644
--- a/libcxx/include/streambuf
+++ b/libcxx/include/streambuf
@@ -119,6 +119,7 @@ protected:
 #    include <__locale>
 #    include <__type_traits/is_same.h>
 #    include <__utility/is_valid_range.h>
+#    include <__utility/scope_guard.h>
 #    include <climits>
 #    include <ios>
 #    include <iosfwd>
@@ -178,18 +179,27 @@ public:
   // Get and put areas:
   // 27.6.2.2.3 Get area:
   inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 streamsize in_avail() {
+    __check_invariants();
+    auto __guard = std::__make_scope_guard([this] { this->__check_invariants(); });
+
     if (gptr() < egptr())
       return static_cast<streamsize>(egptr() - gptr());
     return showmanyc();
   }
 
   inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 int_type snextc() {
+    __check_invariants();
+    auto __guard = std::__make_scope_guard([this] { this->__check_invariants(); });
+
     if (sbumpc() == traits_type::eof())
       return traits_type::eof();
     return sgetc();
   }
 
   inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 int_type sbumpc() {
+    __check_invariants();
+    auto __guard = std::__make_scope_guard([this] { this->__check_invariants(); });
+
     if (gptr() == egptr())
       return uflow();
     int_type __c = traits_type::to_int_type(*gptr());
@@ -198,6 +208,9 @@ public:
   }
 
   inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 int_type sgetc() {
+    __check_invariants();
+    auto __guard = std::__make_scope_guard([this] { this->__check_invariants(); });
+
     if (gptr() == egptr())
       return underflow();
     return traits_type::to_int_type(*gptr());
@@ -207,6 +220,9 @@ public:
 
   // 27.6.2.2.4 Putback:
   inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 int_type sputbackc(char_type __c) {
+    __check_invariants();
+    auto __guard = std::__make_scope_guard([this] { this->__check_invariants(); });
+
     if (eback() == gptr() || !traits_type::eq(__c, *(gptr() - 1)))
       return pbackfail(traits_type::to_int_type(__c));
     this->gbump(-1);
@@ -214,6 +230,9 @@ public:
   }
 
   inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 int_type sungetc() {
+    __check_invariants();
+    auto __guard = std::__make_scope_guard([this] { this->__check_invariants(); });
+
     if (eback() == gptr())
       return pbackfail();
     this->gbump(-1);
@@ -222,6 +241,9 @@ public:
 
   // 27.6.2.2.5 Put area:
   inline _LIBCPP_HIDE_FROM_ABI_AFTER_V1 int_type sputc(char_type __c) {
+    __check_invariants();
+    auto __guard = std::__make_scope_guard([this] { this->__check_invariants(); });
+
     if (pptr() == epptr())
       return overflow(traits_type::to_int_type(__c));
     *pptr() = __c;
@@ -317,6 +339,9 @@ protected:
   virtual streamsize showmanyc() { return 0; }
 
   virtual streamsize xsgetn(char_type* __s, streamsize __n) {
+    __check_invariants();
+    auto __guard = std::__make_scope_guard([this] { this->__check_invariants(); });
+
     int_type __c;
     streamsize __i = 0;
     while (__i < __n) {
@@ -338,6 +363,9 @@ protected:
 
   virtual int_type underflow() { return traits_type::eof(); }
   virtual int_type uflow() {
+    __check_invariants();
+    auto __guard = std::__make_scope_guard([this] { this->__check_invariants(); });
+
     if (underflow() == traits_type::eof())
       return traits_type::eof();
     int_type __c = traits_type::to_int_type(*gptr());
@@ -350,6 +378,9 @@ protected:
 
   // 27.6.2.4.5 Put area:
   virtual streamsize xsputn(const char_type* __s, streamsize __n) {
+    __check_invariants();
+    auto __guard = std::__make_scope_guard([this] { this->__check_invariants(); });
+
     streamsize __i = 0;
     while (__i < __n) {
       if (pptr() >= epptr()) {
@@ -370,6 +401,15 @@ protected:
 
   virtual int_type overflow(int_type = traits_type::eof()) { return traits_type::eof(); }
 
+  // This function checks some invariants of the class (it isn't exhaustive).
+  _LIBCPP_HIDE_FROM_ABI void __check_invariants() const {
+    _LIBCPP_ASSERT_INTERNAL(pbase() <= pptr(), "this is an invariant of the class");
+    _LIBCPP_ASSERT_INTERNAL(pptr() <= epptr(), "this is an invariant of the class");
+
+    _LIBCPP_ASSERT_INTERNAL(eback() <= gptr(), "this is an invariant of the class");
+    _LIBCPP_ASSERT_INTERNAL(gptr() <= egptr(), "this is an invariant of the class");
+  }
+
 private:
   locale __loc_;
   char_type* __binp_ = nullptr;

[ldionne]

I'm a bit conflicted: on the one hand these invariant checks are extremely useful (especially when debugging tricky issues), but on the other hand they're somewhat clunky. We could also technically do this in all methods of all classes in the library, and clearly we don't want to go down that route.

@philnik777 Do you have thoughts on this? I could also scale it down to just the two methods where checking these invariants is really helpful to debug the issue I'm looking into.

[philnik777]

I guess this LGTM. I'd much rather have some other way to say "this is an invariant", but I don't know how we'd do it.

[frederick-vs-ja]

Seems like that we should merge this now. CI failures are probably unrelated.

[ldionne]

Sorry, this went under my radar. Merging.

[Michael137]

This unfortunately broke the LLDB macOS CI:

08:14:18  Unresolved Tests (16):
08:14:18    lldb-api :: commands/expression/import-std-module/array/TestArrayFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/deque-basic/TestDequeFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/deque-dbg-info-content/TestDbgInfoContentDequeFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/forward_list-dbg-info-content/TestDbgInfoContentForwardListFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/forward_list/TestForwardListFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/iterator/TestIteratorFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/non-module-type-separation/TestNonModuleTypeSeparation.py
08:14:18    lldb-api :: commands/expression/import-std-module/shared_ptr-dbg-info-content/TestSharedPtrDbgInfoContentFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/shared_ptr/TestSharedPtrFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/unique_ptr-dbg-info-content/TestUniquePtrDbgInfoContent.py
08:14:18    lldb-api :: commands/expression/import-std-module/unique_ptr/TestUniquePtrFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/vector-of-vectors/TestVectorOfVectorsFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/weak_ptr-dbg-info-content/TestDbgInfoContentWeakPtrFromStdModule.py
08:14:18    lldb-api :: commands/expression/import-std-module/weak_ptr/TestWeakPtrFromStdModule.py
08:14:18 

Same issue as #149477

We worked around it last time by not using a lambda in #149390. But that doesn't seem like a reasonable ask in this case?

I'll try to repro this locally and see if there's an LLDB fix for this

[Michael137]

@ldionne could we revert this for now until I fix/skip the tests? I know what the root cause is, just working towards a fix. But won't be landing it until monday. If I end up not fixing it by then we can skip the tests temporarily to unblock libc++

Also, curious why these tests didn't run in pre-merge CI. Need to check

EDIT: looks like we skip these tests on Linux :(

[Michael137]

Actually nvm, I'm working around it in: #154962

No need to revert