| Message ID | 20230411092747.3759032-1-kevin.brodsky@arm.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [RESEND] uapi/linux/const.h: Prefer ISO-friendly __typeof__ | expand |
Hi Kevin, > typeof is (still) a GNU extension, which means that it cannot be > used when building ISO C (e.g. -std=c99). It should therefore be > avoided in uapi headers in favour of the ISO-friendly __typeof__. IMHO UAPI are built with -std=c90 -Wall -Werror=implicit-function-declaration (see usr/include/Makefile). But one or the other, you're right both require __typeof__. "If you are writing a header file that must work when included in ISO C programs, write __typeof__ instead of typeof." https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Typeof.html Reviewed-by: Petr Vorel <pvorel@suse.cz> Tested-by: Petr Vorel <pvorel@suse.cz> Kind regards, Petr > Unfortunately this issue could not be detected by > CONFIG_UAPI_HEADER_TEST=y as the __ALIGN_KERNEL() macro is not > expanded in any uapi header. > Reported-by: Ruben Ayrapetyan <ruben.ayrapetyan@arm.com> > Tested-by: Ruben Ayrapetyan <ruben.ayrapetyan@arm.com> > Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com> > --- > include/uapi/linux/const.h | 2 +- > 1 file changed, 1 insertion(+), 1 deletion(-) > diff --git a/include/uapi/linux/const.h b/include/uapi/linux/const.h > index af2a44c08683..a429381e7ca5 100644 > --- a/include/uapi/linux/const.h > +++ b/include/uapi/linux/const.h > @@ -28,7 +28,7 @@ > #define _BITUL(x) (_UL(1) << (x)) > #define _BITULL(x) (_ULL(1) << (x)) > -#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) > +#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (__typeof__(x))(a) - 1) > #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) > #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
> Hi Kevin, > > typeof is (still) a GNU extension, which means that it cannot be > > used when building ISO C (e.g. -std=c99). It should therefore be > > avoided in uapi headers in favour of the ISO-friendly __typeof__. > IMHO UAPI are built with -std=c90 -Wall -Werror=implicit-function-declaration > (see usr/include/Makefile). > But one or the other, you're right both require __typeof__. > "If you are writing a header file that must work when included in ISO C > programs, write __typeof__ instead of typeof." > https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Typeof.html > Reviewed-by: Petr Vorel <pvorel@suse.cz> > Tested-by: Petr Vorel <pvorel@suse.cz> IMHO problem was introduced when -std=c90 was added (back then the code was in include/uapi/linux/kernel.h). Fixes: d6fc9fcbaa65 ("kbuild: compile-test exported headers to ensure they are self-contained") Kind regards, Petr > Kind regards, > Petr > > Unfortunately this issue could not be detected by > > CONFIG_UAPI_HEADER_TEST=y as the __ALIGN_KERNEL() macro is not > > expanded in any uapi header. > > Reported-by: Ruben Ayrapetyan <ruben.ayrapetyan@arm.com> > > Tested-by: Ruben Ayrapetyan <ruben.ayrapetyan@arm.com> > > Signed-off-by: Kevin Brodsky <kevin.brodsky@arm.com> > > --- > > include/uapi/linux/const.h | 2 +- > > 1 file changed, 1 insertion(+), 1 deletion(-) > > diff --git a/include/uapi/linux/const.h b/include/uapi/linux/const.h > > index af2a44c08683..a429381e7ca5 100644 > > --- a/include/uapi/linux/const.h > > +++ b/include/uapi/linux/const.h > > @@ -28,7 +28,7 @@ > > #define _BITUL(x) (_UL(1) << (x)) > > #define _BITULL(x) (_ULL(1) << (x)) > > -#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) > > +#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (__typeof__(x))(a) - 1) > > #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) > > #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))
On Tue, 11 Apr 2023 23:39:46 +0200 Petr Vorel <pvorel@suse.cz> wrote: > > Hi Kevin, > > > > typeof is (still) a GNU extension, which means that it cannot be > > > used when building ISO C (e.g. -std=c99). It should therefore be > > > avoided in uapi headers in favour of the ISO-friendly __typeof__. > > > IMHO UAPI are built with -std=c90 -Wall -Werror=implicit-function-declaration > > (see usr/include/Makefile). > > But one or the other, you're right both require __typeof__. > > > "If you are writing a header file that must work when included in ISO C > > programs, write __typeof__ instead of typeof." > > https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Typeof.html > > > Reviewed-by: Petr Vorel <pvorel@suse.cz> > > Tested-by: Petr Vorel <pvorel@suse.cz> > > IMHO problem was introduced when -std=c90 was added (back then the code was in > include/uapi/linux/kernel.h). Well... what actually _is_ the problem? Presumably build issues under some circumstances. Could we please see an instance of those issues and a description of the circumstances under which they occur? > Fixes: d6fc9fcbaa65 ("kbuild: compile-test exported headers to ensure they are self-contained") Might need a cc:stable, depending on the answers to the above.
On 12/04/2023 02:24, Andrew Morton wrote: > On Tue, 11 Apr 2023 23:39:46 +0200 Petr Vorel <pvorel@suse.cz> wrote: > >>> Hi Kevin, >>>> typeof is (still) a GNU extension, which means that it cannot be >>>> used when building ISO C (e.g. -std=c99). It should therefore be >>>> avoided in uapi headers in favour of the ISO-friendly __typeof__. >>> IMHO UAPI are built with -std=c90 -Wall -Werror=implicit-function-declaration >>> (see usr/include/Makefile). >>> But one or the other, you're right both require __typeof__. >>> "If you are writing a header file that must work when included in ISO C >>> programs, write __typeof__ instead of typeof." >>> https://gcc.gnu.org/onlinedocs/gcc-12.2.0/gcc/Typeof.html >>> Reviewed-by: Petr Vorel <pvorel@suse.cz> >>> Tested-by: Petr Vorel <pvorel@suse.cz> >> IMHO problem was introduced when -std=c90 was added (back then the code was in >> include/uapi/linux/kernel.h). > Well... what actually _is_ the problem? Presumably build issues under > some circumstances. Could we please see an instance of those issues > and a description of the circumstances under which they occur? This matters from a userspace perspective, not a kernel one. uapi headers and their contents are expected to be usable in a variety of situations, and in particular when building ISO C applications (with -std=c99 or similar). This particular problem can be reproduced by trying to use the __ALIGN_KERNEL macro directly in application code, say: #include <linux/const.h> int align(int x, int a) { return __KERNEL_ALIGN(x, a); } and trying to build that with -std=c99. I do not believe this patch really fixes any other patch: __ALIGN_KERNEL() has always used typeof, so it has never been ISO-friendly. d6fc9fcbaa65 ("kbuild: compile-test exported headers to ensure they are self-contained") could not have spotted this issue, because the macro is never expanded when simply including uapi headers. One could consider that 607ca46e97a1 ("UAPI: (Scripted) Disintegrate include/linux") is the culprit because it moved the macro to include/uapi as-is, but that's arguable. > >> Fixes: d6fc9fcbaa65 ("kbuild: compile-test exported headers to ensure they are self-contained") > Might need a cc:stable, depending on the answers to the above. Considering this issue has always been present, I do not believe it is the case. Thanks, Kevin IMPORTANT NOTICE: The contents of this email and any attachments are confidential and may also be privileged. If you are not the intended recipient, please notify the sender immediately and do not disclose the contents to any other person, use it for any purpose, or store or copy the information in any medium. Thank you.
diff --git a/include/uapi/linux/const.h b/include/uapi/linux/const.h index af2a44c08683..a429381e7ca5 100644 --- a/include/uapi/linux/const.h +++ b/include/uapi/linux/const.h @@ -28,7 +28,7 @@ #define _BITUL(x) (_UL(1) << (x)) #define _BITULL(x) (_ULL(1) << (x)) -#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (typeof(x))(a) - 1) +#define __ALIGN_KERNEL(x, a) __ALIGN_KERNEL_MASK(x, (__typeof__(x))(a) - 1) #define __ALIGN_KERNEL_MASK(x, mask) (((x) + (mask)) & ~(mask)) #define __KERNEL_DIV_ROUND_UP(n, d) (((n) + (d) - 1) / (d))