| Message ID | cover.1689444638.git.falcon@tinylab.org (mailing list archive) |
|---|---|
| Headers | show |
| Series | tools/nolibc: shrink arch support | expand |
Hi Zhangjin, On Sun, Jul 16, 2023 at 02:16:36AM +0800, Zhangjin Wu wrote: > Hi, Willy, Thomas > > Thanks very much for your careful review and great suggestions, now, we > get v4 revision of the arch shrink series [1], it mainly include a new > fixup for -O0 under gcc < 11.1.0, the stackprotector support for > _start_c(), new testcases for startup code and two new test targets. > > All of the tests passed or skipped (tinyconfig + few options + > qemu-system) for both -Os and -O0: (...) First, good news, it looks OK from the nolibc-test perspective and by looking at the code, so I merged all this into branch 20230715-nolibc-next-1 Second, bad news, my preinit code doesn't build anymore due to missing definitions for statx. It's built using the default method which involves just including nolibc.h (and getting linux includes from the default path). I could simplify it to this one-liner: $ printf "int test_stat(const char *p, struct stat *b) { return stat(p,b); }\n" | gcc -c -o test.o -xc - -nostdlib -include ./sysroot/x86/include/nolibc.h In file included from ././sysroot/x86/include/nolibc.h:98:0, from <command-line>:32: ././sysroot/x86/include/sys.h:952:78: warning: 'struct statx' declared inside parameter list will not be visible outside of this definition or declaration int sys_statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf) ^~~~~ ././sysroot/x86/include/sys.h:962:74: warning: 'struct statx' declared inside parameter list will not be visible outside of this definition or declaration int statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf) ^~~~~ ././sysroot/x86/include/sys.h: In function 'statx': ././sysroot/x86/include/sys.h:964:51: warning: passing argument 5 of 'sys_statx' from incompatible pointer type [-Wincompatible-pointer-types] return __sysret(sys_statx(fd, path, flags, mask, buf)); ^~~ ././sysroot/x86/include/sys.h:952:5: note: expected 'struct statx *' but argument is of type 'struct statx *' int sys_statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf) ^~~~~~~~~ ././sysroot/x86/include/sys.h: In function 'stat': ././sysroot/x86/include/sys.h:971:15: error: storage size of 'statx' isn't known struct statx statx; ^~~~~ ././sysroot/x86/include/sys.h:974:60: error: 'STATX_BASIC_STATS' undeclared (first use in this function) ret = __sysret(sys_statx(AT_FDCWD, path, AT_NO_AUTOMOUNT, STATX_BASIC_STATS, &statx)); ^~~~~~~~~~~~~~~~~ ././sysroot/x86/include/sys.h:974:60: note: each undeclared identifier is reported only once for each function it appears in I finally found that it's due to the lack of -Isysroot/x86/include, so it used to get linux includes from those provided by glibc and these ones were missing statx since packaged for an older kernel. I knew that sooner or later I'd have to reinstall this machine but I can't get out of my head that to date I have yet not been convinced by the absolute necessity of this modification which is progressively adding more burden :-/ Time will tell... Cheers, Willy
Hi, Willy > Hi Zhangjin, > > On Sun, Jul 16, 2023 at 02:16:36AM +0800, Zhangjin Wu wrote: > > Hi, Willy, Thomas > > > > Thanks very much for your careful review and great suggestions, now, we > > get v4 revision of the arch shrink series [1], it mainly include a new > > fixup for -O0 under gcc < 11.1.0, the stackprotector support for > > _start_c(), new testcases for startup code and two new test targets. > > > > All of the tests passed or skipped (tinyconfig + few options + > > qemu-system) for both -Os and -O0: > (...) > > First, good news, it looks OK from the nolibc-test perspective and > by looking at the code, so I merged all this into branch > > 20230715-nolibc-next-1 > Thanks very much. > Second, bad news, my preinit code doesn't build anymore due to missing > definitions for statx. It's built using the default method which involves > just including nolibc.h (and getting linux includes from the default path). > I could simplify it to this one-liner: > > $ printf "int test_stat(const char *p, struct stat *b) { return stat(p,b); }\n" | > gcc -c -o test.o -xc - -nostdlib -include ./sysroot/x86/include/nolibc.h > > In file included from ././sysroot/x86/include/nolibc.h:98:0, > from <command-line>:32: > ././sysroot/x86/include/sys.h:952:78: warning: 'struct statx' declared inside parameter list will not be visible outside of this definition or declaration > int sys_statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf) > ^~~~~ > ././sysroot/x86/include/sys.h:962:74: warning: 'struct statx' declared inside parameter list will not be visible outside of this definition or declaration > int statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf) > ^~~~~ > ././sysroot/x86/include/sys.h: In function 'statx': > ././sysroot/x86/include/sys.h:964:51: warning: passing argument 5 of 'sys_statx' from incompatible pointer type [-Wincompatible-pointer-types] > return __sysret(sys_statx(fd, path, flags, mask, buf)); > ^~~ > ././sysroot/x86/include/sys.h:952:5: note: expected 'struct statx *' but argument is of type 'struct statx *' > int sys_statx(int fd, const char *path, int flags, unsigned int mask, struct statx *buf) > ^~~~~~~~~ > ././sysroot/x86/include/sys.h: In function 'stat': > ././sysroot/x86/include/sys.h:971:15: error: storage size of 'statx' isn't known > struct statx statx; > ^~~~~ > ././sysroot/x86/include/sys.h:974:60: error: 'STATX_BASIC_STATS' undeclared (first use in this function) > ret = __sysret(sys_statx(AT_FDCWD, path, AT_NO_AUTOMOUNT, STATX_BASIC_STATS, &statx)); > ^~~~~~~~~~~~~~~~~ > ././sysroot/x86/include/sys.h:974:60: note: each undeclared identifier is reported only once for each function it appears in > > I finally found that it's due to the lack of -Isysroot/x86/include, so > it used to get linux includes from those provided by glibc and these ones > were missing statx since packaged for an older kernel. > So, your local glibc may be older than 2.28 (The one we mentioned in the commit message who supports statx)? mine 2.31 glibc is ok: $ ldd --version ldd (Ubuntu GLIBC 2.31-0ubuntu9.2) 2.31 Copyright (C) 2020 Free Software Foundation, Inc. This is free software; see the source for copying conditions. There is NO warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE. Written by Roland McGrath and Ulrich Drepper. // anyone of the following commands work $ echo -e "int test_stat(const char *p, struct stat *b) { return stat(p,b); }\n" | gcc -c -o test.o -xc - -nostdlib -include sysroot/x86/include/nolibc.h $ echo -e "int test_stat(const char *p, struct stat *b) { return stat(p,b); }\n" | gcc -c -o test.o -xc - -nostdlib -Isysroot/x86/include -include ../../../include/nolibc/nolibc.h $ echo -e "int test_stat(const char *p, struct stat *b) { return stat(p,b); }\n" | gcc -c -o test.o -xc - -nostdlib -include ../../../include/nolibc/nolibc.h For older Linux systems without a newer libc may really require the installation of the linux sysroot (linux/uapi). In Ubuntu 20.04, the "struct statx" is provided by the linux-libc-dev package: $ dpkg -S /usr/include/linux/ linux-libc-dev:amd64: /usr/include/linux $ dpkg -l | grep linux-libc-dev ii linux-libc-dev:amd64 5.4.0-88.99 amd64 Linux Kernel Headers for development ii linux-libc-dev-arm64-cross 5.4.0-59.65cross1 all Linux Kernel Headers for development (for cross-compiling) ii linux-libc-dev-armel-cross 5.4.0-59.65cross1 all Linux Kernel Headers for development (for cross-compiling) ii linux-libc-dev-i386-cross 5.4.0-59.65cross1 all Linux Kernel Headers for development (for cross-compiling) ii linux-libc-dev-riscv64-cross 5.4.0-21.25cross1 all Linux Kernel Headers for development (for cross-compiling) $ grep "struct statx" -ur /usr/include/linux/ /usr/include/linux/stat.h: * Timestamp structure for the timestamps in struct statx. /usr/include/linux/stat.h:struct statx_timestamp { /usr/include/linux/stat.h:struct statx { /usr/include/linux/stat.h: struct statx_timestamp stx_atime; /* Last access time */ /usr/include/linux/stat.h: struct statx_timestamp stx_btime; /* File creation time */ /usr/include/linux/stat.h: struct statx_timestamp stx_ctime; /* Last attribute change time */ /usr/include/linux/stat.h: struct statx_timestamp stx_mtime; /* Last data modification time */ /usr/include/linux/stat.h: * Query request/result mask for statx() and struct statx::stx_mask. /usr/include/linux/stat.h:#define STATX__RESERVED 0x80000000U /* Reserved for future struct statx expansion */ This may be relative to glibc version, it is a dep of libc package: Package: libc6-dev Source: glibc Version: 2.36-9 Architecture: amd64 Maintainer: GNU Libc Maintainers <debian-glibc@lists.debian.org> Installed-Size: 11954 Depends: libc6 (= 2.36-9), libc-dev-bin (= 2.36-9), linux-libc-dev, libcrypt-dev, libnsl-dev, rpcsvc-proto > I knew that sooner or later I'd have to reinstall this machine but I > can't get out of my head that to date I have yet not been convinced by > the absolute necessity of this modification which is progressively adding > more burden :-/ Time will tell... > This may also let us think about the removing of <linux/xxx.h> from our nolibc headers? just like musl does ;-) $ grep "include <linux" -ur ../../../include/nolibc/ ../../../include/nolibc/stdlib.h:#include <linux/auxvec.h> ../../../include/nolibc/sys.h:#include <linux/fs.h> ../../../include/nolibc/sys.h:#include <linux/loop.h> ../../../include/nolibc/sys.h:#include <linux/time.h> ../../../include/nolibc/sys.h:#include <linux/auxvec.h> ../../../include/nolibc/sys.h:#include <linux/fcntl.h> /* for O_* and AT_* */ ../../../include/nolibc/sys.h:#include <linux/stat.h> /* for statx() */ ../../../include/nolibc/sys.h:#include <linux/prctl.h> ../../../include/nolibc/types.h:#include <linux/mman.h> ../../../include/nolibc/types.h:#include <linux/reboot.h> /* for LINUX_REBOOT_* */ ../../../include/nolibc/types.h:#include <linux/stat.h> ../../../include/nolibc/types.h:#include <linux/time.h> If simply put all of them to types.h, it may be too much, a new "sys/" directory with almost the same Linux type files may be required, but as an in-kernel libc, this duplication may be a "big" issue too, so, adding minimal required macros and structs in types.h may be another choice. After removing the duplicated ones, it is not that much: ../../../include/nolibc/stdlib.h:#include <linux/auxvec.h> ../../../include/nolibc/sys.h:#include <linux/fs.h> ../../../include/nolibc/sys.h:#include <linux/loop.h> ../../../include/nolibc/sys.h:#include <linux/time.h> ../../../include/nolibc/sys.h:#include <linux/fcntl.h> /* for O_* and AT_* */ ../../../include/nolibc/sys.h:#include <linux/stat.h> /* for statx() */ ../../../include/nolibc/sys.h:#include <linux/prctl.h> ../../../include/nolibc/types.h:#include <linux/mman.h> ../../../include/nolibc/types.h:#include <linux/reboot.h> /* for LINUX_REBOOT_* */ The required new macros and structs may be around 100-300 lines? but it may help to avoid the installation of sysroot completely and also avoid the cross including the linux-libc-dev package used by glibc? Best regards, Zhangjin > Cheers, > Willy
On Sun, Jul 16, 2023 at 09:17:44AM +0800, Zhangjin Wu wrote: > > I finally found that it's due to the lack of -Isysroot/x86/include, so > > it used to get linux includes from those provided by glibc and these ones > > were missing statx since packaged for an older kernel. > > > > So, your local glibc may be older than 2.28 (The one we mentioned in the > commit message who supports statx)? mine 2.31 glibc is ok: Oh definitely! It's a 2.23, and on another machine I'm having a 2.27 on an ubuntu 18 but it was built against a more recent kernel so its linux/stat.h has the required entries, and on another one I'm having a 2.17 which was built against a 3.10 kernel. > For older Linux systems without a newer libc may really require the > installation of the linux sysroot (linux/uapi). Yes. My point was that it wasn't very hard to already spot breakage on existing code built on existing setups. > > I knew that sooner or later I'd have to reinstall this machine but I > > can't get out of my head that to date I have yet not been convinced by > > the absolute necessity of this modification which is progressively adding > > more burden :-/ Time will tell... > > > > This may also let us think about the removing of <linux/xxx.h> from our > nolibc headers? just like musl does ;-) > > $ grep "include <linux" -ur ../../../include/nolibc/ > ../../../include/nolibc/stdlib.h:#include <linux/auxvec.h> > ../../../include/nolibc/sys.h:#include <linux/fs.h> > ../../../include/nolibc/sys.h:#include <linux/loop.h> > ../../../include/nolibc/sys.h:#include <linux/time.h> > ../../../include/nolibc/sys.h:#include <linux/auxvec.h> > ../../../include/nolibc/sys.h:#include <linux/fcntl.h> /* for O_* and AT_* */ > ../../../include/nolibc/sys.h:#include <linux/stat.h> /* for statx() */ > ../../../include/nolibc/sys.h:#include <linux/prctl.h> > ../../../include/nolibc/types.h:#include <linux/mman.h> > ../../../include/nolibc/types.h:#include <linux/reboot.h> /* for LINUX_REBOOT_* */ > ../../../include/nolibc/types.h:#include <linux/stat.h> > ../../../include/nolibc/types.h:#include <linux/time.h> > > If simply put all of them to types.h, it may be too much, a new "sys/" > directory with almost the same Linux type files may be required, but as > an in-kernel libc, this duplication may be a "big" issue too, so, adding > minimal required macros and structs in types.h may be another choice. (...) > The required new macros and structs may be around 100-300 lines? but it may > help to avoid the installation of sysroot completely and also avoid the cross > including the linux-libc-dev package used by glibc? No, really, that's what we used to do previously. If you remember we recently removed lots of structs and defines from various files because they used to regularly conflict with linux/foo.h (that we can't prevent users from including), while not always being 100% up-to-date. It's particularly annoying when there are typedefs for example because it's difficult to detect them, and if you redefine them you end up with build errors. We should only keep that for absolute necessity. In fact, maybe we could have these few ones precisely for statx, right after including linux/stat.h (which is supposed to provide them): #ifndef STATX_BASIC_STATS /* pre-4.10 linux uapi headers present, missing statx that we need */ #define STATX_BASIC_STATS xxx struct statx { ... }; #endif I may give this a try to see if it's sufficient to fix the build on these machines. But it's not critical anyway. I might try once I'm bored of seeing build failures. Cheers, Willy
Hi, Willy, Thomas Thanks very much for your careful review and great suggestions, now, we get v4 revision of the arch shrink series [1], it mainly include a new fixup for -O0 under gcc < 11.1.0, the stackprotector support for _start_c(), new testcases for startup code and two new test targets. All of the tests passed or skipped (tinyconfig + few options + qemu-system) for both -Os and -O0: arch/board | result ------------|------------ arm/versatilepb | 165 test(s): 158 passed, 7 skipped, 0 failed => status: warning. arm/vexpress-a9 | 165 test(s): 158 passed, 7 skipped, 0 failed => status: warning. arm/virt | 165 test(s): 158 passed, 7 skipped, 0 failed => status: warning. aarch64/virt | 165 test(s): 158 passed, 7 skipped, 0 failed => status: warning. i386/pc | 165 test(s): 158 passed, 7 skipped, 0 failed => status: warning. x86_64/pc | 165 test(s): 158 passed, 7 skipped, 0 failed => status: warning. mipsel/malta | 165 test(s): 158 passed, 7 skipped, 0 failed => status: warning. loongarch64/virt | 165 test(s): 158 passed, 7 skipped, 0 failed => status: warning. riscv64/virt | 165 test(s): 158 passed, 7 skipped, 0 failed => status: warning. s390x/s390-ccw-virtio | 165 test(s): 158 passed, 7 skipped, 0 failed => status: warning. And more, for both -Os and -O0: $ for r in run-user run-nolibc-test run-libc-test; do make clean > /dev/null; make $r | grep status; done 165 test(s): 157 passed, 8 skipped, 0 failed => status: warning 165 test(s): 157 passed, 8 skipped, 0 failed => status: warning 165 test(s): 153 passed, 12 skipped, 0 failed => status: warning // for make run-user, the euid0 and 32bit limit related tests are // skipped $ make clean && make run-user $ grep -i skip run.out 17 chroot_root [SKIPPED] 39 link_dir [SKIPPED] 62 limit_intptr_min_32 [SKIPPED] 63 limit_intptr_max_32 [SKIPPED] 64 limit_uintptr_max_32 [SKIPPED] 65 limit_ptrdiff_min_32 [SKIPPED] 66 limit_ptrdiff_max_32 [SKIPPED] 67 limit_size_max_32 [SKIPPED] // for run-libc-test, the _auxv variables, euid0, 32bits limit and // stackprotector related tests are skipped $ make clean && make run-libc-test $ grep -i skip run.out 9 environ_auxv [SKIPPED] 10 environ_total [SKIPPED] 12 auxv_addr [SKIPPED] 17 chroot_root [SKIPPED] 39 link_dir [SKIPPED] 62 limit_intptr_min_32 [SKIPPED] 63 limit_intptr_max_32 [SKIPPED] 64 limit_uintptr_max_32 [SKIPPED] 65 limit_ptrdiff_min_32 [SKIPPED] 66 limit_ptrdiff_max_32 [SKIPPED] 67 limit_size_max_32 [SKIPPED] 0 -fstackprotector not supported [SKIPPED] $ make clean >/dev/null; make run-libc-test CC=/labs/linux-lab/src/examples/musl-install/bin/musl-gcc | grep status 165 test(s): 151 passed, 12 skipped, 2 failed => status: failure // The failures are expected for musl has disabled both sbrk and brk // but not the sbrk(0); the _auxv variables, euid0, 32bits limit and // stackprotector related tests are skipped for musl too $ grep FAIL -ur run.out 9 sbrk = 1 ENOMEM [FAIL] 10 brk = -1 ENOMEM [FAIL] $ grep "SKIP" -ur run.out 9 environ_auxv [SKIPPED] 10 environ_total [SKIPPED] 12 auxv_addr [SKIPPED] 17 chroot_root [SKIPPED] 39 link_dir [SKIPPED] 62 limit_intptr_min_32 [SKIPPED] 63 limit_intptr_max_32 [SKIPPED] 64 limit_uintptr_max_32 [SKIPPED] 65 limit_ptrdiff_min_32 [SKIPPED] 66 limit_ptrdiff_max_32 [SKIPPED] 67 limit_size_max_32 [SKIPPED] 0 -fstackprotector not supported [SKIPPED] For stackprotector, gcc 13.1.0 is used to test on x86_64 standalonely: $ make run-user CROSS_COMPILE=x86_64-linux- | grep status 165 test(s): 157 passed, 8 skipped, 0 failed => status: warning $ grep stack -ur run.out 0 -fstackprotector [OK] $ make run-nolibc-test CROSS_COMPILE=x86_64-linux- | grep status 165 test(s): 157 passed, 8 skipped, 0 failed => status: warning $ grep stack -ur run.out 0 -fstackprotector [OK] Changes from v3 --> v4: * tools/nolibc: arch-*.h: add missing space after ',' tools/nolibc: fix up startup failures for -O0 under gcc < 11.1.0 Both of the above changes are for _start, it is able to merge them if necessary. The first one is old for format errors reported by scripts/checkpatch.pl The second one is for -O0 failure under gcc < 11.1.0, applied the optimize("-Os", "omit-frame-pointer") suggestion from Thomas. * tools/nolibc: remove the old sys_stat support As suggested by Willy, Document carefully about the statx supported Linux version info. * tools/nolibc: add new crt.h with _start_c The code is polished carefully for smaller size and better readability. * tools/nolibc: stackprotector.h: add empty __stack_chk_init for !_NOLIBC_STACKPROTECTOR tools/nolibc: crt.h: initialize stack protector As suggested by Thomas, init stackprotector in _start_c() too. * tools/nolibc: arm: shrink _start with _start_c tools/nolibc: aarch64: shrink _start with _start_c tools/nolibc: i386: shrink _start with _start_c tools/nolibc: x86_64: shrink _start with _start_c tools/nolibc: mips: shrink _start with _start_c tools/nolibc: loongarch: shrink _start with _start_c tools/nolibc: riscv: shrink _start with _start_c tools/nolibc: s390: shrink _start with _start_c Removed the stackprotector initialization from _start too, we already have it in _start_c(). * selftests/nolibc: add EXPECT_PTRGE, EXPECT_PTRGT, EXPECT_PTRLE, EXPECT_PTRLT selftests/nolibc: add testcases for startup code Add a new startup test group to cover the testing of argc, argv/argv0, envp/environ and _auxv. Some testcases are enhanced, some are newly added from after the discussion during v3 review. * selftests/nolibc: allow run nolibc-test locally selftests/nolibc: allow test -include /path/to/nolibc.h Two new test targets are added to cover more scenes. Hope you like this revisoin ;-) Next patchset is powerpc & powerpc64 support, after that we will send the v2 of tinyconfig support, at last the left rv32 patches (mainly 64bit time). Best regards, Zhangjin --- [1]: https://lore.kernel.org/lkml/20230715100134.GD24086@1wt.eu/ Zhangjin Wu (18): tools/nolibc: arch-*.h: add missing space after ',' tools/nolibc: fix up startup failures for -O0 under gcc < 11.1.0 tools/nolibc: remove the old sys_stat support tools/nolibc: add new crt.h with _start_c tools/nolibc: stackprotector.h: add empty __stack_chk_init for !_NOLIBC_STACKPROTECTOR tools/nolibc: crt.h: initialize stack protector tools/nolibc: arm: shrink _start with _start_c tools/nolibc: aarch64: shrink _start with _start_c tools/nolibc: i386: shrink _start with _start_c tools/nolibc: x86_64: shrink _start with _start_c tools/nolibc: mips: shrink _start with _start_c tools/nolibc: loongarch: shrink _start with _start_c tools/nolibc: riscv: shrink _start with _start_c tools/nolibc: s390: shrink _start with _start_c selftests/nolibc: add EXPECT_PTRGE, EXPECT_PTRGT, EXPECT_PTRLE, EXPECT_PTRLT selftests/nolibc: add testcases for startup code selftests/nolibc: allow run nolibc-test locally selftests/nolibc: allow test -include /path/to/nolibc.h tools/include/nolibc/Makefile | 1 + tools/include/nolibc/arch-aarch64.h | 57 +--------- tools/include/nolibc/arch-arm.h | 83 ++------------- tools/include/nolibc/arch-i386.h | 62 ++--------- tools/include/nolibc/arch-loongarch.h | 46 +------- tools/include/nolibc/arch-mips.h | 76 ++----------- tools/include/nolibc/arch-riscv.h | 69 ++---------- tools/include/nolibc/arch-s390.h | 63 ++--------- tools/include/nolibc/arch-x86_64.h | 58 ++-------- tools/include/nolibc/crt.h | 61 +++++++++++ tools/include/nolibc/stackprotector.h | 2 + tools/include/nolibc/sys.h | 63 ++--------- tools/include/nolibc/types.h | 4 +- tools/testing/selftests/nolibc/Makefile | 12 +++ tools/testing/selftests/nolibc/nolibc-test.c | 106 ++++++++++++++++++- 15 files changed, 246 insertions(+), 517 deletions(-) create mode 100644 tools/include/nolibc/crt.h