| Message ID | 1390768248-1688-1-git-send-email-ard.biesheuvel@linaro.org (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
Hi Ard, On Sun, Jan 26, 2014 at 08:30:48PM +0000, Ard Biesheuvel wrote: > In a way similar to ARM commit 09096f6a0ee2 ("ARM: 7822/1: add workaround > for ambiguous C99 stdint.h types"), this patch redefines the macros that > are used in stdint.h so its definitions of uint64_t and int64_t are > compatible with those of the kernel. > > In order to do so, drop types.h from generic-y and create a specific arm64 > version identical to the generic one with just the #define overrides added. I tried but still can't get what this patch is about. Do the linux/types.h types ever get to user space? We have uapi/linux/types.h for this. Can you give an example of where this is needed? Which source file includes both stdint.h and linux/types.h (non-uapi version)?
On 17 February 2014 13:23, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Sun, Jan 26, 2014 at 08:30:48PM +0000, Ard Biesheuvel wrote: >> In a way similar to ARM commit 09096f6a0ee2 ("ARM: 7822/1: add workaround >> for ambiguous C99 stdint.h types"), this patch redefines the macros that >> are used in stdint.h so its definitions of uint64_t and int64_t are >> compatible with those of the kernel. >> >> In order to do so, drop types.h from generic-y and create a specific arm64 >> version identical to the generic one with just the #define overrides added. > > I tried but still can't get what this patch is about. Do the > linux/types.h types ever get to user space? We have uapi/linux/types.h > for this. > > Can you give an example of where this is needed? Which source file > includes both stdint.h and linux/types.h (non-uapi version)? > It's not about user space, it is mainly about the use of NEON instrinsics in the kernel. If you do the following: #Include <linux/types.h> #include <arm_neon.h> (and build with -ffreestanding or you will get /another/ error) you will get the following error: /usr/lib/gcc-cross/aarch64-linux-gnu/4.8/include/stdint-gcc.h:43:24: error: conflicting types for 'int64_t' typedef __INT64_TYPE__ int64_t; ^ In file included from /home/ard/linux-2.6/drivers/crypto/neon/aes-async-modes.c:11:0: /home/ard/linux-2.6/include/linux/types.h:113:17: note: previous declaration of 'int64_t' was here typedef __s64 int64_t; ^ In file included from /usr/lib/gcc-cross/aarch64-linux-gnu/4.8/include/stdint.h:11:0, from /usr/lib/gcc-cross/aarch64-linux-gnu/4.8/include/arm_neon.h:30, This is caused by the fact that GCC and the kernel don't agree on the definition of int64_t and uint64_t. (long vs long long) Fortunately, GCC's definition is not built in but based on the builtin #define __INT64_TYPE, which we can override if we want to. That is what the patch does. Regards, Ard.
On Mon, Feb 17, 2014 at 12:40:18PM +0000, Ard Biesheuvel wrote: > On 17 February 2014 13:23, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Sun, Jan 26, 2014 at 08:30:48PM +0000, Ard Biesheuvel wrote: > >> In a way similar to ARM commit 09096f6a0ee2 ("ARM: 7822/1: add workaround > >> for ambiguous C99 stdint.h types"), this patch redefines the macros that > >> are used in stdint.h so its definitions of uint64_t and int64_t are > >> compatible with those of the kernel. > >> > >> In order to do so, drop types.h from generic-y and create a specific arm64 > >> version identical to the generic one with just the #define overrides added. > > > > I tried but still can't get what this patch is about. Do the > > linux/types.h types ever get to user space? We have uapi/linux/types.h > > for this. > > > > Can you give an example of where this is needed? Which source file > > includes both stdint.h and linux/types.h (non-uapi version)? > > It's not about user space, it is mainly about the use of NEON > instrinsics in the kernel. > > If you do the following: > > #Include <linux/types.h> > #include <arm_neon.h> For other intrinsics that we use like __builtin_ctzl(), do we need to explicitly include gcc headers? I don't think we do and I really don't like such arm_neon.h include which brings in other user headers. Don't we have any work around this? My inbox only has some discussion in May last year on the linaro-kernel list without any clear conclusion (it could be that I deleted other emails).
On 17 February 2014 18:42, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Mon, Feb 17, 2014 at 12:40:18PM +0000, Ard Biesheuvel wrote: >> On 17 February 2014 13:23, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > On Sun, Jan 26, 2014 at 08:30:48PM +0000, Ard Biesheuvel wrote: >> >> In a way similar to ARM commit 09096f6a0ee2 ("ARM: 7822/1: add workaround >> >> for ambiguous C99 stdint.h types"), this patch redefines the macros that >> >> are used in stdint.h so its definitions of uint64_t and int64_t are >> >> compatible with those of the kernel. >> >> >> >> In order to do so, drop types.h from generic-y and create a specific arm64 >> >> version identical to the generic one with just the #define overrides added. >> > >> > I tried but still can't get what this patch is about. Do the >> > linux/types.h types ever get to user space? We have uapi/linux/types.h >> > for this. >> > >> > Can you give an example of where this is needed? Which source file >> > includes both stdint.h and linux/types.h (non-uapi version)? >> >> It's not about user space, it is mainly about the use of NEON >> instrinsics in the kernel. >> >> If you do the following: >> >> #Include <linux/types.h> >> #include <arm_neon.h> > > For other intrinsics that we use like __builtin_ctzl(), do we need to > explicitly include gcc headers? I don't think we do and I really don't > like such arm_neon.h include which brings in other user headers. Don't > we have any work around this? > Well, I talked to the toolchain guys at the time and they really disliked the idea of coding directly against the __builtins because they are not considered a stable interface, especially because the interface that /is/ considered stable (arm_neon.h) is supported both on ARM and on arm64. > My inbox only has some discussion in May last year on the linaro-kernel > list without any clear conclusion (it could be that I deleted other > emails). > There was some discussion, indeed, but for ARM, with the conclusion being the fix I mentioned in the patch: 09096f6a0ee2 ("ARM: 7822/1: add workaround for ambiguous C99 stdint.h types"), only in that case, the ambiguity is (unsurprisingly) about the 32 bit types, not the 64 bit ones. It all comes down to whether you are interested in supporting NEON intrinsics (not __builtins, but intrinsics that require a support header). If you do, this is probably the easiest way to do so, i.e., merge this patch and document the fact that the NEON intrinsics object files need to be built with -ffreestanding (as is the case for ARM). Only then can you freely #include <arm_neon.h> in code that also (indirectly) includes <linux/types.h>. Regards, Ard.
On Mon, Feb 17, 2014 at 05:57:22PM +0000, Ard Biesheuvel wrote: > On 17 February 2014 18:42, Catalin Marinas <catalin.marinas@arm.com> wrote: > > On Mon, Feb 17, 2014 at 12:40:18PM +0000, Ard Biesheuvel wrote: > >> On 17 February 2014 13:23, Catalin Marinas <catalin.marinas@arm.com> wrote: > >> > On Sun, Jan 26, 2014 at 08:30:48PM +0000, Ard Biesheuvel wrote: > >> >> In a way similar to ARM commit 09096f6a0ee2 ("ARM: 7822/1: add workaround > >> >> for ambiguous C99 stdint.h types"), this patch redefines the macros that > >> >> are used in stdint.h so its definitions of uint64_t and int64_t are > >> >> compatible with those of the kernel. > >> >> > >> >> In order to do so, drop types.h from generic-y and create a specific arm64 > >> >> version identical to the generic one with just the #define overrides added. > >> > > >> > I tried but still can't get what this patch is about. Do the > >> > linux/types.h types ever get to user space? We have uapi/linux/types.h > >> > for this. > >> > > >> > Can you give an example of where this is needed? Which source file > >> > includes both stdint.h and linux/types.h (non-uapi version)? > >> > >> It's not about user space, it is mainly about the use of NEON > >> instrinsics in the kernel. > >> > >> If you do the following: > >> > >> #Include <linux/types.h> > >> #include <arm_neon.h> > > > > For other intrinsics that we use like __builtin_ctzl(), do we need to > > explicitly include gcc headers? I don't think we do and I really don't > > like such arm_neon.h include which brings in other user headers. Don't > > we have any work around this? > > Well, I talked to the toolchain guys at the time and they really > disliked the idea of coding directly against the __builtins because > they are not considered a stable interface, especially because the > interface that /is/ considered stable (arm_neon.h) is supported both > on ARM and on arm64. Than we don't use the Neon __builtins in the kernel. > > My inbox only has some discussion in May last year on the linaro-kernel > > list without any clear conclusion (it could be that I deleted other > > emails). > > There was some discussion, indeed, but for ARM, with the conclusion > being the fix I mentioned in the patch: 09096f6a0ee2 ("ARM: 7822/1: > add workaround > for ambiguous C99 stdint.h types"), only in that case, the ambiguity > is (unsurprisingly) about the 32 bit types, not the 64 bit ones. My worry is that some future toolchain may include something else in this file and get other type conflicts. It really looks fragile.
On 17 February 2014 19:02, Catalin Marinas <catalin.marinas@arm.com> wrote: > On Mon, Feb 17, 2014 at 05:57:22PM +0000, Ard Biesheuvel wrote: >> On 17 February 2014 18:42, Catalin Marinas <catalin.marinas@arm.com> wrote: >> > On Mon, Feb 17, 2014 at 12:40:18PM +0000, Ard Biesheuvel wrote: >> >> On 17 February 2014 13:23, Catalin Marinas <catalin.marinas@arm.com> wrote: >> >> > On Sun, Jan 26, 2014 at 08:30:48PM +0000, Ard Biesheuvel wrote: >> >> >> In a way similar to ARM commit 09096f6a0ee2 ("ARM: 7822/1: add workaround >> >> >> for ambiguous C99 stdint.h types"), this patch redefines the macros that >> >> >> are used in stdint.h so its definitions of uint64_t and int64_t are >> >> >> compatible with those of the kernel. >> >> >> >> >> >> In order to do so, drop types.h from generic-y and create a specific arm64 >> >> >> version identical to the generic one with just the #define overrides added. >> >> > >> >> > I tried but still can't get what this patch is about. Do the >> >> > linux/types.h types ever get to user space? We have uapi/linux/types.h >> >> > for this. >> >> > >> >> > Can you give an example of where this is needed? Which source file >> >> > includes both stdint.h and linux/types.h (non-uapi version)? >> >> >> >> It's not about user space, it is mainly about the use of NEON >> >> instrinsics in the kernel. >> >> >> >> If you do the following: >> >> >> >> #Include <linux/types.h> >> >> #include <arm_neon.h> >> > >> > For other intrinsics that we use like __builtin_ctzl(), do we need to >> > explicitly include gcc headers? I don't think we do and I really don't >> > like such arm_neon.h include which brings in other user headers. Don't >> > we have any work around this? >> >> Well, I talked to the toolchain guys at the time and they really >> disliked the idea of coding directly against the __builtins because >> they are not considered a stable interface, especially because the >> interface that /is/ considered stable (arm_neon.h) is supported both >> on ARM and on arm64. > > Than we don't use the Neon __builtins in the kernel. > >> > My inbox only has some discussion in May last year on the linaro-kernel >> > list without any clear conclusion (it could be that I deleted other >> > emails). >> >> There was some discussion, indeed, but for ARM, with the conclusion >> being the fix I mentioned in the patch: 09096f6a0ee2 ("ARM: 7822/1: >> add workaround >> for ambiguous C99 stdint.h types"), only in that case, the ambiguity >> is (unsurprisingly) about the 32 bit types, not the 64 bit ones. > > My worry is that some future toolchain may include something else in > this file and get other type conflicts. It really looks fragile. > Well, the GCC folks are quite careful not to depend on arbitrary user headers when the -ffreestanding option is set. Also, the real problem is the fact that Linux defines C99 types, but does so in an incompatible way. (I.e., one could also argue that the Linux typedefs should be based on GCC's builtin #defines of __INT64_TYPE, __UINT64_TYPE, etc if defined). So the chances of something similar reappearing all of a sudden are quite slim imo. Regards, Ard.
diff --git a/arch/arm64/include/asm/Kbuild b/arch/arm64/include/asm/Kbuild index d0ff25de67ca..93e0653e8c65 100644 --- a/arch/arm64/include/asm/Kbuild +++ b/arch/arm64/include/asm/Kbuild @@ -44,7 +44,6 @@ generic-y += termbits.h generic-y += termios.h generic-y += topology.h generic-y += trace_clock.h -generic-y += types.h generic-y += unaligned.h generic-y += user.h generic-y += vga.h diff --git a/arch/arm64/include/asm/types.h b/arch/arm64/include/asm/types.h new file mode 100644 index 000000000000..6519296da003 --- /dev/null +++ b/arch/arm64/include/asm/types.h @@ -0,0 +1,26 @@ +#ifndef __ASM_TYPES_H +#define __ASM_TYPES_H + +#include <asm-generic/int-ll64.h> + +/* + * For Aarch64, there is some ambiguity in the definition of the types below + * between the kernel and GCC itself. This is usually not a big deal, but it + * causes trouble when including GCC's version of 'stdint.h' (this is the file + * that gets included when you #include <stdint.h> on a -ffreestanding build). + * As this file also gets included implicitly when including 'arm_neon.h' (the + * NEON intrinsics support header), we need the following to work around the + * issue if we want to use NEON intrinsics in the kernel. + */ + +#ifdef __INT64_TYPE__ +#undef __INT64_TYPE__ +#define __INT64_TYPE__ __signed__ long long +#endif + +#ifdef __UINT64_TYPE__ +#undef __UINT64_TYPE__ +#define __UINT64_TYPE__ unsigned long long +#endif + +#endif /* __ASM_TYPES_H */
In a way similar to ARM commit 09096f6a0ee2 ("ARM: 7822/1: add workaround for ambiguous C99 stdint.h types"), this patch redefines the macros that are used in stdint.h so its definitions of uint64_t and int64_t are compatible with those of the kernel. In order to do so, drop types.h from generic-y and create a specific arm64 version identical to the generic one with just the #define overrides added. Signed-off-by: Ard Biesheuvel <ard.biesheuvel@linaro.org> --- arch/arm64/include/asm/Kbuild | 1 - arch/arm64/include/asm/types.h | 26 ++++++++++++++++++++++++++ 2 files changed, 26 insertions(+), 1 deletion(-) create mode 100644 arch/arm64/include/asm/types.h