Page MenuHomePhabricator

[libc] Add endianness support
ClosedPublic

Authored by gchatelet on Apr 15 2021, 8:39 AM.

Details

Summary

Add endianness detection support. This will be useful to implement memcmp.

Diff Detail

Event Timeline

gchatelet created this revision.Apr 15 2021, 8:39 AM
Herald added a project: Restricted Project. · View Herald TranscriptApr 15 2021, 8:39 AM
gchatelet requested review of this revision.Apr 15 2021, 8:39 AM

Let me know if you want this header to live somewhere else, or if you had other plans in mind.

libc/src/__support/endian.h
31

Here I decided to always define the symbol and set it to 0 or 1 but maybe it's better to leave it undefined. I fear that someone writes #ifdef LLVM_LIBC_IS_LITTLE_ENDIAN. It would always be true.

gchatelet updated this revision to Diff 337785.Apr 15 2021, 8:50 AM
  • Added missing modification to CMakeLists.txt

Let me know if you want this header to live somewhere else, or if you had other plans in mind.

The endianess support will rarely ever be updated. So, putting it in common is OK. If it were something which gets updated frequently, I would have suggested a separate library to avoid recompiles of parts which don't use endianess support.

libc/src/__support/endian.h
13

Include stdint.h as cstdint is a C++ header.

31

To avoid macro confusion, may be this:

namespace __llvm_libc {
namespace internal {

template <bool IsLittleEndian>
class Endian;

template <>
class Endian<__BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__> {
 public:
  static uint8_t toBigEndian(uint8_t v) { return v; }
  static uint16_t toBigEndian(uint16_t v) { return __builtin_bswap16(v); }
  ...
};

template <>
class Endian<__BYTE_ORDER__ != __ORDER_LITTLE_ENDIAN__> {
 public:
  static uint8_t toBigEndian(uint8_t v) { return v; }
  static uint16_t toBigEndian(uint16_t v) { return v; }
  ...
};

} // namespace internal

using Endian = internal::Endian<true>;
} // namespace __llvm_libc

You can also add toLittleEndian methods for the sake of completeness. Also, we probably need tests for this? For, we need the return type, the arg type, and the builtin function name to match. So, tests might protect us from mismatches.

gchatelet updated this revision to Diff 338079.Apr 16 2021, 6:01 AM
  • Add tests and remove macros
gchatelet updated this revision to Diff 338081.Apr 16 2021, 6:17 AM
gchatelet marked an inline comment as done.
  • Simplify tests

Thx for the suggestion, I didn't go with explicit member function as it would allow for integer promotion.
In the case of endianness we want to make sure we only accept one of uint8_t, uint16_t, uint32_t, uint64_t and no other type.
I've added the symmetric functions as you suggested.

sivachandra accepted this revision.Apr 16 2021, 1:53 PM

Thx for the suggestion, I didn't go with explicit member function as it would allow for integer promotion.
In the case of endianness we want to make sure we only accept one of uint8_t, uint16_t, uint32_t, uint64_t and no other type.
I've added the symmetric functions as you suggested.

I think we can try to be cute and make it a compile-time error to use a non-uint*_t type, but not sure if it is worth it.

This revision is now accepted and ready to land.Apr 16 2021, 1:53 PM
This revision was landed with ongoing or failed builds.Apr 16 2021, 2:35 PM
This revision was automatically updated to reflect the committed changes.