Re: [Minios-devel] [UNIKRAFT/NEWLIB PATCH] include/endian.h Define the __bswap16, __bswap32, __bswap64 builtin functions only if the compilation is done with gcc

[Razvan Deaconescu <razvan.deaconescu@xxxxxxxxx>]

Felipe, Simon, please see the discussion below.

Do you have any suggestions? Should I just make a decision and submit
updated patches?

Razvan

Razvan Deaconescu <razvan.deaconescu@xxxxxxxxx> writes:
> Razvan Deaconescu <razvan.deaconescu@xxxxxxxxx> writes:
>> Alice Suiu <alicesuiu17@xxxxxxxxx> writes:
>>> În joi, 2 apr. 2020 la 19:56, Simon Kuenzer <simon.kuenzer@xxxxxxxxx> a 
>>> scris:
>>>> Hi Alice,
>>>>
>>>> I have a question to this patch:
>>>>
>>>> On 01.03.20 20:13, alicesuiu wrote:
>>>> > Signed-off-by: Alice Suiu <alicesuiu17@xxxxxxxxx>
>>>> > ---
>>>> >   include/endian.h | 2 ++
>>>> >   1 file changed, 2 insertions(+)
>>>> >
>>>> > diff --git a/include/endian.h b/include/endian.h
>>>> > index 3c8a752..63d3650 100644
>>>> > --- a/include/endian.h
>>>> > +++ b/include/endian.h
>>>> > @@ -41,6 +41,7 @@
>>>> >
>>>> >   #include <stdint.h>
>>>> >
>>>> > +#ifndef __clang__
>>>> >   static inline uint16_t __bswap16(uint16_t __x)
>>>> >   {
>>>> >       return __x<<8 | __x>>8;
>>>> > @@ -55,6 +56,7 @@ static inline uint64_t __bswap64(uint64_t __x)
>>>> >   {
>>>> >       return (__bswap32(__x)+0ULL)<<32 | __bswap32(__x>>32);
>>>> >   }
>>>> > +#endif
>>>>
>>>> Doesn't GCC has a builtin for this and does clang has one, too? We also
>>>> need to make sure that clang is not introducing libc replacements as
>>>> default. On GCC that broke the printing with nolibc...
>>>
>>> Hi Simon,
>>>
>>> Yes, GCC doesn't have these builtin functions, but clang has them. When I
>>> use nolibc + clang, I don't receive errors. But when I use newlib [1] +
>>> clang [2], I receive errors because both have these builtin functions.
>>
>> Both GCC and Clang have define the `__builtin_bswapXY()` builtins. The
>> actual issue is a different behavior between the two compilers: the
>> simultaneous definition of a macro `__bswapXY` and a function
>> `__bswapXY`; GCC doesn't complain, but Clang does. See the sample
>> here[3].
>>
>> This is happening in Unikraft because the Unikraft core libraries are
>> using a minimized Unikraft newlib library[4], while the Unikraft app is
>> using the upstream (complete) newlib library[5]. In the end there are
>> two `endian.h` files: one in the Unikraft newlib[6], another one in the
>> upstream newlib (<repo>/newlib/libc/include/machine/endian.h). The
>> Unikraft newlib version of `endian.h` defines `__bswapXY` as inline
>> functions, whereas the upstream newlib defines them as macros. This
>> results in an error from Clang; GCC doesn't mind, though it's an issue
>> having multiple definitions for the same thing (even if one is a macro
>> or one is a function).
>>
>> Ideally you wouldn't have multiple headers with the same content. But
>> given the way Unikraft is created (with the core libraries not requiring
>> the upstream newlib version, only the minimal Unikraft newlib) we need
>> both of them.
>>
>> The proper way to fix this is to make the two compatible: update the
>> Unikraft newlib `endian.h` definitions using the ones from the upstream
>> newlib, as in the patch below[7]. This works for GCC and Clang.
>>
>> Simon, does this patch look OK to you? If yes, I will send it to the
>> mailing list and we can reject the current one.
>
> There's a cleaner solution, as we discussed, today: removing the
> __bswapXY() parts and including the upstream version of endian.h, as in
> the patch below.
>
> There's also the option of removing the endian.h file altogether and
> updating the calling functions. I looked into Unikraft core libs, newlib
> and lwip, and removing it works; but I don't know about other libraries
> that may be using the endian.h file. To be on the safe side, I favor the
> patch below.
>
> -----
> diff --git a/include/endian.h b/include/endian.h
> index 3c8a752..f09c312 100644
> --- a/include/endian.h
> +++ b/include/endian.h
> @@ -27,6 +27,8 @@
>  #ifndef _ENDIAN_H
>  #define _ENDIAN_H
>  
> +#include <machine/endian.h>
> +
>  #define __LITTLE_ENDIAN 1234
>  #define __BIG_ENDIAN 4321
>  #define __PDP_ENDIAN 3412
> @@ -41,21 +43,6 @@
>  
>  #include <stdint.h>
>  
> -static inline uint16_t __bswap16(uint16_t __x)
> -{
> -       return __x<<8 | __x>>8;
> -}
> -
> -static inline uint32_t __bswap32(uint32_t __x)
> -{
> -       return __x>>24 | (__x>>8&0xff00) | (__x<<8&0xff0000) | __x<<24;
> -}
> -
> -static inline uint64_t __bswap64(uint64_t __x)
> -{
> -       return (__bswap32(__x)+0ULL)<<32 | __bswap32(__x>>32);
> -}
> -
>  #if __BYTE_ORDER == __LITTLE_ENDIAN
>  #define htobe16(x) __bswap16(x)
>  #define be16toh(x) __bswap16(x)
> -----
>
> Razvan