Skip to content

Conversation

llvmbot
Copy link
Member

@llvmbot llvmbot commented Aug 27, 2025

Backport 6291b63

Requested by: @var-const

@llvmbot llvmbot requested a review from a team as a code owner August 27, 2025 22:50
@llvmbot llvmbot added this to the LLVM 21.x Release milestone Aug 27, 2025
@github-project-automation github-project-automation bot moved this to Needs Triage in LLVM Release Status Aug 27, 2025
@llvmbot
Copy link
Member Author

llvmbot commented Aug 27, 2025

@philnik777 What do you think about merging this PR to the release branch?

@llvmbot llvmbot requested a review from philnik777 August 27, 2025 22:50
@llvmbot llvmbot added the libc++ libc++ C++ Standard Library. Not GNU libstdc++. Not libc++abi. label Aug 27, 2025
@llvmbot
Copy link
Member Author

llvmbot commented Aug 27, 2025

@llvm/pr-subscribers-lldb

@llvm/pr-subscribers-libcxx

Author: None (llvmbot)

Changes

Backport 6291b63

Requested by: @var-const


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

2 Files Affected:

  • (modified) libcxx/include/fstream (+24-11)
  • (added) libcxx/test/std/input.output/file.streams/fstreams/filebuf.virtuals/overflow.writefail.pass.cpp (+72)
diff --git a/libcxx/include/fstream b/libcxx/include/fstream
index c86f709bedb80..dc5c47304f014 100644
--- a/libcxx/include/fstream
+++ b/libcxx/include/fstream
@@ -821,6 +821,14 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>
 
 template <class _CharT, class _Traits>
 typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>::overflow(int_type __c) {
+  auto __failed = [this]() {
+    if (this->pptr() == this->epptr() + 1) {
+      this->pbump(-1); // lose the character we overflowed above -- we don't really have a
+                       // choice since we couldn't commit the contents of the put area
+    }
+    return traits_type::eof();
+  };
+
   if (__file_ == nullptr)
     return traits_type::eof();
   __write_mode();
@@ -841,8 +849,9 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>
 
   if (__always_noconv_) {
     size_t __n = static_cast<size_t>(this->pptr() - this->pbase());
-    if (std::fwrite(this->pbase(), sizeof(char_type), __n, __file_) != __n)
-      return traits_type::eof();
+    if (std::fwrite(this->pbase(), sizeof(char_type), __n, __file_) != __n) {
+      return __failed();
+    }
   } else {
     if (!__cv_)
       std::__throw_bad_cast();
@@ -854,34 +863,38 @@ typename basic_filebuf<_CharT, _Traits>::int_type basic_filebuf<_CharT, _Traits>
     char* __extbuf_end = __extbuf_;
     do {
       codecvt_base::result __r = __cv_->out(__st_, __b, __p, __end, __extbuf_, __extbuf_ + __ebs_, __extbuf_end);
-      if (__end == __b)
-        return traits_type::eof();
+      if (__end == __b) {
+        return __failed();
+      }
 
       // No conversion needed: output characters directly to the file, done.
       if (__r == codecvt_base::noconv) {
         size_t __n = static_cast<size_t>(__p - __b);
-        if (std::fwrite(__b, 1, __n, __file_) != __n)
-          return traits_type::eof();
+        if (std::fwrite(__b, 1, __n, __file_) != __n) {
+          return __failed();
+        }
         break;
 
         // Conversion successful: output the converted characters to the file, done.
       } else if (__r == codecvt_base::ok) {
         size_t __n = static_cast<size_t>(__extbuf_end - __extbuf_);
-        if (std::fwrite(__extbuf_, 1, __n, __file_) != __n)
-          return traits_type::eof();
+        if (std::fwrite(__extbuf_, 1, __n, __file_) != __n) {
+          return __failed();
+        }
         break;
 
         // Conversion partially successful: output converted characters to the file and repeat with the
         // remaining characters.
       } else if (__r == codecvt_base::partial) {
         size_t __n = static_cast<size_t>(__extbuf_end - __extbuf_);
-        if (std::fwrite(__extbuf_, 1, __n, __file_) != __n)
-          return traits_type::eof();
+        if (std::fwrite(__extbuf_, 1, __n, __file_) != __n) {
+          return __failed();
+        }
         __b = const_cast<char_type*>(__end);
         continue;
 
       } else {
-        return traits_type::eof();
+        return __failed();
       }
     } while (true);
   }
diff --git a/libcxx/test/std/input.output/file.streams/fstreams/filebuf.virtuals/overflow.writefail.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/filebuf.virtuals/overflow.writefail.pass.cpp
new file mode 100644
index 0000000000000..27e06982d749b
--- /dev/null
+++ b/libcxx/test/std/input.output/file.streams/fstreams/filebuf.virtuals/overflow.writefail.pass.cpp
@@ -0,0 +1,72 @@
+//===----------------------------------------------------------------------===//
+//
+// 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: no-filesystem
+
+// setrlimit(RLIMIT_FSIZE) seems to only work as intended on Apple platforms
+// REQUIRES: target={{.+}}-apple-{{.+}}
+
+// <fstream>
+
+// Make sure that we properly handle the case where we try to write content to a file
+// but we fail to do so because std::fwrite fails.
+
+#include <cassert>
+#include <csignal>
+#include <cstddef>
+#include <fstream>
+#include <string>
+
+#include "platform_support.h"
+#include "test_macros.h"
+
+#if __has_include(<sys/resource.h>)
+#  include <sys/resource.h>
+void limit_file_size_to(std::size_t bytes) {
+  rlimit lim = {bytes, bytes};
+  assert(setrlimit(RLIMIT_FSIZE, &lim) == 0);
+
+  std::signal(SIGXFSZ, [](int) {}); // ignore SIGXFSZ to ensure std::fwrite fails
+}
+#else
+#  error No known way to limit the amount of filesystem space available
+#endif
+
+template <class CharT>
+void test() {
+  std::string temp = get_temp_file_name();
+  std::basic_filebuf<CharT> fbuf;
+  assert(fbuf.open(temp, std::ios::out | std::ios::trunc));
+
+  std::size_t const limit = 100000;
+  limit_file_size_to(limit);
+
+  std::basic_string<CharT> large_block(limit / 10, CharT(42));
+
+  std::streamsize ret;
+  std::size_t bytes_written = 0;
+  while ((ret = fbuf.sputn(large_block.data(), large_block.size())) != 0) {
+    bytes_written += ret;
+
+    // In theory, it's possible for an implementation to allow writing arbitrarily more bytes than
+    // set by setrlimit, but in practice if we bust 100x our limit, something else is wrong with the
+    // test and we'd end up looping forever.
+    assert(bytes_written < 100 * limit);
+  }
+
+  fbuf.close();
+}
+
+int main(int, char**) {
+  test<char>();
+#ifndef TEST_HAS_NO_WIDE_CHARACTERS
+  test<wchar_t>();
+#endif
+
+  return 0;
+}

@github-project-automation github-project-automation bot moved this from Needs Triage to Needs Merge in LLVM Release Status Aug 27, 2025
@var-const
Copy link
Member

@tru For context: we have originally requested a cherry-pick of this around the time it was merged to the main branch, but it looks like the bot wasn't triggered for some reason, and I don't see this commit in the release branch (see e.g. the log for <fstream>). This is an important bug fix for us and I hope this can still be cherry-picked into the upcoming release; sorry for missing the fact that this wasn't merged like we expected! (cc @ldionne)

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

We should backport #149390 as well.

ldionne and others added 3 commits August 29, 2025 14:09
…llvm#147389)

In rare circumstances, the invariants could fail to be restored.

(cherry picked from commit 6291b63)
…lvm#149390)

This patch makes the `__failed` lambda a member function on `fstream`.
This fixes two LLDB expression evaluation test failures that got
introduced with llvm#147389:
```
16:22:51  ********************
16:22:51  Unresolved Tests (2):
16:22:51    lldb-api :: commands/expression/import-std-module/list-dbg-info-content/TestDbgInfoContentListFromStdModule.py
16:22:51    lldb-api :: commands/expression/import-std-module/list/TestListFromStdModule.py
```

The expression evaluator is asserting in the Clang parser:
```
Assertion failed: (capture_size() == Class->capture_size() && "Wrong number of captures"), function LambdaExpr, file ExprCXX.cpp, line 1277.
PLEASE submit a bug report to https://github.com/llvm/llvm-project/issues/ and include the crash backtrace.
```

Ideally we'd figure out why LLDB is falling over on this lambda. But to
unblock CI for now, make this a member function.

In the long run we should figure out the LLDB bug here so libc++ doesn't
need to care about whether it uses lambdas like this or not.

(cherry picked from commit 8f4deff)
This patch works around an assertion that we hit in the `LambdaExpr`
constructor when we call it from `ASTNodeImporter::VisitLambdaExpr` (see
llvm#149477). The lambda that we
imported doesn't have the `NumCaptures` field accurately set to the one
on the source decl. This is because in `MinimalImport` mode, we skip
importing of lambda definitions:
https://github.com/llvm/llvm-project/blob/e21b0dd81928a3266df0e3ede008fb7a6676ff95/clang/lib/AST/ASTImporter.cpp#L2499

In practice we have seen this assertion occur in our `import-std-module`
test-suite when libc++ headers decide to use lambdas inside inline
function bodies (the latest failure being caused by
llvm#144602).

To avoid running into this whenever libc++ decides to use lambdas in
headers, this patch skips `ASTImport` of lambdas alltogether. Ideally
this would bubble up to the user or log as an error, but we swallow the
`ASTImportError`s currently. The only way this codepath is hit is when
lambdas are used inside functions in defined in the expression
evaluator, or when importing AST nodes from Clang modules. Both of these
are very niche use-cases (for now), so a workaround seemed appropriate.

(cherry picked from commit 0bbb794)
@tuliom
Copy link
Contributor

tuliom commented Aug 29, 2025

@philnik777 This PR now has the commits from #149390 and #154962 too.

Copy link
Contributor

@philnik777 philnik777 left a comment

Choose a reason for hiding this comment

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

The libc++ parts LGTM. I don't know enough about lldb to feel comfortable approving back-ports there.

@philnik777
Copy link
Contributor

@Michael137 Do you think the lldb patch should be back-ported?

@Michael137
Copy link
Member

@Michael137 Do you think the lldb patch should be back-ported?

Yea that would be nice. Should be safe enough

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. lldb
Projects
Status: Needs Merge
Development

Successfully merging this pull request may close these issues.

6 participants