Fix dup syscalls so that they handle pipes correctly

[hoodmane]

library_syscall duplicates things with var newStream = FS.open(stream.path, stream.flags, 0, arg); This doesn't work for pipes because in that case stream.path is e.g., pipe[1] and then FS.open raises an error because this is an invalid path.

I changed SYSCALL.doDup so that it no longer closes the target pipe because this logic makes sense for dup2 and dup3 but not for dup or fcntl F_DUPFD which don't close the target pipe. So now doDup takes a file stream and an inclusive range of file descriptors (like the last two arguments of createStream). Now doDup also checks if the stream is a pipe, in which case it increments the pipe refcount and uses createStream to duplicate the pipe.

[sbc100]

How does this overlap with: #9396?

[hoodmane]

Looks to me like #9396 also solves this problem.

[hoodmane]

Would be good to at least add a test case for dup with a pipe though.

[hoodmane]

It would be nice to get a fix for this merged. I think when Pyodide is updated to use emscripten v3.1.6 the only remaining patch we will need is something to fix this problem.

[sbc100]

Looks like a lot of dup related tests are failing.

[hoodmane]

Maybe I fixed it?

I guess doDup is now only used in one place. I should probably add some test cases here. Maybe this could be better fixed by moving more of this stuff into syscalls.cpp or something but I'm not really sure how to do that.

[sbc100]

I guess doDup is now only used in one place.

Good call... I guess we should just inline that function now. Do you want to send or PR for that? (if not I can do it).

[sbc100]

Do you know how to run the failing tests to debug them? (See https://emscripten.org/docs/getting_started/test-suite.html)

[hoodmane]

Do you want to send or PR for that?

I can do it, in this PR or you want a different one?

[sbc100]

Do you want to send or PR for that?

I can do it, in this PR or you want a different one?

I think it would make sense as its own PR to land before this one

[hoodmane]

I'm trying to figure out even what isn't working on main. I'm getting test failures in Pyodide without this in test_dup_pipe, test_dup_temp_file and test_dup_stdout.

[sbc100]

It could be that our tests just need to be updated. Some of the tests support WASMFS as well as the legacy filesystem (the thing you are changing in this PR). It could be that the failing tests have an '#ifdef WASMFS` in there, that could be removed?

[hoodmane]

Okay here's an example of a test that would pick up the bug:

int main() {
  int p[2];
  pipe(p); 
  g = dup2(p[0], 7);
  write(p[1], "abc", 3);
  char buf[5];
  read(g, buf, 5);
  printf("buf: %s\n", buf); // should print "buf: abc\n" but it prints "buf: \n".
  return 0;
}

[hoodmane]

But it seems like there might be an additional bug now that's a regression since 2.0.16 because it does FS.open("pipe[0]", 0, 0, 7, 7) and that should return ENOENT but it doesn't.

[sbc100]

I think the issue it not lack of testing of the bug but the breakage of existing tests by this change.

Adding to one of the existing tests would be nice too, but first they need to pass

[hoodmane]

I think I fixed the test failures though, can you trigger the CI?

[hoodmane]

Oh it's just waiting on build-linux.

[sbc100]

I think I fixed the test failures though, can you trigger the CI?

Great... sorry i didn't understand that.

[hoodmane]

I think I fixed the test failures though

Guess not hmm. I must have made a mistake when I ran the tests locally...

[hoodmane]

Okay now it should work.

[sbc100]

So all

[hoodmane]

@sbc100 Could you look into the current test failure? When I try to compile and run the the test_pthread_proxying_cpp test manually, it works fine. Not sure what this has to do with the changeset here...

[sbc100]

I think its probably just a flake.. i restarted that test runner

[sbc100]

I think this is looking good now. But I also think we do need to add some new testing. IIRC there might be some tests that are currently disabled .. taking a look.

[sbc100]

It looks like we have two dup tests as of today:

1. tests/unistd/dup.c
2. tests/wasmfs/wasmfs_dup.c

Currently the later is a lot more extensive and only runs under WASMFS due to current limitations. Some of those limitations seem to have been addressed by this PR (its fails in fewer places with patch applies).

I think to land this PR we should add to the existing tests/unistd/dup.c, perhaps using the test from #14808 (comment).. and then as a followup perhaps look into the remaining reasons why tests/wasmfs/wasmfs_dup.c` doesn't work with the current FS.

[hoodmane]

Yeah good thing we added the test since the patch was still slightly wrong.

[hoodmane]

Perhaps could simplify the code by changed closeStream such that it takes the actual stream object and not the fd?

as a followup perhaps look into the remaining reasons why tests/wasmfs/wasmfs_dup.c doesn't work with the current FS

+1 for both of these.

[hoodmane]

Does this need anything else? I think it's ready to merge?

[kripken]

Looks good to me, thanks, landing.

[kripken]

It looks like this is causing errors on our roller,

https://logs.chromium.org/logs/emscripten-releases/buildbucket/cr-buildbucket/8817755857784351121/+/u/Emscripten_testsuite__upstream__cores_/stdout

Specifically test_unistd_truncate_noderawfs now fails,

-- expected
+++ actual
@@ -1,16 +1,17 @@
+f2: 4
 st_size: 6

-ftruncate(10): 0
-errno: No error information
-st_size: 10
+ftruncate(10): -1
+errno: Invalid argument
+st_size: 6

-ftruncate(4): 0
-errno: No error information
-st_size: 4
+ftruncate(4): -1
+errno: Invalid argument
+st_size: 6

 ftruncate(-1): -1
 errno: Invalid argument
-st_size: 4
+st_size: 6

 truncate(2): 0
 errno: No error information

I would guess the NODERAWFS changes here are causing that. It looks like we missed this on CI here because the test is skipped:

test_unistd_truncate_noderawfs (test_core.core3) ... skipped 'Root access invalidates this test by being able to write on readonly files'

That is, CI here runs as root I guess, while the chromium roller does not, so it can run an additional test, which failed.

Given that, I think you should be able to reproduce this locally in order to debug it @hoodmane ?

[hoodmane]

Yup reproduces locally.

[hoodmane]

Is there an easy way to make the test runner print out the compiler invocations it is using?

[kripken]

@hoodmane passing -v to tests/runner.py should do that I believe.

Doing EMCC_DEBUG=1 in the environment is another way to get full logging of each emcc command, including internals.

See also notes here for saving temp files https://emscripten.org/docs/getting_started/test-suite.html#debugging-test-failures