Skip to content

Commit 1fc552d

Browse files
committed
[libc++] Fix filebuf resetting its underlying buffer upon close()
When closing a filebuf, we would previously call setbuf(0, 0). Not only does this get rid of any underlying buffer set by the user, but it also sets the unbuffered mode. This means that if the filebuf is then reopened to keep writing to a file, it would have lost track of the original user-provided buffer (or the default one which has a size of 4096), and instead it would use the unbuffered mode, which is terribly slow. This led to a bug report where users were complaining that closing and reopening the filebuf led to a significantly worse performance than using it without having closed and reopened. While this is a slightly unusual usage pattern, it should definitely work. rdar://161833214
1 parent aae2b89 commit 1fc552d

File tree

2 files changed

+138
-1
lines changed

2 files changed

+138
-1
lines changed

libcxx/include/fstream

Lines changed: 6 additions & 1 deletion
Original file line numberDiff line numberDiff line change
@@ -778,7 +778,12 @@ basic_filebuf<_CharT, _Traits>* basic_filebuf<_CharT, _Traits>::close() {
778778
if (fclose(__h.release()))
779779
__rt = nullptr;
780780
__file_ = nullptr;
781-
setbuf(0, 0);
781+
// Reset the get and the put areas without nonetheless getting rid of the
782+
// underlying buffers, which might have been configured by the user who might
783+
// reopen the stream.
784+
this->setg(nullptr, nullptr, nullptr);
785+
this->setp(nullptr, nullptr);
786+
__cm_ = __no_io_operations;
782787
}
783788
return __rt;
784789
}
Lines changed: 132 additions & 0 deletions
Original file line numberDiff line numberDiff line change
@@ -0,0 +1,132 @@
1+
//===----------------------------------------------------------------------===//
2+
//
3+
// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
4+
// See https://llvm.org/LICENSE.txt for license information.
5+
// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
6+
//
7+
//===----------------------------------------------------------------------===//
8+
9+
// This test requires the fix to std::filebuf::close() (which is defined in the
10+
// built library) from PR XXX.
11+
// UNSUPPORTED: using-built-library-before-llvm-22
12+
13+
// <fstream>
14+
15+
// basic_filebuf<charT,traits>* close();
16+
17+
//
18+
// Ensure that basic_filebuf::close() does not get rid of the underlying buffer set
19+
// via pubsetbuf(). Otherwise, reopening the stream will result in not reusing the
20+
// same buffer, which might be conforming but is definitely surprising. The standard
21+
// is not very clear on whether that is actually conforming.
22+
//
23+
24+
#include <cassert>
25+
#include <fstream>
26+
#include <string>
27+
28+
#include "platform_support.h"
29+
#include "test_macros.h"
30+
31+
struct overflow_detecting_filebuf : std::filebuf {
32+
explicit overflow_detecting_filebuf(bool* overflow_monitor) : did_overflow_(overflow_monitor) {
33+
assert(overflow_monitor != nullptr && "must provide an overflow monitor");
34+
}
35+
36+
using Traits = std::filebuf::traits_type;
37+
virtual std::filebuf::int_type overflow(std::filebuf::int_type ch = Traits::eof()) {
38+
*did_overflow_ = true;
39+
return std::filebuf::overflow(ch);
40+
}
41+
42+
private:
43+
bool* did_overflow_;
44+
};
45+
46+
int main(int, char**) {
47+
{
48+
std::string temp = get_temp_file_name();
49+
50+
bool did_overflow;
51+
overflow_detecting_filebuf buf(&did_overflow);
52+
53+
// Set a custom buffer (of size 32, reused below)
54+
char underlying_buffer[32];
55+
buf.pubsetbuf(underlying_buffer, sizeof(underlying_buffer));
56+
57+
// (1) Open a file and insert a first character. That should overflow() and set the underlying
58+
// put area to our internal buffer set above.
59+
{
60+
buf.open(temp, std::ios::out | std::ios::trunc);
61+
did_overflow = false;
62+
buf.sputc('c');
63+
assert(did_overflow == true);
64+
}
65+
66+
// (2) Now, confirm that we can still insert 30 more characters without calling
67+
// overflow, since we should be writing to the internal buffer.
68+
{
69+
did_overflow = false;
70+
for (int i = 0; i != 30; ++i) {
71+
buf.sputc('c');
72+
assert(did_overflow == false);
73+
}
74+
}
75+
76+
// (3) Writing the last character may or may not call overflow(), depending on whether
77+
// the library implementation wants to flush as soon as the underlying buffer is
78+
// full, or on the next attempt to insert. For libc++, it doesn't overflow yet.
79+
{
80+
did_overflow = false;
81+
buf.sputc('c');
82+
LIBCPP_ASSERT(!did_overflow);
83+
}
84+
85+
// (4) Writing one-too-many characters will overflow (with libc++).
86+
{
87+
did_overflow = false;
88+
buf.sputc('c');
89+
LIBCPP_ASSERT(did_overflow);
90+
}
91+
92+
// Close the stream. This should NOT unset the underlying buffer we set at the beginning
93+
// Unfortunately, the only way to check that is to repeat the above tests which tries to
94+
// tie the presence of our custom set buffer to whether overflow() gets called. This is
95+
// not entirely portable since implementations are free to call overflow() whenever they
96+
// want, but in practice this works pretty portably.
97+
buf.close();
98+
99+
// Repeat (1)
100+
{
101+
buf.open(temp, std::ios::out | std::ios::trunc);
102+
did_overflow = false;
103+
buf.sputc('c');
104+
assert(did_overflow == true);
105+
}
106+
107+
// Repeat (2)
108+
{
109+
did_overflow = false;
110+
for (int i = 0; i != 30; ++i) {
111+
buf.sputc('c');
112+
assert(did_overflow == false);
113+
}
114+
}
115+
116+
// Repeat (3)
117+
{
118+
did_overflow = false;
119+
buf.sputc('c');
120+
LIBCPP_ASSERT(!did_overflow);
121+
}
122+
123+
// Repeat (4)
124+
{
125+
did_overflow = false;
126+
buf.sputc('c');
127+
LIBCPP_ASSERT(did_overflow);
128+
}
129+
}
130+
131+
return 0;
132+
}

0 commit comments

Comments
 (0)