* [musl] Re: move __BYTE_ORDER definition to alltypes.h [not found] <CA+PODjoUKSfu1Z+b2PukoVOKOCrfzhLAkDa2VOjvNQYnd9Hc1A@mail.gmail.com> @ 2021-01-26 17:23 ` Rich Felker 2021-01-26 21:29 ` Andrey Melnikov 0 siblings, 1 reply; 6+ messages in thread From: Rich Felker @ 2021-01-26 17:23 UTC (permalink / raw) To: Andrey Melnikov; +Cc: musl On Tue, Jan 26, 2021 at 02:55:09PM +0300, Andrey Melnikov wrote: > Hi. > > Your commit 97d35a552ec5b6ddf7923dd2f9a8eb973526acea leads to > miscompile programs which rely on one of defines __LITTLE_ENDIAN or > __BIG_ENDIAN. > Now, both unconditionally defined when included stdarg.h and programs > which define __(BIG|LITTE)_ENDIAN itself - miscompiled. linux kernel > for example - it internally uses #if defined __BIG_ENDIAN and defines > it only for BIGENDAIN arches. > > Any ideas? The conditionally-defined macros that on some archs tell you the endianness are __BIG_ENDIAN__ and __LITTLE_ENDIAN__ (note the final __) or other arch-specific macros. __BIG_ENDIAN and __LITTLE_ENDIAN (without the final __) have always been the possible values for __BYTE_ORDER from endian.h. In any case, all of these are in the reserved namespace and should not be defined by applications or inspected in any way other than a manner documented by the implementation. How did this come up with the Linux kernel? AFAIK it uses -nostdinc and should not see the libc headers at all. But if that #ifdef is present in Linux it's probably a bug since it's contrary to all historical use of __BIG_ENDIAN... Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* [musl] Re: move __BYTE_ORDER definition to alltypes.h 2021-01-26 17:23 ` [musl] Re: move __BYTE_ORDER definition to alltypes.h Rich Felker @ 2021-01-26 21:29 ` Andrey Melnikov 2021-01-26 21:40 ` Rich Felker 0 siblings, 1 reply; 6+ messages in thread From: Andrey Melnikov @ 2021-01-26 21:29 UTC (permalink / raw) To: Rich Felker; +Cc: musl вт, 26 янв. 2021 г. в 20:23, Rich Felker <dalias@aerifal.cx>: > > On Tue, Jan 26, 2021 at 02:55:09PM +0300, Andrey Melnikov wrote: > > Hi. > > > > Your commit 97d35a552ec5b6ddf7923dd2f9a8eb973526acea leads to > > miscompile programs which rely on one of defines __LITTLE_ENDIAN or > > __BIG_ENDIAN. > > Now, both unconditionally defined when included stdarg.h and programs > > which define __(BIG|LITTE)_ENDIAN itself - miscompiled. linux kernel > > for example - it internally uses #if defined __BIG_ENDIAN and defines > > it only for BIGENDAIN arches. > > > > Any ideas? > > The conditionally-defined macros that on some archs tell you the > endianness are __BIG_ENDIAN__ and __LITTLE_ENDIAN__ (note the final Yes, defined by compiller. > __) or other arch-specific macros. __BIG_ENDIAN and __LITTLE_ENDIAN > (without the final __) have always been the possible values for > __BYTE_ORDER from endian.h. From <endian.h> - its correct headers for this, not from <stdarg.h>. <stdarg.h> is for va_* macros. > In any case, all of these are in the > reserved namespace and should not be defined by applications or > inspected in any way other than a manner documented by the > implementation. Where is it reserved? > How did this come up with the Linux kernel? AFAIK it uses -nostdinc > and should not see the libc headers at all. But if that #ifdef is > present in Linux it's probably a bug since it's contrary to all > historical use of __BIG_ENDIAN... Easy. Build an out-of-tree module that includes <linux/kernel.h> which is include <stdarg.h> which is define all variant _ENDIAN macros, later it include <asm/byteorder.h> which is include <linux/byteorder/little_endian.h> in my case, in <linux/byteorder/little_endian.h> (read uapi/linux/byteorder/little_endian.h) we have defines: #ifndef __LITTLE_ENDIAN #define __LITTLE_ENDIAN 1234 #endif #ifndef __LITTLE_ENDIAN_BITFIELD #define __LITTLE_ENDIAN_BITFIELD #endif and later, include <linux/etherdevice.h> in module for is_multicast_ether_addr() function: static inline bool is_multicast_ether_addr(const u8 *addr) { #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) u32 a = *(const u32 *)addr; #else u16 a = *(const u16 *)addr; #endif #ifdef __BIG_ENDIAN return 0x01 & (a >> ((sizeof(a) * 8) - 8)); #else return 0x01 & a; #endif1234 } And now, we have defined __LITTLE_ENDIAN 1234 (from kernel headers), __BIG_ENDIAN 4321 (from stdarg header) - and code for BIG endian compiled for LITTLE endian machide. Doh. Please, return (BIG|LITTLE|PDP)_ENDIAN defines back to <endian.h>. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] Re: move __BYTE_ORDER definition to alltypes.h 2021-01-26 21:29 ` Andrey Melnikov @ 2021-01-26 21:40 ` Rich Felker 2021-01-27 8:12 ` Andrey Melnikov 0 siblings, 1 reply; 6+ messages in thread From: Rich Felker @ 2021-01-26 21:40 UTC (permalink / raw) To: Andrey Melnikov; +Cc: musl On Wed, Jan 27, 2021 at 12:29:05AM +0300, Andrey Melnikov wrote: > вт, 26 янв. 2021 г. в 20:23, Rich Felker <dalias@aerifal.cx>: > > > > On Tue, Jan 26, 2021 at 02:55:09PM +0300, Andrey Melnikov wrote: > > > Hi. > > > > > > Your commit 97d35a552ec5b6ddf7923dd2f9a8eb973526acea leads to > > > miscompile programs which rely on one of defines __LITTLE_ENDIAN or > > > __BIG_ENDIAN. > > > Now, both unconditionally defined when included stdarg.h and programs > > > which define __(BIG|LITTE)_ENDIAN itself - miscompiled. linux kernel > > > for example - it internally uses #if defined __BIG_ENDIAN and defines > > > it only for BIGENDAIN arches. > > > > > > Any ideas? > > > > The conditionally-defined macros that on some archs tell you the > > endianness are __BIG_ENDIAN__ and __LITTLE_ENDIAN__ (note the final > > Yes, defined by compiller. > > > __) or other arch-specific macros. __BIG_ENDIAN and __LITTLE_ENDIAN > > (without the final __) have always been the possible values for > > __BYTE_ORDER from endian.h. > > >From <endian.h> - its correct headers for this, not from <stdarg.h>. > <stdarg.h> is for va_* macros. > > > In any case, all of these are in the > > reserved namespace and should not be defined by applications or > > inspected in any way other than a manner documented by the > > implementation. > > Where is it reserved? 7.1.3 Reserved identifiers "All identifiers that begin with an underscore and either an uppercase letter or another underscore are always reserved for any use." > > How did this come up with the Linux kernel? AFAIK it uses -nostdinc > > and should not see the libc headers at all. But if that #ifdef is > > present in Linux it's probably a bug since it's contrary to all > > historical use of __BIG_ENDIAN... > > Easy. Build an out-of-tree module that includes <linux/kernel.h> which > is include <stdarg.h> which is define all variant _ENDIAN macros, > later it include <asm/byteorder.h> which is include > <linux/byteorder/little_endian.h> in my case, > in <linux/byteorder/little_endian.h> (read > uapi/linux/byteorder/little_endian.h) we have defines: This also requires -nostdinc. The libc headers are not suitable for compiling kernel code. But in this case they should not be breaking anything. > #ifndef __LITTLE_ENDIAN > #define __LITTLE_ENDIAN 1234 > #endif > #ifndef __LITTLE_ENDIAN_BITFIELD > #define __LITTLE_ENDIAN_BITFIELD > #endif > > and later, include <linux/etherdevice.h> in module for > is_multicast_ether_addr() function: > > static inline bool is_multicast_ether_addr(const u8 *addr) > { > #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) > u32 a = *(const u32 *)addr; > #else > u16 a = *(const u16 *)addr; > #endif > #ifdef __BIG_ENDIAN > return 0x01 & (a >> ((sizeof(a) * 8) - 8)); > #else > return 0x01 & a; > #endif1234 > } This code is just wrong. It should be #if __BYTE_ORDER == __BIG_ENDIAN or #ifdef __BIG_ENDIAN__. Not #ifdef __BIG_ENDIAN. Where did the code come from? Whoever wrote it misunderstood the meanings of these macros. Even within the kernel they're not intended to be used this way. Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] Re: move __BYTE_ORDER definition to alltypes.h 2021-01-26 21:40 ` Rich Felker @ 2021-01-27 8:12 ` Andrey Melnikov 2021-01-27 15:45 ` Rich Felker 0 siblings, 1 reply; 6+ messages in thread From: Andrey Melnikov @ 2021-01-27 8:12 UTC (permalink / raw) To: Rich Felker; +Cc: musl ср, 27 янв. 2021 г. в 00:40, Rich Felker <dalias@aerifal.cx>: > > On Wed, Jan 27, 2021 at 12:29:05AM +0300, Andrey Melnikov wrote: > > вт, 26 янв. 2021 г. в 20:23, Rich Felker <dalias@aerifal.cx>: > > > > > > On Tue, Jan 26, 2021 at 02:55:09PM +0300, Andrey Melnikov wrote: > > > > Hi. > > > > > > > > Your commit 97d35a552ec5b6ddf7923dd2f9a8eb973526acea leads to > > > > miscompile programs which rely on one of defines __LITTLE_ENDIAN or > > > > __BIG_ENDIAN. > > > > Now, both unconditionally defined when included stdarg.h and programs > > > > which define __(BIG|LITTE)_ENDIAN itself - miscompiled. linux kernel > > > > for example - it internally uses #if defined __BIG_ENDIAN and defines > > > > it only for BIGENDAIN arches. > > > > > > > > Any ideas? > > > > > > The conditionally-defined macros that on some archs tell you the > > > endianness are __BIG_ENDIAN__ and __LITTLE_ENDIAN__ (note the final > > > > Yes, defined by compiller. > > > > > __) or other arch-specific macros. __BIG_ENDIAN and __LITTLE_ENDIAN > > > (without the final __) have always been the possible values for > > > __BYTE_ORDER from endian.h. > > > > >From <endian.h> - its correct headers for this, not from <stdarg.h>. > > <stdarg.h> is for va_* macros. > > > > > In any case, all of these are in the > > > reserved namespace and should not be defined by applications or > > > inspected in any way other than a manner documented by the > > > implementation. > > > > Where is it reserved? > > 7.1.3 Reserved identifiers > > "All identifiers that begin with an underscore and either an uppercase > letter or another underscore are always reserved for any use." > > > How did this come up with the Linux kernel? AFAIK it uses -nostdinc > > > and should not see the libc headers at all. But if that #ifdef is > > > present in Linux it's probably a bug since it's contrary to all > > > historical use of __BIG_ENDIAN... > > > > Easy. Build an out-of-tree module that includes <linux/kernel.h> which > > is include <stdarg.h> which is define all variant _ENDIAN macros, > > later it include <asm/byteorder.h> which is include > > <linux/byteorder/little_endian.h> in my case, > > in <linux/byteorder/little_endian.h> (read > > uapi/linux/byteorder/little_endian.h) we have defines: > > This also requires -nostdinc. The libc headers are not suitable for > compiling kernel code. But in this case they should not be breaking > anything. Kernel need va_* macros and definitions for printk(). And <stdarg.h> under glibc does not exist, it comes from gcc/llvm distribution includes. > > #ifndef __LITTLE_ENDIAN > > #define __LITTLE_ENDIAN 1234 > > #endif > > #ifndef __LITTLE_ENDIAN_BITFIELD > > #define __LITTLE_ENDIAN_BITFIELD > > #endif > > > > and later, include <linux/etherdevice.h> in module for > > is_multicast_ether_addr() function: > > > > static inline bool is_multicast_ether_addr(const u8 *addr) > > { > > #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) > > u32 a = *(const u32 *)addr; > > #else > > u16 a = *(const u16 *)addr; > > #endif > > #ifdef __BIG_ENDIAN > > return 0x01 & (a >> ((sizeof(a) * 8) - 8)); > > #else > > return 0x01 & a; > > #endif1234 > > } > > This code is just wrong. It should be #if __BYTE_ORDER == __BIG_ENDIAN This code works under glibc/musl-1.1.x and now - broken. > or #ifdef __BIG_ENDIAN__. Not #ifdef __BIG_ENDIAN. Where did the code > come from? https://github.com/torvalds/linux/blob/2ab38c17aac10bf55ab3efde4c4db3893d8691d2/include/linux/etherdevice.h#L116 > Whoever wrote it misunderstood the meanings of these macros. > Even within the kernel they're not intended to be used this way. Kernel code does not expect random defines coming from random headers. Remove #include <bits/alltypes.h> from stdarg.h - it's simply not needed here. Nothing in this header need to know types/endianness. Or remove this header completely - this compiler built-in include. ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] Re: move __BYTE_ORDER definition to alltypes.h 2021-01-27 8:12 ` Andrey Melnikov @ 2021-01-27 15:45 ` Rich Felker 2021-01-28 12:45 ` Andrey Melnikov 0 siblings, 1 reply; 6+ messages in thread From: Rich Felker @ 2021-01-27 15:45 UTC (permalink / raw) To: Andrey Melnikov; +Cc: musl On Wed, Jan 27, 2021 at 11:12:41AM +0300, Andrey Melnikov wrote: > ср, 27 янв. 2021 г. в 00:40, Rich Felker <dalias@aerifal.cx>: > > > > On Wed, Jan 27, 2021 at 12:29:05AM +0300, Andrey Melnikov wrote: > > > вт, 26 янв. 2021 г. в 20:23, Rich Felker <dalias@aerifal.cx>: > > > > > > > > On Tue, Jan 26, 2021 at 02:55:09PM +0300, Andrey Melnikov wrote: > > > > > Hi. > > > > > > > > > > Your commit 97d35a552ec5b6ddf7923dd2f9a8eb973526acea leads to > > > > > miscompile programs which rely on one of defines __LITTLE_ENDIAN or > > > > > __BIG_ENDIAN. > > > > > Now, both unconditionally defined when included stdarg.h and programs > > > > > which define __(BIG|LITTE)_ENDIAN itself - miscompiled. linux kernel > > > > > for example - it internally uses #if defined __BIG_ENDIAN and defines > > > > > it only for BIGENDAIN arches. > > > > > > > > > > Any ideas? > > > > > > > > The conditionally-defined macros that on some archs tell you the > > > > endianness are __BIG_ENDIAN__ and __LITTLE_ENDIAN__ (note the final > > > > > > Yes, defined by compiller. > > > > > > > __) or other arch-specific macros. __BIG_ENDIAN and __LITTLE_ENDIAN > > > > (without the final __) have always been the possible values for > > > > __BYTE_ORDER from endian.h. > > > > > > >From <endian.h> - its correct headers for this, not from <stdarg.h>. > > > <stdarg.h> is for va_* macros. > > > > > > > In any case, all of these are in the > > > > reserved namespace and should not be defined by applications or > > > > inspected in any way other than a manner documented by the > > > > implementation. > > > > > > Where is it reserved? > > > > 7.1.3 Reserved identifiers > > > > "All identifiers that begin with an underscore and either an uppercase > > letter or another underscore are always reserved for any use." > > > > > How did this come up with the Linux kernel? AFAIK it uses -nostdinc > > > > and should not see the libc headers at all. But if that #ifdef is > > > > present in Linux it's probably a bug since it's contrary to all > > > > historical use of __BIG_ENDIAN... > > > > > > Easy. Build an out-of-tree module that includes <linux/kernel.h> which > > > is include <stdarg.h> which is define all variant _ENDIAN macros, > > > later it include <asm/byteorder.h> which is include > > > <linux/byteorder/little_endian.h> in my case, > > > in <linux/byteorder/little_endian.h> (read > > > uapi/linux/byteorder/little_endian.h) we have defines: > > > > This also requires -nostdinc. The libc headers are not suitable for > > compiling kernel code. But in this case they should not be breaking > > anything. > > Kernel need va_* macros and definitions for printk(). And <stdarg.h> > under glibc does not exist, it comes from gcc/llvm distribution > includes. Again, you cannot use libc headers for building the kernel or kernel modules. musl is not glibc. It uses a BSD-like include order and defines all of the headers itself rather than using a mix of gcc headers and libc-provided ones. Add -nostdinc and -I the gcc header path (like the kernel's own build system does) or you will hit obscure problems like this again even if changes are made to the endian macros that make your immediate problem go away. > > > #ifndef __LITTLE_ENDIAN > > > #define __LITTLE_ENDIAN 1234 > > > #endif > > > #ifndef __LITTLE_ENDIAN_BITFIELD > > > #define __LITTLE_ENDIAN_BITFIELD > > > #endif > > > > > > and later, include <linux/etherdevice.h> in module for > > > is_multicast_ether_addr() function: > > > > > > static inline bool is_multicast_ether_addr(const u8 *addr) > > > { > > > #if defined(CONFIG_HAVE_EFFICIENT_UNALIGNED_ACCESS) > > > u32 a = *(const u32 *)addr; > > > #else > > > u16 a = *(const u16 *)addr; > > > #endif > > > #ifdef __BIG_ENDIAN > > > return 0x01 & (a >> ((sizeof(a) * 8) - 8)); > > > #else > > > return 0x01 & a; > > > #endif1234 > > > } > > > > This code is just wrong. It should be #if __BYTE_ORDER == __BIG_ENDIAN > This code works under glibc/musl-1.1.x and now - broken. This would be a regression if it were doing something violating the reserved namespace, but it's not. musl does not aim for bug-compatibility, and if any application entitled to use libc headers (as opposed to the kernel) were doing sketchy things with the __BIG_ENDIAN macro, it would be an application bug. But since it has potential for silent wrong-code generation it may be appropriate to make changes here anyway. > > or #ifdef __BIG_ENDIAN__. Not #ifdef __BIG_ENDIAN. Where did the code > > come from? > https://github.com/torvalds/linux/blob/2ab38c17aac10bf55ab3efde4c4db3893d8691d2/include/linux/etherdevice.h#L116 Thanks. A quick grep -r __BIG_ENDIAN include/linux shows a good deal of inconsistent usage of this macro (some places with #if __BYTE_ORDER == ..., some with #ifdef). It's not unconditionally wrong for them to use the macro in a different way than libc does, since from the standpoint of building the kernel, Linux is "the implementation" and is the one that owns the entire reserved namespace (apart from what it assumes the compiler might be defining). But it is wrong to mix libc headers in such a context, for this and other reasons. It's not urgent but I think the inconsistency should probably be raised with the kernel folks at some point. It's error-prone and confusing to readers who are used to both endian macros being defined as possible values for __BYTE_ORDER rather than one. > > Whoever wrote it misunderstood the meanings of these macros. > > Even within the kernel they're not intended to be used this way. > Kernel code does not expect random defines coming from random headers. > Remove #include <bits/alltypes.h> from stdarg.h - it's simply not > needed here. Nothing in this header need to know types/endianness. Or You cannot remove bits/alltypes.h or va_list will not be defined. > remove this header completely - this compiler built-in include. is not compatible with musl. What possibly can be done is removing the __BIG_ENDIAN and __LITTLE_ENDIAN macros and just using the values 1234 and 4321 explicitly. I don't see any strong reason not to do that, and it's probably what I had in an earlier draft of the relevant changes. The reason __BYTE_ORDER is defined here (alltypes.h) is that it's necessary to correctly define some of the types that alltypes.h is capable of exposing. The alltypes.h mechanism itself is used to avoid redundant definitions and guard logic against multiple definition for types which are exposed by more than one header. va_list is such a type because stdio.h, wchar.h, and syslog.h expose it too. Rich ^ permalink raw reply [flat|nested] 6+ messages in thread
* Re: [musl] Re: move __BYTE_ORDER definition to alltypes.h 2021-01-27 15:45 ` Rich Felker @ 2021-01-28 12:45 ` Andrey Melnikov 0 siblings, 0 replies; 6+ messages in thread From: Andrey Melnikov @ 2021-01-28 12:45 UTC (permalink / raw) To: Rich Felker; +Cc: musl ср, 27 янв. 2021 г. в 18:45, Rich Felker <dalias@aerifal.cx>: > > On Wed, Jan 27, 2021 at 11:12:41AM +0300, Andrey Melnikov wrote: > > ср, 27 янв. 2021 г. в 00:40, Rich Felker <dalias@aerifal.cx>: > > > > > > On Wed, Jan 27, 2021 at 12:29:05AM +0300, Andrey Melnikov wrote: > > > > вт, 26 янв. 2021 г. в 20:23, Rich Felker <dalias@aerifal.cx>: > > > > > > > > > > On Tue, Jan 26, 2021 at 02:55:09PM +0300, Andrey Melnikov wrote: > > > > > > Hi. [...] > > Again, you cannot use libc headers for building the kernel or kernel > modules. musl is not glibc. It uses a BSD-like include order and > defines all of the headers itself rather than using a mix of gcc > headers and libc-provided ones. I'm already able to build kernel, modules and other stuff after removing the installed by musl stdarg.h. > Add -nostdinc and -I the gcc header path (like the kernel's own build system does) or you will hit obscure > problems like this again even if changes are made to the endian macros that make your immediate problem go away. stdarg.h is a compiler-specific thing, and -nostdinc disables that patch too. Search, where located RIGHT stdarg.h - not trivial. For me - patching ONE library with broken design is preferred, rather than search in all other programs where authors define by itself "reserved identifier for ANY USE" and patch it. Thnx. But, if you think what linux kernel have broken design and misuse defines - you may tell kernel developers about that. ^ permalink raw reply [flat|nested] 6+ messages in thread
end of thread, other threads:[~2021-01-28 15:15 UTC | newest] Thread overview: 6+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- [not found] <CA+PODjoUKSfu1Z+b2PukoVOKOCrfzhLAkDa2VOjvNQYnd9Hc1A@mail.gmail.com> 2021-01-26 17:23 ` [musl] Re: move __BYTE_ORDER definition to alltypes.h Rich Felker 2021-01-26 21:29 ` Andrey Melnikov 2021-01-26 21:40 ` Rich Felker 2021-01-27 8:12 ` Andrey Melnikov 2021-01-27 15:45 ` Rich Felker 2021-01-28 12:45 ` Andrey Melnikov
Code repositories for project(s) associated with this public inbox https://git.vuxu.org/mirror/musl/ This is a public inbox, see mirroring instructions for how to clone and mirror all data and code used for this inbox; as well as URLs for NNTP newsgroup(s).