Skip to content

Commit

Permalink
Handle failure of lseek in fflush (#23624)
Browse files Browse the repository at this point in the history
Unlike on linux (it seems) the emscripten lseek can fail for various
reasons. For example, we do not support lseek on TTY or pipe file
descriptors.

Musl, it seems, just assumes that the seek succeeds and adjusts its
internal buffers accordingly. This PR update fflush such that it can
handle the failure of lseek.

The alternative to this approach would be to add `seek` support to file
our stdin implementation but that looks like a bigger change. Since it
seems reasonable for stdout and stdin to not be seekable updating musl
to handle this case seems like the right thing to do for now. If we find
a other code in the wild that depends on stdin supporting lseek we can
reconsider.

The following (14) test expectation files were updated by
running the tests with `--rebaseline`:
```                                                                      
other/codesize/test_codesize_cxx_ctors1.size: 129141 => 129147 [+6 bytes / +0.00%]
other/codesize/test_codesize_cxx_ctors2.size: 128553 => 128559 [+6 bytes / +0.00%]
other/codesize/test_codesize_cxx_except.size: 170809 => 170815 [+6 bytes / +0.00%]
other/codesize/test_codesize_cxx_except_wasm.size: 144517 => 144523 [+6 bytes / +0.00%]
other/codesize/test_codesize_cxx_except_wasm_legacy.size: 142092 => 142098 [+6 bytes / +0.00%]
other/codesize/test_codesize_cxx_lto.size: 122093 => 122099 [+6 bytes / +0.00%]
other/codesize/test_codesize_cxx_mangle.size: 232619 => 232625 [+6 bytes / +0.00%]
other/codesize/test_codesize_cxx_noexcept.size: 131704 => 131710 [+6 bytes / +0.00%]
other/codesize/test_codesize_cxx_wasmfs.size: 169111 => 169117 [+6 bytes / +0.00%]
other/codesize/test_codesize_files_wasmfs.size: 50049 => 50055 [+6 bytes / +0.01%]
other/codesize/test_codesize_hello_O0.size: 15101 => 15105 [+4 bytes / +0.03%]
other/codesize/test_codesize_minimal_O0.size: 1156 => 1160 [+4 bytes / +0.35%]
other/test_unoptimized_code_size.wasm.size: 15101 => 15105 [+4 bytes / +0.03%]
other/test_unoptimized_code_size_strict.wasm.size: 15101 => 15105 [+4 bytes / +0.03%]
                                                                         
Average change: +0.03% (+0.00% - +0.35%)                                 
```      

Fixes: #21791
  • Loading branch information
sbc100 authored Feb 8, 2025
1 parent 242af00 commit babbcc8
Show file tree
Hide file tree
Showing 20 changed files with 63 additions and 23 deletions.
11 changes: 11 additions & 0 deletions system/lib/libc/musl/src/stdio/fflush.c
Original file line number Diff line number Diff line change
Expand Up @@ -34,11 +34,22 @@ int fflush(FILE *f)
}

/* If reading, sync position, per POSIX */
#if __EMSCRIPTEN__
/* Handle failues of lseek, which can happen in emscripten, e.g. for stdin etc */
if (f->rpos != f->rend) {
if (f->seek(f, f->rpos-f->rend, SEEK_CUR) == 0) {
/* Clear read and write modes */
f->wpos = f->wbase = f->wend = 0;
f->rpos = f->rend = 0;
}
}
#else
if (f->rpos != f->rend) f->seek(f, f->rpos-f->rend, SEEK_CUR);

/* Clear read and write modes */
f->wpos = f->wbase = f->wend = 0;
f->rpos = f->rend = 0;
#endif

FUNLOCK(f);
return 0;
Expand Down
14 changes: 7 additions & 7 deletions test/common.py
Original file line number Diff line number Diff line change
Expand Up @@ -1492,7 +1492,8 @@ def cleanup(line):

def run_js(self, filename, engine=None, args=None,
assert_returncode=0,
interleaved_output=True):
interleaved_output=True,
**kwargs):
# use files, as PIPE can get too full and hang us
stdout_file = self.in_dir('stdout')
stderr_file = None
Expand All @@ -1514,7 +1515,8 @@ def run_js(self, filename, engine=None, args=None,
jsrun.run_js(filename, engine, args,
stdout=stdout,
stderr=stderr,
assert_returncode=assert_returncode)
assert_returncode=assert_returncode,
**kwargs)
except subprocess.TimeoutExpired as e:
timeout_error = e
except subprocess.CalledProcessError as e:
Expand Down Expand Up @@ -1937,9 +1939,9 @@ def _build_and_run(self, filename, expected_output, args=None,
includes=None,
assert_returncode=0, assert_identical=False, assert_all=False,
check_for_error=True, force_c=False, emcc_args=None,
interleaved_output=True,
regex=False,
output_basename=None):
output_basename=None,
**kwargs):
logger.debug(f'_build_and_run: {filename}')

if no_build:
Expand All @@ -1963,9 +1965,7 @@ def _build_and_run(self, filename, expected_output, args=None,
if len(engines) == 0:
self.fail('No JS engine present to run this test with. Check %s and the paths therein.' % config.EM_CONFIG)
for engine in engines:
js_output = self.run_js(js_file, engine, args,
assert_returncode=assert_returncode,
interleaved_output=interleaved_output)
js_output = self.run_js(js_file, engine, args, assert_returncode=assert_returncode, **kwargs)
js_output = js_output.replace('\r\n', '\n')
if expected_output:
if type(expected_output) not in [list, tuple]:
Expand Down
5 changes: 3 additions & 2 deletions test/jsrun.py
Original file line number Diff line number Diff line change
Expand Up @@ -86,7 +86,7 @@ def require_engine(engine):
def run_js(filename, engine, args=None,
stdin=None, stdout=PIPE, stderr=None, cwd=None,
full_output=False, assert_returncode=0, skip_check=False,
timeout=DEFAULT_TIMEOUT):
timeout=DEFAULT_TIMEOUT, **kwargs):
"""Execute javascript code generated by tests, with possible timeout."""

# We used to support True here but we no longer do. Assert here just in case.
Expand All @@ -96,6 +96,7 @@ def run_js(filename, engine, args=None,
raise Exception('output file not found: ' + filename)

command = make_command(os.path.abspath(filename), engine, args)
kwargs.setdefault('text', True)
if common.EMTEST_VERBOSE:
print(f"Running: '{shared.shlex_join(command)}'")
try:
Expand All @@ -106,7 +107,7 @@ def run_js(filename, engine, args=None,
stderr=stderr,
cwd=cwd,
timeout=timeout,
universal_newlines=True)
**kwargs)
except Exception:
# the failure may be because the engine is not present. show the proper
# error in that case
Expand Down
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_ctors1.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
129141
129147
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_ctors2.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
128553
128559
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_except.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
170809
170815
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_except_wasm.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
144517
144523
Original file line number Diff line number Diff line change
@@ -1 +1 @@
142092
142098
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_lto.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
122093
122099
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_mangle.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
232619
232625
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_noexcept.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
131704
131710
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_cxx_wasmfs.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
169111
169117
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_files_wasmfs.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
50049
50055
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_hello_O0.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
15101
15105
2 changes: 1 addition & 1 deletion test/other/codesize/test_codesize_minimal_O0.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
1156
1160
17 changes: 17 additions & 0 deletions test/other/test_stdin_fflush.c
Original file line number Diff line number Diff line change
@@ -0,0 +1,17 @@
#include <stdio.h>
#include <string.h>

int main(int argc, char** argv) {
char buf[10] = {0};

int cnt = 0;
while (!feof(stdin)) {
int cnt = fread(buf, 1, 3, stdin);
printf("read %d bytes: '%s'\n", cnt, buf);
fflush(stdin);
memset(buf, 0, sizeof(buf));
}

printf("done\n");
return 0;
}
6 changes: 6 additions & 0 deletions test/other/test_stdin_fflush.out
Original file line number Diff line number Diff line change
@@ -0,0 +1,6 @@
read 3 bytes: 'foo'
read 3 bytes: '
ba'
read 2 bytes: 'r
'
done
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size.wasm.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
15101
15105
2 changes: 1 addition & 1 deletion test/other/test_unoptimized_code_size_strict.wasm.size
Original file line number Diff line number Diff line change
@@ -1 +1 @@
15101
15105
5 changes: 5 additions & 0 deletions test/test_other.py
Original file line number Diff line number Diff line change
Expand Up @@ -1730,6 +1730,11 @@ def test_module_stdin(self):
self.emcc_args += ['--pre-js', 'pre.js']
self.do_runf('module/test_stdin.c', 'hello, world!')

@crossplatform
def test_stdin_fflush(self):
# Force text=False here so that newlines are not treated differently on windows.
self.do_other_test('test_stdin_fflush.c', input=b'foo\nbar\n', text=False)

@crossplatform
def test_module_stdout_stderr(self):
self.set_setting('FORCE_FILESYSTEM')
Expand Down

0 comments on commit babbcc8

Please sign in to comment.