On Mon, Oct 9, 2023 at 4:08 AM Jean Delvare <jdelv...@suse.de> wrote:
>
> Hi Fangrui,
>
> Sorry for the late answer.
Hi Jean, sorry for the late reply.
> On Sun, 24 Sep 2023 10:39:26 -0700, Fangrui Song wrote:
> > Sorry, the patch did not consider big-endian systems.
> > The attached patch has fixed it (__builtin_bswap{16,32}) and the `void
> > *` warnings (should use `const void *`).
>
> The warnings are indeed gone, and I think the code should work fine on
> bigendian systems (unfortunately I can't test that easily at the
> moment). However I have 2 concerns left with your patch:
>
> 1* You detect endianness automatically, based on __BYTE_ORDER__.
> However there's still one place where the explicitly-set BIGENDIAN is
> used. This is inconsistent. I would prefer that you just use BIGENDIAN
> for now, and in a separate patch, add auto-detection code to set
> BIGENDIAN where needed. That way, the code stays consistent, and
> BIGENDIAN can still be set manually if the auto-detection code fails
> for whatever reason.
>
> Which brings a side question: how portable are __BYTE_ORDER__ and
> __ORDER_BIG_ENDIAN__? We would have to check for existence before using
> them if they may not be defined on all systems.
__BYTE_ORDER__/__ORDER_BIG_ENDIAN__ have great portability as they are probably
the most popular way to detect endianness.
They are supported by at least GCC, Clang, and tinycc.
Linux and popular *BSD (FreeBSD, NetBSD, OpenBSD, etc) use either GCC or Clang.
> 2* How portable are __builtin_bswap16 and __builtin_bswap32? Please
> keep in mind that dmidecode is meant to be portable across operating
> systems, therefore it should not depend on glibc- or gcc-specific
> functions. All I can find in the Linux manual pages is bswap in section
> 3, which documents functions bswap_16() and bswap_32(), so different
> from what you used, and describes them as "GNU extensions" so most
> probably not portable.
GCC and Clang support __builtin_bswap16/__builtin_bswap32.
Since big-endian systems are less common today, so
u32 ret;
memcpy(&ret, x, sizeof(ret));
#if __BYTE_ORDER__ == __ORDER_BIG_ENDIAN__
ret = __builtin_bswap32(ret);
#endif
should mostly be fine. Do you know what C compilers dmidecode intends
to support?
If a C compiler doesn't support __builtin_bswap{16,32}, indeed it
could be problematic on a big-endian system,
but I am curious whether such a supported surface is hypothetical.
> As a side note, I wonder if we could now get rid of the U64 quirks I
> implemented long ago and use uint64_t instead. If it is portable, that
> would be much better than my custom code.
I agree. `rg '\.[lh] ' *.c` lists quite a few occurrences. Such a
change should be separate.
> Thanks,
> --
> Jean Delvare
> SUSE L3 Support
--
宋方睿
