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>]

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