| Message ID | B69CCAC6-FB47-4B2B-95F9-15F1BA4AD381@goldelico.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
ping. Am 03.10.2015 um 22:46 schrieb H. Nikolaus Schaller <hns@goldelico.com>: > If the host toolchain is not glibc based then the arm kernel build > fails with > > HOSTCC arch/arm/vdso/vdsomunge > arch/arm/vdso/vdsomunge.c:48:22: fatal error: byteswap.h: No such file or directory > > Observed: with omap2plus_defconfig and compile on Mac OS X with arm ELF > cross-compiler. > > Reason: byteswap.h is a glibc only header. > > Solution: replace by private byte-swapping macros (taken from > arch/mips/boot/elf2ecoff.c) > > Tested to compile on Mac OS X 10.9.5 host. > > Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> > --- > arch/arm/vdso/vdsomunge.c | 19 +++++++++++++++---- > 1 file changed, 15 insertions(+), 4 deletions(-) > > diff --git a/arch/arm/vdso/vdsomunge.c b/arch/arm/vdso/vdsomunge.c > index aedec81..27a9a0b 100644 > --- a/arch/arm/vdso/vdsomunge.c > +++ b/arch/arm/vdso/vdsomunge.c > @@ -45,7 +45,18 @@ > * it does. > */ > > -#include <byteswap.h> > +#define swab16(x) \ > + ((unsigned short)( \ > + (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \ > + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) )) > + > +#define swab32(x) \ > + ((unsigned int)( \ > + (((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) | \ > + (((unsigned int)(x) & (unsigned int)0x0000ff00UL) << 8) | \ > + (((unsigned int)(x) & (unsigned int)0x00ff0000UL) >> 8) | \ > + (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24) )) > + > #include <elf.h> > #include <errno.h> > #include <fcntl.h> > @@ -104,17 +115,17 @@ static void cleanup(void) > > static Elf32_Word read_elf_word(Elf32_Word word, bool swap) > { > - return swap ? bswap_32(word) : word; > + return swap ? swab32(word) : word; > } > > static Elf32_Half read_elf_half(Elf32_Half half, bool swap) > { > - return swap ? bswap_16(half) : half; > + return swap ? swab16(half) : half; > } > > static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap) > { > - *dst = swap ? bswap_32(val) : val; > + *dst = swap ? swab32(val) : val; > } > > int main(int argc, char **argv) > -- > 2.5.1 >
On 10/14/2015 07:47 AM, H. Nikolaus Schaller wrote: >> diff --git a/arch/arm/vdso/vdsomunge.c b/arch/arm/vdso/vdsomunge.c >> index aedec81..27a9a0b 100644 >> --- a/arch/arm/vdso/vdsomunge.c >> +++ b/arch/arm/vdso/vdsomunge.c >> @@ -45,7 +45,18 @@ >> * it does. >> */ >> >> -#include <byteswap.h> >> +#define swab16(x) \ >> + ((unsigned short)( \ >> + (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \ >> + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) )) >> + >> +#define swab32(x) \ >> + ((unsigned int)( \ >> + (((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) | \ >> + (((unsigned int)(x) & (unsigned int)0x0000ff00UL) << 8) | \ >> + (((unsigned int)(x) & (unsigned int)0x00ff0000UL) >> 8) | \ >> + (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24) )) >> + >> #include <elf.h> >> #include <errno.h> >> #include <fcntl.h> >> @@ -104,17 +115,17 @@ static void cleanup(void) >> >> static Elf32_Word read_elf_word(Elf32_Word word, bool swap) >> { >> - return swap ? bswap_32(word) : word; >> + return swap ? swab32(word) : word; >> } >> >> static Elf32_Half read_elf_half(Elf32_Half half, bool swap) >> { >> - return swap ? bswap_16(half) : half; >> + return swap ? swab16(half) : half; >> } >> >> static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap) >> { >> - *dst = swap ? bswap_32(val) : val; >> + *dst = swap ? swab32(val) : val; >> } > ping. Sorry for the delay. This is okay but I'd prefer the swab macros to be below the #include lines, and it would be easier for me to deal with a patch that isn't whitespace-damaged.
Am 14.10.2015 um 16:16 schrieb Nathan Lynch <nathan_lynch@mentor.com>: > On 10/14/2015 07:47 AM, H. Nikolaus Schaller wrote: >>> diff --git a/arch/arm/vdso/vdsomunge.c b/arch/arm/vdso/vdsomunge.c >>> index aedec81..27a9a0b 100644 >>> --- a/arch/arm/vdso/vdsomunge.c >>> +++ b/arch/arm/vdso/vdsomunge.c >>> @@ -45,7 +45,18 @@ >>> * it does. >>> */ >>> >>> -#include <byteswap.h> >>> +#define swab16(x) \ >>> + ((unsigned short)( \ >>> + (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \ >>> + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) )) >>> + >>> +#define swab32(x) \ >>> + ((unsigned int)( \ >>> + (((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) | \ >>> + (((unsigned int)(x) & (unsigned int)0x0000ff00UL) << 8) | \ >>> + (((unsigned int)(x) & (unsigned int)0x00ff0000UL) >> 8) | \ >>> + (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24) )) >>> + >>> #include <elf.h> >>> #include <errno.h> >>> #include <fcntl.h> >>> @@ -104,17 +115,17 @@ static void cleanup(void) >>> >>> static Elf32_Word read_elf_word(Elf32_Word word, bool swap) >>> { >>> - return swap ? bswap_32(word) : word; >>> + return swap ? swab32(word) : word; >>> } >>> >>> static Elf32_Half read_elf_half(Elf32_Half half, bool swap) >>> { >>> - return swap ? bswap_16(half) : half; >>> + return swap ? swab16(half) : half; >>> } >>> >>> static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap) >>> { >>> - *dst = swap ? bswap_32(val) : val; >>> + *dst = swap ? swab32(val) : val; >>> } >> ping. > > Sorry for the delay. > > This is okay but I'd prefer the swab macros to be below the #include > lines, Ok. > and it would be easier for me to deal with a patch that isn't > whitespace-damaged. You mean: ERROR: space prohibited before that close parenthesis ')' #46: FILE: arch/arm/vdso/vdsomunge.c:64: + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) )) ERROR: space prohibited before that close parenthesis ')' #53: FILE: arch/arm/vdso/vdsomunge.c:71: + (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24) )) Well, I had copied that verbatim from arch/mips/boot/elf2ecoff.c and thought that it is better readable, but it is easy to fix. V3 is coming. BR and thanks, Nikolaus
On Thu, Oct 15, 2015 at 07:52:38AM +0200, H. Nikolaus Schaller wrote: > ERROR: space prohibited before that close parenthesis ')' > #46: FILE: arch/arm/vdso/vdsomunge.c:64: > + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) )) I have a pet hatred of too many parens. I also have a hatred of idiotic casts. What about changing the above to one of: (((x) & 0xff00) >> 8) (((x) >> 8) & 0xff) > > ERROR: space prohibited before that close parenthesis ')' > #53: FILE: arch/arm/vdso/vdsomunge.c:71: > + (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24) )) and one of: (((x) & 0xff000000) >> 24) (((x) >> 24) & 0xff) ?
Am 15.10.2015 um 18:37 schrieb Russell King - ARM Linux <linux@arm.linux.org.uk>: > On Thu, Oct 15, 2015 at 07:52:38AM +0200, H. Nikolaus Schaller wrote: >> ERROR: space prohibited before that close parenthesis ')' >> #46: FILE: arch/arm/vdso/vdsomunge.c:64: >> + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) )) > > I have a pet hatred of too many parens. I also have a hatred of idiotic > casts. What about changing the above to one of: > > (((x) & 0xff00) >> 8) > (((x) >> 8) & 0xff) > >> >> ERROR: space prohibited before that close parenthesis ')' >> #53: FILE: arch/arm/vdso/vdsomunge.c:71: >> + (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24) )) > > and one of: > > (((x) & 0xff000000) >> 24) > (((x) >> 24) & 0xff) > > ? Well, the semantics is sometimes the same but I think readability goes down because the macro becomes quite un-symmetric and for >> 8 you should better be sure that values are unsigned if combining with | operator. Of course it is questionable why the constants (with U and UL modifiers) are casted. The full macro (as copied verbatim from existing arch/mips/boot/elf2ecoff.c) is: +#define swab16(x) \ + ((unsigned short)( \ + (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \ + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8))) + +#define swab32(x) \ + ((unsigned int)( \ + (((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) | \ + (((unsigned int)(x) & (unsigned int)0x0000ff00UL) << 8) | \ + (((unsigned int)(x) & (unsigned int)0x00ff0000UL) >> 8) | \ + (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24))) + I have already submitted a V3 of this patch, so please comment that. A question is why this is not generally available for all HOSTCC based tools so that we could simply include some header. But since that is no kernel code but a tool, I think it is not really necessary to put significant efforts into this piece of code. This was probably the intention of the original code to simply include byteswap.h - then you don't even see how complex the macros are - but it is not a standard header. BR and thanks, Nikolaus Schaller
On 10/15/2015 12:52 AM, H. Nikolaus Schaller wrote: > Am 14.10.2015 um 16:16 schrieb Nathan Lynch <nathan_lynch@mentor.com>: >> and it would be easier for me to deal with a patch that isn't >> whitespace-damaged. > > You mean: > > ERROR: space prohibited before that close parenthesis ')' > #46: FILE: arch/arm/vdso/vdsomunge.c:64: > + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) )) > > ERROR: space prohibited before that close parenthesis ')' > #53: FILE: arch/arm/vdso/vdsomunge.c:71: > + (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24) )) > > Well, I had copied that verbatim from arch/mips/boot/elf2ecoff.c and > thought that it is better readable, but it is easy to fix. That's not what I was referring to, but it is fine to fix that too. What I meant was that the first column of the patch body is corrupted. Your v2 has hunks like this (bad): static Elf32_Word read_elf_word(Elf32_Word word, bool swap) { - return swap ? bswap_32(word) : word; + return swap ? swab32(word) : word; } Your v3 has this (good): static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap) { - *dst = swap ? bswap_32(val) : val; + *dst = swap ? swab32(val) : val; }
Am 15.10.2015 um 19:07 schrieb Nathan Lynch <Nathan_Lynch@mentor.com>: > On 10/15/2015 12:52 AM, H. Nikolaus Schaller wrote: >> Am 14.10.2015 um 16:16 schrieb Nathan Lynch <nathan_lynch@mentor.com>: >>> and it would be easier for me to deal with a patch that isn't >>> whitespace-damaged. >> >> You mean: >> >> ERROR: space prohibited before that close parenthesis ')' >> #46: FILE: arch/arm/vdso/vdsomunge.c:64: >> + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) )) >> >> ERROR: space prohibited before that close parenthesis ')' >> #53: FILE: arch/arm/vdso/vdsomunge.c:71: >> + (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24) )) >> >> Well, I had copied that verbatim from arch/mips/boot/elf2ecoff.c and >> thought that it is better readable, but it is easy to fix. > > That's not what I was referring to, but it is fine to fix that too. > > What I meant was that the first column of the patch body is corrupted. > Your v2 has hunks like this (bad): > > static Elf32_Word read_elf_word(Elf32_Word word, bool swap) > { > - return swap ? bswap_32(word) : word; > + return swap ? swab32(word) : word; > } > > > Your v3 has this (good): > > static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap) > { > - *dst = swap ? bswap_32(val) : val; > + *dst = swap ? swab32(val) : val; > } Does this mean it is ok now in V3 in all cases? I have switched my patch editor tool from indirect git imap-send to git send-email (which appears to be more compatible). BR and thanks, Nikolaus Schaller
On 10/15/2015 12:16 PM, H. Nikolaus Schaller wrote: > Does this mean it is ok now in V3 in all cases? I have switched my patch editor > tool from indirect git imap-send to git send-email (which appears to be more > compatible). v3 applied without issue, so yes, in that respect it is fine.
diff --git a/arch/arm/vdso/vdsomunge.c b/arch/arm/vdso/vdsomunge.c index aedec81..27a9a0b 100644 --- a/arch/arm/vdso/vdsomunge.c +++ b/arch/arm/vdso/vdsomunge.c @@ -45,7 +45,18 @@ * it does. */ -#include <byteswap.h> +#define swab16(x) \ + ((unsigned short)( \ + (((unsigned short)(x) & (unsigned short)0x00ffU) << 8) | \ + (((unsigned short)(x) & (unsigned short)0xff00U) >> 8) )) + +#define swab32(x) \ + ((unsigned int)( \ + (((unsigned int)(x) & (unsigned int)0x000000ffUL) << 24) | \ + (((unsigned int)(x) & (unsigned int)0x0000ff00UL) << 8) | \ + (((unsigned int)(x) & (unsigned int)0x00ff0000UL) >> 8) | \ + (((unsigned int)(x) & (unsigned int)0xff000000UL) >> 24) )) + #include <elf.h> #include <errno.h> #include <fcntl.h> @@ -104,17 +115,17 @@ static void cleanup(void) static Elf32_Word read_elf_word(Elf32_Word word, bool swap) { - return swap ? bswap_32(word) : word; + return swap ? swab32(word) : word; } static Elf32_Half read_elf_half(Elf32_Half half, bool swap) { - return swap ? bswap_16(half) : half; + return swap ? swab16(half) : half; } static void write_elf_word(Elf32_Word val, Elf32_Word *dst, bool swap) { - *dst = swap ? bswap_32(val) : val; + *dst = swap ? swab32(val) : val; } int main(int argc, char **argv)
If the host toolchain is not glibc based then the arm kernel build fails with HOSTCC arch/arm/vdso/vdsomunge arch/arm/vdso/vdsomunge.c:48:22: fatal error: byteswap.h: No such file or directory Observed: with omap2plus_defconfig and compile on Mac OS X with arm ELF cross-compiler. Reason: byteswap.h is a glibc only header. Solution: replace by private byte-swapping macros (taken from arch/mips/boot/elf2ecoff.c) Tested to compile on Mac OS X 10.9.5 host. Signed-off-by: H. Nikolaus Schaller <hns@goldelico.com> --- arch/arm/vdso/vdsomunge.c | 19 +++++++++++++++---- 1 file changed, 15 insertions(+), 4 deletions(-)