[libcxx] Fix xsgetn in basic_filebuf

[michaelrj-google]

The optimized version of xsgetn for basic_filebuf added in #165223 has
an issue where if the reads come from both the buffer and the
filesystem it returns the wrong number of characters. This patch should
address the issue.

[michaelrj-google]

I intended to include a test with this change, but when I tried to use the buffer in an ifstream it seemed to have all its pointers as null.

[github-actions]

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

[llvmbot]

@llvm/pr-subscribers-libcxx

Author: Michael Jones (michaelrj-google)

Changes

The optimized version of xsgetn for basic_filebuf added in #165223 has
an issue where if the reads come from both the buffer and the
filesystem it returns the wrong number of characters. This patch should
address the issue.

---

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

1 Files Affected:

• (modified) libcxx/include/fstream (+8-2)

diff --git a/libcxx/include/fstream b/libcxx/include/fstream
index b07ca636094af..90e35740c17cf 100644
--- a/libcxx/include/fstream
+++ b/libcxx/include/fstream
@@ -315,8 +315,14 @@ protected:
         traits_type::copy(__str, this->gptr(), __n);
         this->__gbump_ptrdiff(__n);
       }
-      if (__len - __n >= this->egptr() - this->eback())
-        return std::fread(__str + __n, sizeof(char_type), __len - __n, __file_);
+      const streamsize __remainder    = __len - __n;
+      const streamsize __buffer_space = this->egptr() - this->eback();
+
+      if (__remainder >= __buffer_space)
+        return std::fread(__str + __n, sizeof(char_type), __remainder, __file_) + __n;
+      else if (__remainder > 0)
+        return basic_streambuf<_CharT, _Traits>::xsgetn(__str + __n, __remainder) + __n;
+      return __n;
     }
     return basic_streambuf<_CharT, _Traits>::xsgetn(__str, __len);
   }

[Sterling-Augustine]

I'm not sure what to do without a test, so adding reviewers who also saw problems on #165223

[philnik777]

Sorry for chiming in so late, I didn't see this until now. I think this test should do the trick:

diff --git a/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/xsgetn.pass.cpp b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/xsgetn.pass.cpp
new file mode 100644
index 000000000000..ec555ea4d259
--- /dev/null
+++ b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/xsgetn.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
+//
+//===----------------------------------------------------------------------===//
+
+// FILE_DEPENDENCIES: xsgetn.test.dat
+
+// <fstream>
+
+// template <class charT, class traits = char_traits<charT> >
+// class basic_ifstream
+
+// streamsize xsgetn(char_type*, streamsize) override;
+
+// This isn't a required override by the standard, but most implementations override it, since it allows for
+// significantly improved performance in some cases. All of this code is required to work, so this isn't a libc++
+// extension
+
+#include <cassert>
+#include <fstream>
+
+#include "test_macros.h"
+
+int main(int, char**) {
+  {
+    char buffer[10];
+    std::ifstream fs("xsgetn.test.dat");
+    std::filebuf* fb = fs.rdbuf();
+    fb->pubsetbuf(buffer, 10);
+
+    // Ensure that the buffer is set up
+    assert(fb->sgetc() == 't');
+
+    std::string str(5, '\0');
+
+    { // Check that a read smaller than the buffer works fine
+      assert(fb->sgetn(str.data(), 5) == 5);
+      assert(str == "this ");
+    }
+    { // Check that reading up to the buffer end works fine
+      assert(fb->sgetn(str.data(), 5) == 5);
+      assert(str == "is so");
+    }
+    { // Check that reading from an empty buffer, but more than the buffer can hold works fine
+      str.resize(12);
+      assert(fb->sgetn(str.data(), 12) == 12);
+      assert(str == "me random da");
+    }
+    { // Check that reading from a non-empty buffer, and more than the buffer can hold works fine
+      // Fill the buffer up
+      str.resize(2);
+      assert(fb->sgetn(str.data(), 2) == 2);
+      assert(str == "ta");
+
+      // Do the actual check
+      str.resize(12);
+      assert(fb->sgetn(str.data(), 12) == 12);
+      assert(str == " to be able ");
+    }
+    { // Check that trying to read more than the file size works fine
+      str.resize(30);
+      assert(fb->sgetn(str.data(), 30) == 24);
+      str.resize(24);
+      assert(str == "to test buffer behaviour");
+    }
+  }
+
+  return 0;
+}
diff --git a/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/xsgetn.test.dat b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/xsgetn.test.dat
new file mode 100644
index 000000000000..06d663b9bf23
--- /dev/null
+++ b/libcxx/test/std/input.output/file.streams/fstreams/ifstream.members/xsgetn.test.dat
@@ -0,0 +1 @@
+this is some random data to be able to test buffer behaviour
\ No newline at end of file

[Sterling-Augustine]

I will commit the test separately shortly.