These "new" syscalls have been added in 2.6.16, more than 16 years ago.
Surely that's enough time to migrate. Glibc 2.33 is using them on both
i386 and x86_64. Android has an selinux filter to block the legacy
syscalls in the apps.
Details
Diff Detail
- Repository
- rG LLVM Github Monorepo
Event Timeline
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | ||
---|---|---|
346 |
compiler-rt/lib/sanitizer_common/sanitizer_platform.h | ||
---|---|---|
300–301 | Can we remove this macro and replace it with SANITIZER_LINUX everywhere? |
address comments
compiler-rt/lib/sanitizer_common/sanitizer_platform.h | ||
---|---|---|
300–301 | Sure. I wanted to preserve some context in the source, but that does not really matter. |
compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp | ||
---|---|---|
356–365 | I think it is! Removed. We also need to do something prepocessor code formatting. This spacing is insane and makes the code unreadable. |
This seems to have broken the s390x build bots, there are now >500 failing sanitizer test cases:
https://lab.llvm.org/buildbot/#/builders/94/builds/8761
Not sure yet what exactly the problem is; those syscalls certainly should be available, but we may run into some other issues in these new code paths.
Reverting this didn't help for our Android issue. I'll leave relanding to you because of the s390x comment further up, but from our end I think this is good for relanding.
Thanks. Ulrich, could you help me with the s390 failures? I can reland with a #define SANITIZER_USES_CANONICAL_LINUX_SYSCALLS !SANITIZER_S390 for now, but it would be great to remove the non-canonical code path completely.
Looks like the problem is in internal_fork. This code path:
return internal_syscall(SYSCALL(clone), SIGCHLD, 0);
fails on s390x because the low-level clone syscall interface is different between architectures. To quote the Linux manpage:
The raw system call interface on x86-64 and some other architectures (including sh, tile, and alpha) is roughly: long clone(unsigned long flags, void *child_stack, int *ptid, int *ctid, unsigned long newtls); [... ] On the cris and s390 architectures, the order of the first two arguments is reversed: long clone(void *child_stack, unsigned long flags, int *ptid, int *ctid, unsigned long newtls);
This means that instead of passing NULL as child stack pointer, we're passing SIGCHLD (0x11), and so the child crashes when accessing the stack.
Thanks! I'll update the patch later. Apparently there is like 5 different signatures...
LGTM for SystemZ.
(As an aside, what is the reason for not just using the fork syscall anyway? I thought fork should be completely equivalent to clone with the termination signal set to SIGCHLD and no other flags?)
Thanks!
Primarily, Android's seccomp sandbox only allows fork as an exception for 32-bits apps but not for the system binaries. Arm64 does not even have a fork syscall; glibc uses clone on all targets.
This feels like moving in the right direction.
This change unfortunately broke the build on Linux/sparc64.
I'm not sure whether newfstatat is supposed to be available on Linux/sparc64 [1]:
FAILED: projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoHooks.sparcv9.dir/sanitizer_linux.cpp.o /usr/bin/c++ -DHAVE_RPC_XDR_H=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -I/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/stage1/projects/compiler-rt/lib/sanitizer_common -I/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common -I/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/stage1/include -I/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/llvm/include -I/var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/.. -fPIC -fno-semantic-interposition -fvisibility-inlines-hidden -Werror=date-time -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -pedantic -Wno-long-long -Wimplicit-fallthrough -Wno-maybe-uninitialized -Wno-class-memaccess -Wno-redundant-move -Wno-pessimizing-move -Wno-noexcept-type -Wdelete-non-virtual-dtor -Wsuggest-override -Wno-comment -Wmisleading-indentation -fdiagnostics-color -ffunction-sections -fdata-sections -Wall -std=c++14 -Wno-unused-parameter -O3 -DNDEBUG -m64 -fPIC -fno-builtin -fno-exceptions -fomit-frame-pointer -funwind-tables -fno-stack-protector -fvisibility=hidden -fno-lto -O3 -g -Wno-variadic-macros -nostdinc++ -Wno-format -fno-rtti -Wframe-larger-than=570 -DSANITIZER_SUPPORTS_WEAK_HOOKS=0 -UNDEBUG -std=c++14 -MD -MT projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoHooks.sparcv9.dir/sanitizer_linux.cpp.o -MF projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoHooks.sparcv9.dir/sanitizer_linux.cpp.o.d -o projects/compiler-rt/lib/sanitizer_common/CMakeFiles/RTSanitizerCommonNoHooks.sparcv9.dir/sanitizer_linux.cpp.o -c /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp In file included from /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:191: /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp: In function ‘__sanitizer::uptr __sanitizer::internal_stat(const char*, void*)’: /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_syscall_generic.inc:19:24: error: ‘__NR_newfstatat’ was not declared in this scope 19 | # define SYSCALL(name) __NR_ ## name | ^~~~~ /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:347:27: note: in expansion of macro ‘SYSCALL’ 347 | return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf, | ^~~~~~~ /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp: In function ‘__sanitizer::uptr __sanitizer::internal_lstat(const char*, void*)’: /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_syscall_generic.inc:19:24: error: ‘__NR_newfstatat’ was not declared in this scope 19 | # define SYSCALL(name) __NR_ ## name | ^~~~~ /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:370:27: note: in expansion of macro ‘SYSCALL’ 370 | return internal_syscall(SYSCALL(newfstatat), AT_FDCWD, (uptr)path, (uptr)buf, | ^~~~~~~ /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp: In member function ‘__sanitizer::SignalContext::WriteFlag __sanitizer::SignalContext::GetWriteFlag() const’: /var/lib/buildbot/workers/debian-stadler-sparc64/clang-sparc64-linux-multistage/llvm/compiler-rt/lib/sanitizer_common/sanitizer_linux.cpp:1818:12: warning: unused variable ‘ucontext’ [-Wunused-variable] 1818 | Context *ucontext = (Context *)context; | ^~~~~~~~
Any idea how to fix this? Do we just need to wire up newfstatat in the kernel on SPARC?
FWIW, I have already a draft for fixing this. Will create a revision tomorrow.
EDIT: I mean the build failure on sparc64 ;).
Thanks. Sorry for the breakage. It appears that sparc64 is the only 64-bit target without newfstatat syscall.
I've considered keeping the non-canonical-syscalls code path for cases like this, but getting rid of all the preprocessor conditionals was too tempting :)
We should re-lint this entire file at some point, but there is never a good time for that - you don't want to mix it with a functional change, and then it also makes it harder to revert changes so you'd want to wait a while after any non-trivial commit... Maybe after you land the sparc fix.
That's not true, alpha and hppa64 are the same from what I can tell, maybe others too, I stopped looking after that point, but sparc64 may be the only architecture compiler-rt supports that's like that.
I've considered keeping the non-canonical-syscalls code path for cases like this, but getting rid of all the preprocessor conditionals was too tempting :)
We should re-lint this entire file at some point, but there is never a good time for that - you don't want to mix it with a functional change, and then it also makes it harder to revert changes so you'd want to wait a while after any non-trivial commit... Maybe after you land the sparc fix.
Sure, but you should never be changing the whitespace for #else without also changing #if to match (or vice-versa, or ditto with #endif), currently you have mismatched indentation all over the place which is what I meant by wonky, it's extremely misleading and hard to read.
Maybe after you land the sparc fix.
In case you have time for review, I have posted the SPARC fix: https://reviews.llvm.org/D125572