Hi Fangrui,
Sorry for the late answer.
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.
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.
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.
Thanks,
--
Jean Delvare
SUSE L3 Support
