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

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.

> [1] https://github.com/unikraft/lib-newlib/blob/master/include/endian.h
> [2] https://clang.llvm.org/docs/LanguageExtensions.html
[3] https://github.com/razvand/snippets/tree/master/tests/bswap
[4] https://github.com/unikraft/lib-newlib
[5] https://github.com/unikraft/lib-newlib/blob/master/Makefile.uk#L53
[6] https://github.com/unikraft/lib-newlib/blob/master/include/endian.h#L44
[7]
-----
diff --git a/include/endian.h b/include/endian.h
index 3c8a752..01476bc 100644
--- a/include/endian.h
+++ b/include/endian.h
@@ -41,6 +41,11 @@

 #include <stdint.h>

+#ifdef __GNUC__
+#define        __bswap16(_x)   __builtin_bswap16(_x)
+#define        __bswap32(_x)   __builtin_bswap32(_x)
+#define        __bswap64(_x)   __builtin_bswap64(_x)
+#else /* __GNUC__ */
 static inline uint16_t __bswap16(uint16_t __x)
  {
          return __x<<8 | __x>>8;
          @@ -55,6 +60,7 @@ static inline uint64_t __bswap64(uint64_t
          __x)
           {
                   return (__bswap32(__x)+0ULL)<<32 |
                   __bswap32(__x>>32);
                    }
                    +#endif /* !__GNUC__ */

 #if __BYTE_ORDER == __LITTLE_ENDIAN
  #define htobe16(x) __bswap16(x)~
-----

Razvan