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__.
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 <[email protected]>
Tested-by: Ruben Ayrapetyan <[email protected]>
Signed-off-by: Kevin Brodsky <[email protected]>
---
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))
--
2.38.1
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 <[email protected]>
Tested-by: Petr Vorel <[email protected]>
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 <[email protected]>
> Tested-by: Ruben Ayrapetyan <[email protected]>
> Signed-off-by: Kevin Brodsky <[email protected]>
> ---
> 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 <[email protected]>
> Tested-by: Petr Vorel <[email protected]>
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 <[email protected]>
> > Tested-by: Ruben Ayrapetyan <[email protected]>
> > Signed-off-by: Kevin Brodsky <[email protected]>
> > ---
> > 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 <[email protected]> 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 <[email protected]>
> > Tested-by: Petr Vorel <[email protected]>
>
> 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 <[email protected]> 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 <[email protected]>
>>> Tested-by: Petr Vorel <[email protected]>
>> 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.