| Message ID | 20200923100220.674903-5-philmd@redhat.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | qemu/bswap: Use compiler __builtin_bswap() | expand |
On Wed, 23 Sep 2020 at 11:09, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: > > All our supported compilers provide the __builtin_bswap() > functions. Drop the <byteswap.h> dependency. > > Suggested-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h > index 6875e48d617..c192a6ad3e7 100644 > --- a/include/qemu/bswap.h > +++ b/include/qemu/bswap.h > @@ -10,23 +10,6 @@ > # include <sys/endian.h> > #elif defined(__HAIKU__) > # include <endian.h> > -#elif defined(CONFIG_BYTESWAP_H) > -# include <byteswap.h> > - > -static inline uint16_t bswap16(uint16_t x) > -{ > - return bswap_16(x); > -} > - > -static inline uint32_t bswap32(uint32_t x) > -{ > - return bswap_32(x); > -} > - > -static inline uint64_t bswap64(uint64_t x) > -{ > - return bswap_64(x); > -} > #else > # define bswap16(_x) __builtin_bswap16(_x) > # define bswap32(_x) __builtin_bswap32(_x) I suppose if we wanted to just use the __builtin_bswap* implementation on all hosts (ie drop the special casing of CONFIG_MACHINE_BSWAP_H/FreeBSD/Haiku) we'd need to rename our macros to avoid potential conflicts with the versions in the system headers there in case they were pulled in via some other path ? thanks -- PMM
On 9/23/20 12:16 PM, Peter Maydell wrote: > On Wed, 23 Sep 2020 at 11:09, Philippe Mathieu-Daudé <philmd@redhat.com> wrote: >> >> All our supported compilers provide the __builtin_bswap() >> functions. Drop the <byteswap.h> dependency. >> >> Suggested-by: Richard Henderson <richard.henderson@linaro.org> >> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> >> diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h >> index 6875e48d617..c192a6ad3e7 100644 >> --- a/include/qemu/bswap.h >> +++ b/include/qemu/bswap.h >> @@ -10,23 +10,6 @@ >> # include <sys/endian.h> >> #elif defined(__HAIKU__) >> # include <endian.h> >> -#elif defined(CONFIG_BYTESWAP_H) >> -# include <byteswap.h> >> - >> -static inline uint16_t bswap16(uint16_t x) >> -{ >> - return bswap_16(x); >> -} >> - >> -static inline uint32_t bswap32(uint32_t x) >> -{ >> - return bswap_32(x); >> -} >> - >> -static inline uint64_t bswap64(uint64_t x) >> -{ >> - return bswap_64(x); >> -} >> #else >> # define bswap16(_x) __builtin_bswap16(_x) >> # define bswap32(_x) __builtin_bswap32(_x) > > I suppose if we wanted to just use the __builtin_bswap* > implementation on all hosts (ie drop the special casing > of CONFIG_MACHINE_BSWAP_H/FreeBSD/Haiku) we'd need to > rename our macros to avoid potential conflicts with the > versions in the system headers there in case they were > pulled in via some other path ? Yes, this is why I couldn't get ride of everything After reading Paolo's suggestion to use qatomic*, I am tempted to suggest qbswap* but I am still looking for better alternatives... > > thanks > -- PMM >
On 9/23/20 3:02 AM, Philippe Mathieu-Daudé wrote: > All our supported compilers provide the __builtin_bswap() > functions. Drop the <byteswap.h> dependency. > > Suggested-by: Richard Henderson <richard.henderson@linaro.org> > Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> > --- > configure | 13 ------------- > include/qemu/bswap.h | 17 ----------------- > 2 files changed, 30 deletions(-) Reviewed-by: Richard Henderson <richard.henderson@linaro.org> r~
On 9/23/20 3:30 AM, Philippe Mathieu-Daudé wrote: > On 9/23/20 12:16 PM, Peter Maydell wrote: >> I suppose if we wanted to just use the __builtin_bswap* >> implementation on all hosts (ie drop the special casing >> of CONFIG_MACHINE_BSWAP_H/FreeBSD/Haiku) we'd need to >> rename our macros to avoid potential conflicts with the >> versions in the system headers there in case they were >> pulled in via some other path ? > > Yes, this is why I couldn't get ride of everything > > After reading Paolo's suggestion to use qatomic*, I > am tempted to suggest qbswap* but I am still looking > for better alternatives... Hum, maybe. It's pretty ugly. We could just leave those alone and hope the system version is decent enough. r~
On Thu, 24 Sep 2020 at 20:30, Richard Henderson <richard.henderson@linaro.org> wrote: > > On 9/23/20 3:30 AM, Philippe Mathieu-Daudé wrote: > > On 9/23/20 12:16 PM, Peter Maydell wrote: > >> I suppose if we wanted to just use the __builtin_bswap* > >> implementation on all hosts (ie drop the special casing > >> of CONFIG_MACHINE_BSWAP_H/FreeBSD/Haiku) we'd need to > >> rename our macros to avoid potential conflicts with the > >> versions in the system headers there in case they were > >> pulled in via some other path ? > > > > Yes, this is why I couldn't get ride of everything > > > > After reading Paolo's suggestion to use qatomic*, I > > am tempted to suggest qbswap* but I am still looking > > for better alternatives... > > Hum, maybe. It's pretty ugly. We could just leave those alone and hope the > system version is decent enough. Mostly what I would like is to be able to be rid of the configure machinery so we could just use a single portable implementation. thanks -- PMM
On 9/24/20 9:33 PM, Peter Maydell wrote: > On Thu, 24 Sep 2020 at 20:30, Richard Henderson > <richard.henderson@linaro.org> wrote: >> >> On 9/23/20 3:30 AM, Philippe Mathieu-Daudé wrote: >>> On 9/23/20 12:16 PM, Peter Maydell wrote: >>>> I suppose if we wanted to just use the __builtin_bswap* >>>> implementation on all hosts (ie drop the special casing >>>> of CONFIG_MACHINE_BSWAP_H/FreeBSD/Haiku) we'd need to >>>> rename our macros to avoid potential conflicts with the >>>> versions in the system headers there in case they were >>>> pulled in via some other path ? >>> >>> Yes, this is why I couldn't get ride of everything >>> >>> After reading Paolo's suggestion to use qatomic*, I >>> am tempted to suggest qbswap* but I am still looking >>> for better alternatives... >> >> Hum, maybe. It's pretty ugly. We could just leave those alone and hope the >> system version is decent enough. > > Mostly what I would like is to be able to be rid of the > configure machinery so we could just use a single > portable implementation. Good news. Using the Haiku image provided by Alexander, GCC is pretty recent and supports builtin atomic: > uname -a Haiku shredder 1 hrev54154+115 Jun 30 2020 15:20 x86_64 x86_64 Haiku > gcc --version gcc (2019_05_24) 8.3.0 Copyright (C) 2018 Free Software Foundation, Inc. The <endian.h> include (/system/develop/headers/posix/endian.h) only defines BYTE_ORDER. Looking at FreeBSD, bswap*() are defined as macro: https://github.com/freebsd/freebsd/blob/master/sys/sys/endian.h#L58 So we could eventually undefine them... but the manpage describe them as functions [*]: DESCRIPTION The bswap16(), bswap32(), and bswap64() functions return a byte order swapped integer. So theoretically they could change the prototype still respecting the documentation contract, and undefines wouldn't work anymore. Would this be acceptable? Any clever idea? [*] https://www.freebsd.org/cgi/man.cgi?query=bswap32&apropos=0&sektion=9 > > thanks > -- PMM >
On 9/25/20 2:22 AM, Philippe Mathieu-Daudé wrote: > Looking at FreeBSD, bswap*() are defined as macro: > https://github.com/freebsd/freebsd/blob/master/sys/sys/endian.h#L58 That works for me. > So theoretically they could change the prototype still respecting > the documentation contract, and undefines wouldn't work anymore. But we'd be #defining our own macros that would mean that the out-of-line functions would never be used. So it still works. r~
diff --git a/configure b/configure index e8e8e984f24..bff787daea7 100755 --- a/configure +++ b/configure @@ -4886,16 +4886,6 @@ if test "$docs" != "no" ; then fi fi -# Search for bswap_32 function -byteswap_h=no -cat > $TMPC << EOF -#include <byteswap.h> -int main(void) { return bswap_32(0); } -EOF -if compile_prog "" "" ; then - byteswap_h=yes -fi - # Search for bswap32 function bswap_h=no cat > $TMPC << EOF @@ -6789,9 +6779,6 @@ fi if test "$st_atim" = "yes" ; then echo "HAVE_STRUCT_STAT_ST_ATIM=y" >> $config_host_mak fi -if test "$byteswap_h" = "yes" ; then - echo "CONFIG_BYTESWAP_H=y" >> $config_host_mak -fi if test "$bswap_h" = "yes" ; then echo "CONFIG_MACHINE_BSWAP_H=y" >> $config_host_mak fi diff --git a/include/qemu/bswap.h b/include/qemu/bswap.h index 6875e48d617..c192a6ad3e7 100644 --- a/include/qemu/bswap.h +++ b/include/qemu/bswap.h @@ -10,23 +10,6 @@ # include <sys/endian.h> #elif defined(__HAIKU__) # include <endian.h> -#elif defined(CONFIG_BYTESWAP_H) -# include <byteswap.h> - -static inline uint16_t bswap16(uint16_t x) -{ - return bswap_16(x); -} - -static inline uint32_t bswap32(uint32_t x) -{ - return bswap_32(x); -} - -static inline uint64_t bswap64(uint64_t x) -{ - return bswap_64(x); -} #else # define bswap16(_x) __builtin_bswap16(_x) # define bswap32(_x) __builtin_bswap32(_x)
All our supported compilers provide the __builtin_bswap() functions. Drop the <byteswap.h> dependency. Suggested-by: Richard Henderson <richard.henderson@linaro.org> Signed-off-by: Philippe Mathieu-Daudé <philmd@redhat.com> --- configure | 13 ------------- include/qemu/bswap.h | 17 ----------------- 2 files changed, 30 deletions(-)