by Zhangjin Wu

[permalink] [raw]

Subject: Re: [PATCH v4 00/18] tools/nolibc: shrink arch support

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 <[email protected]>
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

2023-07-16 07:07:31

by Willy Tarreau

[permalink] [raw]

Subject: Re: [PATCH v4 00/18] tools/nolibc: shrink arch support

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