Define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS in compiler-rt #15411

sbc100 · 2021-11-02T18:44:14Z

In defining this I noticed that musl will also choose to use
these canonical syscalls when the alternative is not defined.

This allows us to remove a bunch of duplicate syscalls and
their implementation.

kripken

Can you elaborate on the benefits here more? I'm unclear on the purpose of this.

kripken · 2021-11-02T19:45:38Z

system/lib/fetch/asmfs.cpp

@@ -1054,7 +1054,7 @@ static long open(const char* pathname, int flags, int mode) {
  return (long)desc;
 }

-long __syscall_open(long path, long flags, ...)


These changes to use at() variants increase code size slightly. Very little, but still, is it worth the benefits here?

Yes, indeed. I think its worth it for the reduced complexity and maintenance.

It also a code size win for programs that call both flavors of these functions and fewer JS imports.

I agree it reduces complexity somewhat. I also agree it's a code size win for programs that use both. But I worry that maybe the common case is a program that uses just one, and it used to use the simple one, which is the case we'd be regressing. That might be worth measuring at least.

Actually I don't think it will be a regression even for simple such programs, at least no more than the single line which is the call to SYSCALLS.calculateAt. SYSCALLS.calculateAt is part of the SYSCALLS objects which IIRC clusure is not able to remove anyway.. so I think the calculateAt method gets included in any case. But I can/will measure.

Given that we just landed af4a408 is seems a reasonable extension to land this too.

system/lib/libc/musl/src/unistd/dup2.c

kripken

Sounds good. Basically lgtm % comments and also assuming you don't measure more than a few bytes of change in size in simple programs using files (and not using files, just to be sure, but there should be 0 change there).

kripken · 2021-11-03T17:11:45Z

system/lib/libc/musl/src/unistd/dup2.c

@@ -14,7 +14,7 @@ int dup2(int old, int new)
 #else
 	if (old==new) {
 #ifdef __EMSCRIPTEN__
-		r = __wasi_fd_is_valid(old) ? 0 : -1;
+		r = __wasi_fd_is_valid(old) ? 0 : -EBADF;


Is this a separate bugfix from this PR?

Yes indeed, let me see if I can split that out.

Actually this is bugfix, but it doesn't make sense to land separately really since this is basically in dead code. Prior to this PR SYS_dup2 is defined so this entire block of code is ifdef'd out. By removing SYS_dup2 in this PR we are forced to fix this existing bug.

I guess I could split out the removal of dup2.... that could work.

system/lib/libc/musl/src/unistd/dup3.c

system/lib/wasmfs/syscalls.cpp

The musl implemenation of dup2 works fine without this syscall as it can be implemented in terms of dup3. As part of this change I was forced to implement __wasi_fd_fdstat_get for wasmfs since dup2 not depends on this wasi syscall. Split out from #15411

sbc100 · 2022-02-19T00:28:06Z

No change in side for non-file using code (I ran ./tests/runner other.*code_size* other.*metadce* --rebase and it registered no changes).

For an example that uses fopen I see 3 bytes increase in wasm size and 18 byte increase in (non-closure) output (out of 65041 bytes!).

$ /bin/ls -la after.* before.*
-rw-r----- 1 sbc primarygroup 65041 Feb 18 16:25 before.js
-rwxr-x--- 1 sbc primarygroup  8198 Feb 18 16:25 before.wasm
-rw-r----- 1 sbc primarygroup 65058 Feb 18 16:25 after.js
-rwxr-x--- 1 sbc primarygroup  8201 Feb 18 16:25 after.wasm

kripken

lgtm, however, is it possible to delay landing this? The changes to library_syscall.js here will require updates to the external PThreadFS codebase, which is temporary and not heavily tested. I would prefer to wait on landing this until WasmFS replaces PThreadFS to avoid disruption to our partners.

sbc100 · 2022-02-22T21:51:38Z

Sure, no hurry.

In defining this I noticed that musl will also choose to use these canonical syscalls when the alternative is not defined. This allows us to remove a bunch of duplicate syscalls and their implementation.

This syscall was removed in #15411

…18984) This syscall was removed in emscripten-core#15411

sbc100 requested review from kripken and dschuff November 2, 2021 18:44

sbc100 force-pushed the use_canonical_syscalls branch from 4acbd74 to cce72bf Compare November 2, 2021 18:45

Base automatically changed from remove_unused_syscalls to main November 2, 2021 18:53

sbc100 force-pushed the use_canonical_syscalls branch 2 times, most recently from 77f3ebf to 1f6d26a Compare November 2, 2021 19:38

sbc100 requested a review from ethanalee November 2, 2021 19:38

sbc100 force-pushed the use_canonical_syscalls branch from 1f6d26a to c4ad261 Compare November 2, 2021 19:42

kripken reviewed Nov 2, 2021

View reviewed changes

sbc100 force-pushed the use_canonical_syscalls branch 2 times, most recently from c2dbfa9 to fef8f84 Compare November 2, 2021 20:15

sbc100 requested a review from kripken November 2, 2021 20:17

kripken reviewed Nov 3, 2021

View reviewed changes

sbc100 mentioned this pull request Nov 3, 2021

Remove SYS_dup2 syscall. NFC #15418

Merged

sbc100 force-pushed the use_canonical_syscalls branch from fef8f84 to 5fa5a32 Compare November 4, 2021 00:21

sbc100 force-pushed the use_canonical_syscalls branch from 5fa5a32 to 617b4d8 Compare February 19, 2022 00:07

kripken approved these changes Feb 22, 2022

View reviewed changes

sbc100 mentioned this pull request Mar 3, 2022

[WasmFS] Implement unlinkat and openat #16412

Merged

sbc100 force-pushed the use_canonical_syscalls branch 3 times, most recently from f167bd5 to 4c318cf Compare March 4, 2022 20:28

Define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS in compiler-rt

25c18cb

In defining this I noticed that musl will also choose to use these canonical syscalls when the alternative is not defined. This allows us to remove a bunch of duplicate syscalls and their implementation.

sbc100 force-pushed the use_canonical_syscalls branch from 4c318cf to 25c18cb Compare March 11, 2022 14:57

sbc100 merged commit 1a0b77c into main Mar 12, 2022

sbc100 deleted the use_canonical_syscalls branch March 12, 2022 00:26

sbc100 added a commit that referenced this pull request Mar 16, 2023

Remove unused __syscall_rename implementations. NFC

c5bfa61

This syscall was removed in #15411

sbc100 mentioned this pull request Mar 16, 2023

Remove unused __syscall_rename implementations. NFC #18984

Merged

sbc100 added a commit that referenced this pull request Mar 17, 2023

Remove unused __syscall_rename implementations. NFC (#18984)

fab93a2

This syscall was removed in #15411

impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023

Remove unused __syscall_rename implementations. NFC (emscripten-core#…

7455469

…18984) This syscall was removed in emscripten-core#15411

impact-maker pushed a commit to impact-maker/emscripten that referenced this pull request Mar 17, 2023

Remove unused __syscall_rename implementations. NFC (emscripten-core#…

6dbb540

…18984) This syscall was removed in emscripten-core#15411

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS in compiler-rt #15411

Define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS in compiler-rt #15411

Uh oh!

sbc100 commented Nov 2, 2021

Uh oh!

kripken left a comment

Uh oh!

kripken Nov 2, 2021 •

edited

Loading

Uh oh!

sbc100 Nov 2, 2021 •

edited

Loading

Uh oh!

kripken Nov 2, 2021

Uh oh!

sbc100 Nov 2, 2021

Uh oh!

sbc100 Feb 19, 2022

Uh oh!

Uh oh!

kripken left a comment

Uh oh!

kripken Nov 3, 2021

Uh oh!

sbc100 Nov 3, 2021

Uh oh!

sbc100 Nov 3, 2021

Uh oh!

Uh oh!

Uh oh!

sbc100 commented Feb 19, 2022

Uh oh!

kripken left a comment

Uh oh!

sbc100 commented Feb 22, 2022

Uh oh!

Uh oh!

Define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS in compiler-rt #15411

Define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS in compiler-rt #15411

Uh oh!

Conversation

sbc100 commented Nov 2, 2021

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

kripken Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

sbc100 Nov 2, 2021 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

kripken Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

sbc100 Nov 2, 2021

Choose a reason for hiding this comment

Uh oh!

sbc100 Feb 19, 2022

Choose a reason for hiding this comment

Uh oh!

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

kripken Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

sbc100 Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

sbc100 Nov 3, 2021

Choose a reason for hiding this comment

Uh oh!

Uh oh!

Uh oh!

sbc100 commented Feb 19, 2022

Uh oh!

kripken left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 commented Feb 22, 2022

Uh oh!

Uh oh!

kripken Nov 2, 2021 •

edited

Loading

sbc100 Nov 2, 2021 •

edited

Loading