| Message ID | YO3txvw87MjKfdpq@localhost.localdomain (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Decouple build from userspace headers | expand |
On Wed, Jul 14, 2021 at 4:47 AM Alexey Dobriyan <adobriyan@gmail.com> wrote: > > In theory, userspace headers can be under incompatible license. > > Linux by virtue of being OS kernel is fully independent piece of code > and should not require anything from userspace. As far as I know, <stdarg.h> was the only exception, which was borrowed from the compiler. I like this as long as: - license is clear (please add SPDX tag to the new header) - it works for both gcc and clang (I guess the answer is yes) I think removing <stdbool.h> and <stddef.h> are non-controversial. Mayby, you can split it into 1/2. > > For this: > > * ship minimal <stdarg.h> > 2 types, 4 macros > > * delete "-isystem" > This is what enables leakage. > > * fixup compilation where necessary. > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > --- > > Makefile | 2 +- > arch/um/include/shared/irq_user.h | 1 - > arch/um/os-Linux/signal.c | 2 +- > crypto/aegis128-neon-inner.c | 2 -- > drivers/net/wwan/iosm/iosm_ipc_imem.h | 1 - > drivers/pinctrl/aspeed/pinmux-aspeed.h | 1 - > drivers/staging/media/atomisp/pci/hive_isp_css_common/host/isp_local.h | 2 -- > include/stdarg.h | 9 +++++++++ > sound/aoa/codecs/onyx.h | 1 - > sound/aoa/codecs/tas.c | 1 - > 10 files changed, 11 insertions(+), 11 deletions(-) > > new file mode 100644 > --- /dev/null > +++ b/include/stdarg.h > @@ -0,0 +1,9 @@ This is a new file, so please add the SPDX tag. What project did you copy the code from? If gcc, is it GPL v3 (but not compatible for GPL v2) ? If clang, is it SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception Or, can we license this small portion of code as GPL v2? > +#ifndef _LINUX_STDARG_H > +#define _LINUX_STDARG_H > +typedef __builtin_va_list __gnuc_va_list; Where is __gnuc_va_list needed? BTW, once this is accepted, I'd like to change all <stdarg.h> to <linux/stdarg.h>.
On Wed, Jul 14, 2021 at 01:54:59PM +0900, Masahiro Yamada wrote: > On Wed, Jul 14, 2021 at 4:47 AM Alexey Dobriyan <adobriyan@gmail.com> wrote: > > > > In theory, userspace headers can be under incompatible license. > > > > Linux by virtue of being OS kernel is fully independent piece of code > > and should not require anything from userspace. > > As far as I know, > <stdarg.h> was the only exception, > which was borrowed from the compiler. > > > I like this as long as: > - license is clear (please add SPDX tag to the new header) > - it works for both gcc and clang (I guess the answer is yes) It should. clang version is essentially the same (with less prehistoric macrology). > I think removing <stdbool.h> and <stddef.h> are non-controversial. > Mayby, you can split it into 1/2. > > > > > > > > For this: > > > > * ship minimal <stdarg.h> > > 2 types, 4 macros > > > > * delete "-isystem" > > This is what enables leakage. > > > > * fixup compilation where necessary. > > > > Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> > > --- > > > > Makefile | 2 +- > > arch/um/include/shared/irq_user.h | 1 - > > arch/um/os-Linux/signal.c | 2 +- > > crypto/aegis128-neon-inner.c | 2 -- > > drivers/net/wwan/iosm/iosm_ipc_imem.h | 1 - > > drivers/pinctrl/aspeed/pinmux-aspeed.h | 1 - > > drivers/staging/media/atomisp/pci/hive_isp_css_common/host/isp_local.h | 2 -- > > include/stdarg.h | 9 +++++++++ > > sound/aoa/codecs/onyx.h | 1 - > > sound/aoa/codecs/tas.c | 1 - > > 10 files changed, 11 insertions(+), 11 deletions(-) > > > > > new file mode 100644 > > --- /dev/null > > +++ b/include/stdarg.h > > @@ -0,0 +1,9 @@ > > > This is a new file, so please add the SPDX tag. > What project did you copy the code from? > > If gcc, is it GPL v3 (but not compatible for GPL v2) ? It is GPL 2, brought to you by Debian! I'll add a link. http://archive.debian.org/debian/pool/main/g/gcc-4.2/ > If clang, is it > SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception > > Or, can we license this small portion of code > as GPL v2? > > > > > +#ifndef _LINUX_STDARG_H > > +#define _LINUX_STDARG_H > > +typedef __builtin_va_list __gnuc_va_list; > > Where is __gnuc_va_list needed? > > BTW, once this is accepted, I'd like to > change all <stdarg.h> to <linux/stdarg.h>. Yes. I've just realised <stdarg.h> is the wrong place: gcc -Wp,-MMD,scripts/selinux/genheaders/.genheaders.d -Wall -Wmissing-prototypes -Wstrict-prototypes -O2 -fomit-frame-pointer -std=gnu89 -I/home/ad/linux/linux-1/include/uapi -I/home/ad/linux/linux-1/include -I/home/ad/linux/linux-1/security/selinux/include -I ./scripts/selinux/genheaders -o scripts/selinux/genheaders/genheaders /home/ad/linux/linux-1/scripts/selinux/genheaders/genheaders.c In file included from /home/ad/linux/linux-1/scripts/selinux/genheaders/genheaders.c:6: /usr/include/stdio.h:52:9: error: unknown type name ‘__gnuc_va_list’ 52 | typedef __gnuc_va_list va_list; Or maybe <stdarg.h> is the right place by passing all those include directories to userspace helpers is the wrong thing to do.
> -#define signals_blocked false > +#define signals_blocked 0 Why can't we get at the kernel definition of false here? > new file mode 100644 > --- /dev/null > +++ b/include/stdarg.h > @@ -0,0 +1,9 @@ > +#ifndef _LINUX_STDARG_H > +#define _LINUX_STDARG_H > +typedef __builtin_va_list __gnuc_va_list; > +typedef __builtin_va_list va_list; > +#define va_start(v, l) __builtin_va_start(v, l) > +#define va_end(v) __builtin_va_end(v) > +#define va_arg(v, T) __builtin_va_arg(v, T) > +#define va_copy(d, s) __builtin_va_copy(d, s) > +#endif Empty lines before and after the include guards would be nice. What do we need the __gnuc_va_list typedef for? Otherwise this looks great. As a follow on maybe move the new header to <linux/stdarg.h> to make clear to everyone that we are using our own version.
On Wed, Jul 14, 2021 at 03:22:08PM +0100, Christoph Hellwig wrote: > > -#define signals_blocked false > > +#define signals_blocked 0 > > Why can't we get at the kernel definition of false here? Variable and other code surrounding this wants "int". I don't really want to expand into bool conversion. > > new file mode 100644 > > --- /dev/null > > +++ b/include/stdarg.h > > @@ -0,0 +1,9 @@ > > +#ifndef _LINUX_STDARG_H > > +#define _LINUX_STDARG_H > > +typedef __builtin_va_list __gnuc_va_list; > > +typedef __builtin_va_list va_list; > > +#define va_start(v, l) __builtin_va_start(v, l) > > +#define va_end(v) __builtin_va_end(v) > > +#define va_arg(v, T) __builtin_va_arg(v, T) > > +#define va_copy(d, s) __builtin_va_copy(d, s) > > +#endif > > Empty lines before and after the include guards would be nice. > > What do we need the __gnuc_va_list typedef for? That's because without __gnuc_va_list something didn't compile. I'm preparing second version with <linux/stdarg.h> where __gnuc_va_list is unnecessary indeed. > Otherwise this looks great. As a follow on maybe move the new header > to <linux/stdarg.h> to make clear to everyone that we are using our > own version.
On Wed, Jul 14, 2021 at 06:54:08PM +0300, Alexey Dobriyan wrote: > On Wed, Jul 14, 2021 at 03:22:08PM +0100, Christoph Hellwig wrote: > > > -#define signals_blocked false > > > +#define signals_blocked 0 > > > > Why can't we get at the kernel definition of false here? > > Variable and other code surrounding this wants "int". > I don't really want to expand into bool conversion. Maybe split this into a separate prep patch then.
On Wed, Jul 14, 2021 at 04:56:46PM +0100, Christoph Hellwig wrote: > On Wed, Jul 14, 2021 at 06:54:08PM +0300, Alexey Dobriyan wrote: > > On Wed, Jul 14, 2021 at 03:22:08PM +0100, Christoph Hellwig wrote: > > > > -#define signals_blocked false > > > > +#define signals_blocked 0 > > > > > > Why can't we get at the kernel definition of false here? > > > > Variable and other code surrounding this wants "int". > > I don't really want to expand into bool conversion. > > Maybe split this into a separate prep patch then. And get accused of KPI padding? :-)
--- a/Makefile +++ b/Makefile @@ -978,7 +978,7 @@ KBUILD_CFLAGS += -falign-functions=64 endif # arch Makefile may override CC so keep this after arch Makefile is included -NOSTDINC_FLAGS += -nostdinc -isystem $(shell $(CC) -print-file-name=include) +NOSTDINC_FLAGS += -nostdinc # warn about C99 declaration after statement KBUILD_CFLAGS += -Wdeclaration-after-statement --- a/arch/um/include/shared/irq_user.h +++ b/arch/um/include/shared/irq_user.h @@ -7,7 +7,6 @@ #define __IRQ_USER_H__ #include <sysdep/ptrace.h> -#include <stdbool.h> enum um_irq_type { IRQ_READ, --- a/arch/um/os-Linux/signal.c +++ b/arch/um/os-Linux/signal.c @@ -67,7 +67,7 @@ int signals_enabled; #ifdef UML_CONFIG_UML_TIME_TRAVEL_SUPPORT static int signals_blocked; #else -#define signals_blocked false +#define signals_blocked 0 #endif static unsigned int signals_pending; static unsigned int signals_active = 0; --- a/crypto/aegis128-neon-inner.c +++ b/crypto/aegis128-neon-inner.c @@ -15,8 +15,6 @@ #define AEGIS_BLOCK_SIZE 16 -#include <stddef.h> - extern int aegis128_have_aes_insn; void *memcpy(void *dest, const void *src, size_t n); --- a/drivers/net/wwan/iosm/iosm_ipc_imem.h +++ b/drivers/net/wwan/iosm/iosm_ipc_imem.h @@ -7,7 +7,6 @@ #define IOSM_IPC_IMEM_H #include <linux/skbuff.h> -#include <stdbool.h> #include "iosm_ipc_mmio.h" #include "iosm_ipc_pcie.h" --- a/drivers/pinctrl/aspeed/pinmux-aspeed.h +++ b/drivers/pinctrl/aspeed/pinmux-aspeed.h @@ -5,7 +5,6 @@ #define ASPEED_PINMUX_H #include <linux/regmap.h> -#include <stdbool.h> /* * The ASPEED SoCs provide typically more than 200 pins for GPIO and other --- a/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/isp_local.h +++ b/drivers/staging/media/atomisp/pci/hive_isp_css_common/host/isp_local.h @@ -16,8 +16,6 @@ #ifndef __ISP_LOCAL_H_INCLUDED__ #define __ISP_LOCAL_H_INCLUDED__ -#include <stdbool.h> - #include "isp_global.h" #include <isp2400_support.h> new file mode 100644 --- /dev/null +++ b/include/stdarg.h @@ -0,0 +1,9 @@ +#ifndef _LINUX_STDARG_H +#define _LINUX_STDARG_H +typedef __builtin_va_list __gnuc_va_list; +typedef __builtin_va_list va_list; +#define va_start(v, l) __builtin_va_start(v, l) +#define va_end(v) __builtin_va_end(v) +#define va_arg(v, T) __builtin_va_arg(v, T) +#define va_copy(d, s) __builtin_va_copy(d, s) +#endif --- a/sound/aoa/codecs/onyx.h +++ b/sound/aoa/codecs/onyx.h @@ -6,7 +6,6 @@ */ #ifndef __SND_AOA_CODEC_ONYX_H #define __SND_AOA_CODEC_ONYX_H -#include <stddef.h> #include <linux/i2c.h> #include <asm/pmac_low_i2c.h> #include <asm/prom.h> --- a/sound/aoa/codecs/tas.c +++ b/sound/aoa/codecs/tas.c @@ -58,7 +58,6 @@ * and up to the hardware designer to not wire * them up in some weird unusable way. */ -#include <stddef.h> #include <linux/i2c.h> #include <asm/pmac_low_i2c.h> #include <asm/prom.h>
In theory, userspace headers can be under incompatible license. Linux by virtue of being OS kernel is fully independent piece of code and should not require anything from userspace. For this: * ship minimal <stdarg.h> 2 types, 4 macros * delete "-isystem" This is what enables leakage. * fixup compilation where necessary. Signed-off-by: Alexey Dobriyan <adobriyan@gmail.com> --- Makefile | 2 +- arch/um/include/shared/irq_user.h | 1 - arch/um/os-Linux/signal.c | 2 +- crypto/aegis128-neon-inner.c | 2 -- drivers/net/wwan/iosm/iosm_ipc_imem.h | 1 - drivers/pinctrl/aspeed/pinmux-aspeed.h | 1 - drivers/staging/media/atomisp/pci/hive_isp_css_common/host/isp_local.h | 2 -- include/stdarg.h | 9 +++++++++ sound/aoa/codecs/onyx.h | 1 - sound/aoa/codecs/tas.c | 1 - 10 files changed, 11 insertions(+), 11 deletions(-)