Byteorder conditional compilation problems

[David Howells]

Hi Linus,

Looking at commit 13da9e200fe4740b02cd51e07ab454627e228920, I suspect the
answer will be no, but can we please revisit the use of __BYTE_ORDER in the
kernel?


The reason for this is that I've noticed a number of places in the UAPI
headers where __LITTLE_ENDIAN and __BIG_ENDIAN are assumed to act as in the
main kernel: they are tested for definedness, which is incorrect in UAPI
headers unless guarded by __KERNEL__.  In UAPI headers, they _must_ be
compared to __BYTE_ORDER in !__KERNEL__ conditions and <endian.h> should
perhaps be #included as well.

The following headers are all incorrect in their usage in this manner:

	include/uapi/linux/aio_abi.h - definition of PADDED()
	include/uapi/linux/acct.h - definition of ACCT_BYTEORDER
	include/uapi/linux/raid/md_p.h - order of members in mdp_superblock_s
	arch/m32r/include/uapi/asm/stat.h - order of members in stat64

The last two are the most worrying as these are visible from userspace.  Both
cases will likely use the __BIG_ENDIAN version in userspace (depending on
header inclusion ordering) - and it wouldn't surprise me if there's a nasty
bug in the RAID API that no one has spotted yet.  M32R will likely have a
problem too if anyone uses that in LE mode (the default is "no" on that one,
though).

And then there are:

	arch/arc/include/uapi/asm/byteorder.h - uses CONFIG_CPU_BIG_ENDIAN
	arch/c6x/include/uapi/asm/byteorder.h - uses _BIG_ENDIAN
	arch/c6x/include/uapi/asm/ptrace.h - uses _BIG_ENDIAN
	arch/mips/include/uapi/asm/msgbuf.h - uses CONFIG_CPU_LITTLE_ENDIAN

which all look like they might be wrong.

And finally, there are:

	include/{uapi/,}linux/patchkey.h - macro _PATCHKEY
	include/{uapi/,}linux/soundcard.h - macro AFMT_S16_NE

which have two separate bits (one UAPI, one KAPI) defining the same macro in
the same way because the macro is endianness-dependent, but the logic to
select the variant cannot be shared between UAPI and KAPI.

The following:

	include/uapi/sound/asound.h
	include/uapi/sound/sfnt_info.h

seem to dodge the problem by using SNDRV_{LITTLE,BIG}_ENDIAN instead - though
this doesn't seem to be defined in any headers under /usr/include, so I
presume it is required to be set manually by anyone using those headers.


Whilst the following opinion:

	The kernel has never had that crazy "__BYTE_ORDER == __LITTLE_ENDIAN"
	model.  It's not how we do things, and it isn't how we _should_ do
	things.  So don't go there.

may be a valid, it is incompatible with userspace.  We cannot change how
userspace works, but we could change how the kernel works to match it, thus
reducing the confusion.


Looking at commit b3b77c8caef1750ebeea1054e39e358550ea9f55, I think the
problems with it are simple to resolve:

 (1) linux/byteorder/big_endian.h didn't define __LITTLE_ENDIAN and
     .../little_endian.h didn't define __BIG_ENDIAN.  These should be placed
     in a common header, and only __BYTE_ORDER defined in the separate
     headers.

 (2) Tests for the definedness of __BIG_ENDIAN and __LITTLE_ENDIAN must be
     replaced en mass with comparisons against __BYTE_ORDER.

Sadly, (1) and (2) would have to be done in the same commit to avoid problems
since __BIG_ENDIAN and __LITTLE_ENDIAN must both be defined to for (1):-/


Throughout the whole kernel:

 - There are 703 instances of "defined(__XXX_ENDIAN)" scattered over 68
   files.  None of these appear to be in comments.

 - There are 6 instances of "def __XXX_ENDIAN[^_]" scattered over 4 files and
   these are all in comments.

 - There are a lot of "__XXX_ENDIAN__" - but these are not a problem.
   Possibly these should be converted to "__XXX_ENDIAN" and the relevant -D
   preprocessor flags removed.

 - There are a lot of "__XXX_ENDIAN_BITFIELD" - which aren't a problem either.


Scripting a change from "defined(__XXX_ENDIAN)" to "(__XXX_ENDIAN==__BYTEORDER)"
should be easy to script.

David
--
To unsubscribe from this list: send the line "unsubscribe linux-kernel" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Please read the FAQ at  http://www.tux.org/lkml/