Use correct types for syscalls #19559

kleisauke · 2023-06-08T14:35:39Z

Split from: #13007.

sbc100

If the tests pass then LGTM.

IIRC there was a reason we did it this way when porting the musl syscall layer, but I can't remember exactly what that was.. so lets just go with this if the tests pass.

sbc100 · 2023-06-08T19:25:36Z

system/lib/libc/emscripten_syscall_stubs.c

  return g_pid;
 }

-weak int __syscall_getppid() {
+weak pid_t __syscall_getppid() {
  return g_ppid;
 }

 weak int __syscall_linkat(int olddirfd, intptr_t oldpath, int newdirfd, intptr_t newpath, int flags) {


Should we also use pointer types here for oldpath/newpath? and elsewhere?

sbc100 · 2023-06-08T19:26:04Z

system/lib/libc/emscripten_syscall_stubs.c

  return g_ppid;
 }

 weak int __syscall_linkat(int olddirfd, intptr_t oldpath, int newdirfd, intptr_t newpath, int flags) {
  return -EMLINK; // no hardlinks for us
 }

-weak int __syscall_getgroups32(int size, intptr_t list) {
-  if (size < 1) {
+weak int __syscall_getgroups32(int count, intptr_t list) {


Should list be gid_t* (avoiding the case below?)

sbc100 · 2023-06-09T15:24:07Z

system/lib/compiler-rt/lib/sanitizer_common/sanitizer_emscripten.cpp

+  if (internal_fstat(fd, &st))
+    return -1;
+  return (uptr)st.st_size;
+}


Why are these new functions needed?

sbc100

Can we avoid the all the changes/additions to the sanitizers? This change seems a lot more risky once you add those in?

Also changing the socket syscalls to be varargs seems a little out of scope and perhaps separable.

sbc100 · 2023-06-09T15:32:08Z

system/lib/libc/musl/arch/emscripten/syscall_arch.h

-int __syscall_chmod(intptr_t path, int mode);
-int __syscall_getpid(void);
+typedef unsigned socklen_t;
+typedef unsigned long nfds_t;


Duplicating these definitions here seems a little sad.

kleisauke · 2023-06-09T15:54:19Z

Let me mark this as a draft, there are many more changes needed than expected. For example, doing this:

-int __syscall_openat(int dirfd, intptr_t path, int flags, ...); // mode is optional
+int __syscall_openat(int dirfd, const char *path, int flags, ...); // mode is optional

Will fail within the sanitizers (which uses the internal syscall function of musl):

emscripten/system/lib/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp

Line 262 in b182182

return internal_syscall(SYSCALL(openat), AT_FDCWD, (uptr)filename, flags);

emscripten/tools/system_libs.py

Lines 1925 to 1928 in b182182

    
           class libsanitizer_common_rt(CompilerRTLibrary, MTLibrary): 
        
             name = 'libsanitizer_common_rt' 
        
             # TODO(sbc): We should not need musl-internal headers here. 
        
             includes = ['system/lib/libc/musl/src/internal',

Details

/home/kleisauke/emscripten/system/lib/compiler-rt/lib/sanitizer_common/sanitizer_syscall_generic.inc:17:24: note: expanded from macro 'SYSCALL'
   17 | # define SYSCALL(name) SYS_ ## name
      |                        ^~~~~~~~~~~~
<scratch space>:132:1: note: expanded from here
  132 | SYS_openat
      | ^~~~~~~~~~
/home/kleisauke/emscripten/cache/sysroot/include/bits/syscall.h:61:21: note: expanded from macro 'SYS_openat'
   61 | #define SYS_openat              __syscall_openat
      |                                 ^~~~~~~~~~~~~~~~
note: (skipping 2 expansions in backtrace; use -fmacro-backtrace-limit=0 to see all)
/home/kleisauke/emscripten/system/lib/libc/musl/src/internal/syscall.h:65:80: note: expanded from macro '__SYSCALL_DISP'
   65 | #define __SYSCALL_DISP(b,...) __SYSCALL_CONCAT(b,__SYSCALL_NARGS(__VA_ARGS__))(__VA_ARGS__)
      |                                                                                ^~~~~~~~~~~
/home/kleisauke/emscripten/system/lib/libc/musl/src/internal/syscall.h:55:61: note: expanded from macro '__syscall_emscripten3'
   55 | #define __syscall_emscripten3(n,a,b,c) __syscall_emscripten(n,__scc(a),__scc(b),__scc(c))
      |                                                             ^
/home/kleisauke/emscripten/system/lib/libc/musl/src/internal/syscall.h:51:38: note: expanded from macro '__syscall_emscripten'
   51 | #define __syscall_emscripten(n, ...) n(__VA_ARGS__)
      |                                      ^
/home/kleisauke/emscripten/cache/sysroot/include/syscall_arch.h:93:5: note: candidate function not viable: no known conversion from 'uptr' (aka 'unsigned long') to 'const char *' for 2nd argument
   93 | int __syscall_openat(int dirfd, const char *path, int flags, ...); // mode is optional
      |     ^
/home/kleisauke/emscripten/system/lib/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:270:27: error: no matching function for call to '__syscall_openat'
  270 |   return internal_syscall(SYSCALL(openat), AT_FDCWD, (uptr)filename, flags,

The solution is to rework the sanitizers to avoid musl's syscall layer, similar to the NetBSD and macOS implementations.

emscripten/system/lib/compiler-rt/lib/sanitizer_common/sanitizer_mac.cpp

Lines 165 to 167 in b182182

    
           uptr internal_open(const char *filename, int flags) { 
        
             return open(filename, flags); 
        
           }

sbc100 · 2023-06-09T16:47:32Z

Yeah I think this is one reason things are the way they today.. to allow the SYSCALL stuff to work.

Perhaps we could land the initial/smaller version of this change that doesn't cause some much churn?

kleisauke mentioned this pull request Jun 8, 2023

WasmFS JS API: Implement truncate #19543

Merged

sbc100 approved these changes Jun 8, 2023

View reviewed changes

sbc100 reviewed Jun 8, 2023

View reviewed changes

sbc100 reviewed Jun 9, 2023

View reviewed changes

kleisauke marked this pull request as draft June 9, 2023 15:54

kleisauke force-pushed the correct-types branch from fe86721 to 24a54bf Compare June 15, 2023 09:12

kleisauke mentioned this pull request Jun 15, 2023

Fix signedness mismatches in the syscall layer #19631

Merged

kleisauke force-pushed the correct-types branch from 24a54bf to dfd573f Compare June 15, 2023 11:43

kleisauke force-pushed the correct-types branch 2 times, most recently from f7a239f to 7b111a0 Compare June 29, 2024 12:39

kleisauke mentioned this pull request Jun 29, 2024

Upstream emscripten compiler-rt changes (In particular the new asan, lsan, platform support) #15097

Open

kleisauke force-pushed the correct-types branch from 7b111a0 to 768961e Compare February 28, 2025 16:08

kleisauke added 4 commits March 5, 2025 10:40

Use correct types for syscalls

b8e058c

Use correct types for syscalls (2)

b5028a4

clang-format syscalls.cpp

476c428

WasmFS JS API: prefer const char*

7ffdaab

kleisauke force-pushed the correct-types branch from 768961e to 7ffdaab Compare March 5, 2025 10:03

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Use correct types for syscalls #19559

Use correct types for syscalls #19559

Uh oh!

kleisauke commented Jun 8, 2023

Uh oh!

sbc100 left a comment

Uh oh!

sbc100 Jun 8, 2023

Uh oh!

sbc100 Jun 8, 2023

Uh oh!

sbc100 Jun 9, 2023

Uh oh!

sbc100 left a comment

Uh oh!

sbc100 Jun 9, 2023

Uh oh!

kleisauke commented Jun 9, 2023

Uh oh!

sbc100 commented Jun 9, 2023

Uh oh!

Uh oh!

Use correct types for syscalls #19559

Are you sure you want to change the base?

Use correct types for syscalls #19559

Uh oh!

Conversation

kleisauke commented Jun 8, 2023

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 Jun 8, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

sbc100 left a comment

Choose a reason for hiding this comment

Uh oh!

sbc100 Jun 9, 2023

Choose a reason for hiding this comment

Uh oh!

kleisauke commented Jun 9, 2023

Uh oh!

sbc100 commented Jun 9, 2023

Uh oh!

Uh oh!