Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[FS] Make fstat work on file descriptors with no name in memfs #23470

Open
wants to merge 6 commits into
base: main
Choose a base branch
from

Conversation

hoodmane
Copy link
Collaborator

This makes fstat work on anonymous memfs file descriptors. It is split off from #23058.

This makes fstat work on anonymous memfs file descriptors. It is
split off from emscripten-core#23058.
@hoodmane
Copy link
Collaborator Author

So code size went down by a little. But now ./tools/maint/rebaseline_tests.py doesn't update anything when I run it locally but check-expectations is mad. hmm...

@hoodmane
Copy link
Collaborator Author

Well it's always possible to directly copy the ci output. Maybe check_expectations could print something like the following when it fails so I could paste it into the terminal and apply the updates that CI wanted:

echo 129211 > test/other/codesize/test_codesize_cxx_ctors1.size
echo 128623 > test/other/codesize/test_codesize_cxx_ctors2.size
echo 170879 > test/other/codesize/test_codesize_cxx_except.size
echo 144708 > test/other/codesize/test_codesize_cxx_except_wasm.size
echo 142162 > test/other/codesize/test_codesize_cxx_except_wasm_legacy.size
echo 232724 > test/other/codesize/test_codesize_cxx_mangle.size
echo 131774 > test/other/codesize/test_codesize_cxx_noexcept.size
echo 169181 > test/other/codesize/test_codesize_cxx_wasmfs.size
echo 15101 > test/other/codesize/test_codesize_hello_O0.size
echo 19394 > test/other/codesize/test_codesize_minimal_pthreads.size
echo 15101 > test/other/test_unoptimized_code_size.wasm.size
echo 12182 > test/other/test_unoptimized_code_size_no_asserts.wasm.size
echo 15101 > test/other/test_unoptimized_code_size_strict.wasm.size

@sbc100
Copy link
Collaborator

sbc100 commented Jan 22, 2025

Well it's always possible to directly copy the ci output. Maybe check_expectations could print something like the following when it fails so I could paste it into the terminal and apply the updates that CI wanted:

echo 129211 > test/other/codesize/test_codesize_cxx_ctors1.size
echo 128623 > test/other/codesize/test_codesize_cxx_ctors2.size
echo 170879 > test/other/codesize/test_codesize_cxx_except.size
echo 144708 > test/other/codesize/test_codesize_cxx_except_wasm.size
echo 142162 > test/other/codesize/test_codesize_cxx_except_wasm_legacy.size
echo 232724 > test/other/codesize/test_codesize_cxx_mangle.size
echo 131774 > test/other/codesize/test_codesize_cxx_noexcept.size
echo 169181 > test/other/codesize/test_codesize_cxx_wasmfs.size
echo 15101 > test/other/codesize/test_codesize_hello_O0.size
echo 19394 > test/other/codesize/test_codesize_minimal_pthreads.size
echo 15101 > test/other/test_unoptimized_code_size.wasm.size
echo 12182 > test/other/test_unoptimized_code_size_no_asserts.wasm.size
echo 15101 > test/other/test_unoptimized_code_size_strict.wasm.size

I'm hoping to have the CI automatically add make those change perhaps?

In order to get the same results as the CI you need to make sure you do emsdk install tot and then emcc --clear-cache before rebaselineing.

test/test_core.py Outdated Show resolved Hide resolved
src/lib/libfs.js Outdated Show resolved Hide resolved
src/lib/libfs.js Outdated Show resolved Hide resolved
src/lib/libfs.js Outdated Show resolved Hide resolved
@hoodmane
Copy link
Collaborator Author

In order to get the same results as the CI you need to make sure you do emsdk install tot and then emcc --clear-cache before rebaselineing.

I think I have been doing that, but it seems to work only some of the time...

@sbc100
Copy link
Collaborator

sbc100 commented Jan 22, 2025

In order to get the same results as the CI you need to make sure you do emsdk install tot and then emcc --clear-cache before rebaselineing.

I think I have been doing that, but it seems to work only some of the time...

Oh, there is also the case when main can actually be out of date and needs updating.

This happens when llvm of binaryen changes effect code sizes. In that case I normally push a specific update commit, which I can look into doing now.

@sbc100
Copy link
Collaborator

sbc100 commented Jan 22, 2025

Indeed if you looks at the failing CI test its called Check test expectation on target branch .. which means that expectations on main are failing, unrelated to your change.

I updated main in 12ca58d

@hoodmane
Copy link
Collaborator Author

Okay I rearranged the code in attempt to address your comments. Does it look better?

ftruncate(fd, len) {
var stream = FS.getStreamChecked(fd);
if ((stream.flags & {{{ cDefs.O_ACCMODE }}}) === {{{ cDefs.O_RDONLY}}}) {
if (len < 0 || (stream.flags & {{{ cDefs.O_ACCMODE }}}) === {{{ cDefs.O_RDONLY}}}) {
Copy link
Collaborator

Choose a reason for hiding this comment

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

Is this a separate fix?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Yeah

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants