| Message ID | 20250304-nolibc-kselftest-harness-v1-24-adca7cd231e2@linutronix.de (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | kselftest harness and nolibc compatibility | expand |
On Tue, Mar 04, 2025 at 08:10:54AM +0100, Thomas Weißschuh wrote: > diff --git a/tools/include/nolibc/getopt.h b/tools/include/nolibc/getopt.h > new file mode 100644 > index 0000000000000000000000000000000000000000..35aee582681b79e21bce8ddbf634ae9dfdef8f1d > --- /dev/null > +++ b/tools/include/nolibc/getopt.h > @@ -0,0 +1,105 @@ > +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */ > +/* > + * getopt function definitions for NOLIBC, adapted from musl libc > + * Copyright (C) 2005-2020 Rich Felker, et al. > + * Copyright (C) 2025 Thomas Weißschuh <linux@weissschuh.net> > + */ > + > +#ifndef _NOLIBC_GETOPT_H > +#define _NOLIBC_GETOPT_H > + > +struct FILE; > +static struct FILE *const stderr; > +static int fprintf(struct FILE *stream, const char *fmt, ...); Is there a particular reason why you had to define these here and include nolibc.h at the bottom instead of doing it the usual way with the include at the top ? If that's due to a limitation in nolibc, we might want to have a closer look at it before it starts to affect other areas. Also if in the future we have to add some str* dependencies here, it would be easier if we can simply include the file as well. > +__attribute__((weak,unused,section(".data.nolibc_getopt"))) > +char *optarg; > +__attribute__((weak,unused,section(".data.nolibc_getopt"))) > +int optind = 1; > +__attribute__((weak,unused,section(".data.nolibc_getopt"))) > +int opterr = 1; > +__attribute__((weak,unused,section(".data.nolibc_getopt"))) > +int optopt; > +__attribute__((weak,unused,section(".data.nolibc_getopt"))) > +int __optpos; I think that for better readability, you'd need to either place them on the same line, or leave a blank line between each declaration. > +static __inline__ > +int getopt(int argc, char * const argv[], const char *optstring) It would be better marked with the usual unused attribute. That's a bit large for inlining, and I'm not convinced that the compiler will see any opportunity for simplifying it given that it acts on a list of actions taken from a string. Willy
On Tue, Mar 04, 2025 at 08:54:29AM +0100, Willy Tarreau wrote: > On Tue, Mar 04, 2025 at 08:10:54AM +0100, Thomas Weißschuh wrote: > > diff --git a/tools/include/nolibc/getopt.h b/tools/include/nolibc/getopt.h > > new file mode 100644 > > index 0000000000000000000000000000000000000000..35aee582681b79e21bce8ddbf634ae9dfdef8f1d > > --- /dev/null > > +++ b/tools/include/nolibc/getopt.h > > @@ -0,0 +1,105 @@ > > +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */ > > +/* > > + * getopt function definitions for NOLIBC, adapted from musl libc > > + * Copyright (C) 2005-2020 Rich Felker, et al. > > + * Copyright (C) 2025 Thomas Weißschuh <linux@weissschuh.net> > > + */ > > + > > +#ifndef _NOLIBC_GETOPT_H > > +#define _NOLIBC_GETOPT_H > > + > > +struct FILE; > > +static struct FILE *const stderr; > > +static int fprintf(struct FILE *stream, const char *fmt, ...); > > Is there a particular reason why you had to define these here > and include nolibc.h at the bottom instead of doing it the usual > way with the include at the top ? > > If that's due to a limitation in nolibc, we might want to have a > closer look at it before it starts to affect other areas. Also if > in the future we have to add some str* dependencies here, it would > be easier if we can simply include the file as well. Doing a regular #include "stdio.h" does fail with the following error: In file included from sysroot/i386/include/nolibc.h:109, from sysroot/i386/include/errno.h:26, from sysroot/i386/include/stdio.h:12, from harness-selftest.c:3, from nolibc-test.c:5: sysroot/i386/include/getopt.h: In function 'getopt': sysroot/i386/include/getopt.h:72:25: error: implicit declaration of function 'fprintf' [-Werror=implicit-function-declaration] 72 | fprintf(stderr, "%s: unrecognized option: %c\n", argv[0], *optchar); | ^~~~~~~ [+ some followup errors] The include chain is important here. The user code includes "stdio.h", which at the very beginning includes errno.h->nolibc.h->getopt.h. Now getopt.h tries to use the definitions from stdio.h. However as stdio.h was the entrypoint and is not yet fully parsed, these definitions are not yet available. > > +__attribute__((weak,unused,section(".data.nolibc_getopt"))) > > +char *optarg; > > +__attribute__((weak,unused,section(".data.nolibc_getopt"))) > > +int optind = 1; > > +__attribute__((weak,unused,section(".data.nolibc_getopt"))) > > +int opterr = 1; > > +__attribute__((weak,unused,section(".data.nolibc_getopt"))) > > +int optopt; > > +__attribute__((weak,unused,section(".data.nolibc_getopt"))) > > +int __optpos; > > I think that for better readability, you'd need to either place > them on the same line, or leave a blank line between each > declaration. Ack. > > +static __inline__ > > +int getopt(int argc, char * const argv[], const char *optstring) > > It would be better marked with the usual unused attribute. That's a > bit large for inlining, and I'm not convinced that the compiler will > see any opportunity for simplifying it given that it acts on a list > of actions taken from a string. Ack.
On Wed, Mar 05, 2025 at 08:25:14AM +0100, Thomas Weißschuh wrote: > On Tue, Mar 04, 2025 at 08:54:29AM +0100, Willy Tarreau wrote: > > On Tue, Mar 04, 2025 at 08:10:54AM +0100, Thomas Weißschuh wrote: > > > diff --git a/tools/include/nolibc/getopt.h b/tools/include/nolibc/getopt.h > > > new file mode 100644 > > > index 0000000000000000000000000000000000000000..35aee582681b79e21bce8ddbf634ae9dfdef8f1d > > > --- /dev/null > > > +++ b/tools/include/nolibc/getopt.h > > > @@ -0,0 +1,105 @@ > > > +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */ > > > +/* > > > + * getopt function definitions for NOLIBC, adapted from musl libc > > > + * Copyright (C) 2005-2020 Rich Felker, et al. > > > + * Copyright (C) 2025 Thomas Weißschuh <linux@weissschuh.net> > > > + */ > > > + > > > +#ifndef _NOLIBC_GETOPT_H > > > +#define _NOLIBC_GETOPT_H > > > + > > > +struct FILE; > > > +static struct FILE *const stderr; > > > +static int fprintf(struct FILE *stream, const char *fmt, ...); > > > > Is there a particular reason why you had to define these here > > and include nolibc.h at the bottom instead of doing it the usual > > way with the include at the top ? > > > > If that's due to a limitation in nolibc, we might want to have a > > closer look at it before it starts to affect other areas. Also if > > in the future we have to add some str* dependencies here, it would > > be easier if we can simply include the file as well. > > Doing a regular #include "stdio.h" does fail with the following error: > > In file included from sysroot/i386/include/nolibc.h:109, > from sysroot/i386/include/errno.h:26, > from sysroot/i386/include/stdio.h:12, > from harness-selftest.c:3, > from nolibc-test.c:5: > sysroot/i386/include/getopt.h: In function 'getopt': > sysroot/i386/include/getopt.h:72:25: error: implicit declaration of function 'fprintf' [-Werror=implicit-function-declaration] > 72 | fprintf(stderr, "%s: unrecognized option: %c\n", argv[0], *optchar); > | ^~~~~~~ > [+ some followup errors] > > The include chain is important here. > The user code includes "stdio.h", which at the very beginning includes > errno.h->nolibc.h->getopt.h. Now getopt.h tries to use the definitions from > stdio.h. However as stdio.h was the entrypoint and is not yet fully parsed, > these definitions are not yet available. OK got it, the usual includes dependency mess when it comes to inline code (here it's static but it's the same) :-( In the early days I had thought about placing everything in to nolibc.h and making the standard include files just stubs that would include it, but I didn't pursue that direction since I had not reached the point of the problems. Maybe for the long term we'll have to reopen that reflexion. We could even have: nolibc.h: #include "nolibc-types.h" #include "nolibc-stdio.h" #include "nolibc-stdlib.h" ... etc and stdio.h, stdlib, etc: #include "nolibc.h" That could be a clean and non-invasive change that would make sure we always include everything we need in the desired order. If we still end up with trouble due to some cross-references (since statics are painful for that), then it becomes possible to have extra -proto.h files to only declare types and prototypes, not inlines, and that will be included first. Let's discuss that later, thanks for explaining! Willy
diff --git a/tools/include/nolibc/Makefile b/tools/include/nolibc/Makefile index dceec0e1a135119108d6f4dcb3d2ec57c002ffd3..821a716094549ff9ee2fbe006fb7b3066e5e418d 100644 --- a/tools/include/nolibc/Makefile +++ b/tools/include/nolibc/Makefile @@ -31,6 +31,7 @@ all_files := \ ctype.h \ dirent.h \ errno.h \ + getopt.h \ nolibc.h \ signal.h \ stackprotector.h \ diff --git a/tools/include/nolibc/getopt.h b/tools/include/nolibc/getopt.h new file mode 100644 index 0000000000000000000000000000000000000000..35aee582681b79e21bce8ddbf634ae9dfdef8f1d --- /dev/null +++ b/tools/include/nolibc/getopt.h @@ -0,0 +1,105 @@ +/* SPDX-License-Identifier: LGPL-2.1 OR MIT */ +/* + * getopt function definitions for NOLIBC, adapted from musl libc + * Copyright (C) 2005-2020 Rich Felker, et al. + * Copyright (C) 2025 Thomas Weißschuh <linux@weissschuh.net> + */ + +#ifndef _NOLIBC_GETOPT_H +#define _NOLIBC_GETOPT_H + +struct FILE; +static struct FILE *const stderr; +static int fprintf(struct FILE *stream, const char *fmt, ...); + +__attribute__((weak,unused,section(".data.nolibc_getopt"))) +char *optarg; +__attribute__((weak,unused,section(".data.nolibc_getopt"))) +int optind = 1; +__attribute__((weak,unused,section(".data.nolibc_getopt"))) +int opterr = 1; +__attribute__((weak,unused,section(".data.nolibc_getopt"))) +int optopt; +__attribute__((weak,unused,section(".data.nolibc_getopt"))) +int __optpos; + +static __inline__ +int getopt(int argc, char * const argv[], const char *optstring) +{ + int i; + char c, d; + char *optchar; + + if (!optind) { + __optpos = 0; + optind = 1; + } + + if (optind >= argc || !argv[optind]) + return -1; + + if (argv[optind][0] != '-') { + if (optstring[0] == '-') { + optarg = argv[optind++]; + return 1; + } + return -1; + } + + if (!argv[optind][1]) + return -1; + + if (argv[optind][1] == '-' && !argv[optind][2]) + return optind++, -1; + + if (!__optpos) + __optpos++; + c = argv[optind][__optpos]; + optchar = argv[optind] + __optpos; + __optpos++; + + if (!argv[optind][__optpos]) { + optind++; + __optpos = 0; + } + + if (optstring[0] == '-' || optstring[0] == '+') + optstring++; + + i = 0; + d = 0; + do { + d = optstring[i++]; + } while (d && d != c); + + if (d != c || c == ':') { + optopt = c; + if (optstring[0] != ':' && opterr) + fprintf(stderr, "%s: unrecognized option: %c\n", argv[0], *optchar); + return '?'; + } + if (optstring[i] == ':') { + optarg = 0; + if (optstring[i + 1] != ':' || __optpos) { + optarg = argv[optind++]; + if (__optpos) + optarg += __optpos; + __optpos = 0; + } + if (optind > argc) { + optopt = c; + if (optstring[0] == ':') + return ':'; + if (opterr) + fprintf(stderr, "%s: option requires argument: %c\n", + argv[0], *optchar); + return '?'; + } + } + return c; +} + +/* make sure to include all global symbols */ +#include "nolibc.h" + +#endif /* _NOLIBC_GETOPT_H */ diff --git a/tools/include/nolibc/nolibc.h b/tools/include/nolibc/nolibc.h index 05d92afedb7258f0e3c311bf6f12be68b25d6e9a..d4043051072928f43d4d73c0e509430087c66e94 100644 --- a/tools/include/nolibc/nolibc.h +++ b/tools/include/nolibc/nolibc.h @@ -106,6 +106,7 @@ #include "time.h" #include "stackprotector.h" #include "dirent.h" +#include "getopt.h" /* Used by programs to avoid std includes */ #define NOLIBC
Introduce a getopt() implementation based on the one from musl. The only deviations are adaption to the kernel coding style and nolibc infrastructure and removal of multi-byte support. Signed-off-by: Thomas Weißschuh <thomas.weissschuh@linutronix.de> --- tools/include/nolibc/Makefile | 1 + tools/include/nolibc/getopt.h | 105 ++++++++++++++++++++++++++++++++++++++++++ tools/include/nolibc/nolibc.h | 1 + 3 files changed, 107 insertions(+)