[llvm-commits] [PATCH][Support] Add Endian.h which contains generic code to deal with endian specific data.

[Michael Spencer]

On Wed, Oct 20, 2010 at 3:24 PM, Rafael Espindola <espindola at google.com> wrote:
> On 14 September 2010 17:53, Michael Spencer <bigcheesegs at gmail.com> wrote:
>> The attached patch adds Endian.h to Support. This patch is a
>> prerequisite for the object file library I am writing.
>>
>> Endian.h contains:
>
> Should SwapByteOrder be in here?

No, because it contains heavily platform specific code, while this
provides utilities.

>> * SwapByteOrderIfDifferent
>> This is a templated function that takes a value_type, and returns the
>> value if host and target are the same, or returns the value with its
>> endianness swapped if they are different.
>> (host and target are int's because MSVC doesn't support enums used
>> like this. see:
>> http://stackoverflow.com/questions/2763836/sfinae-failing-with-enum-template-parameter)
>
> OK
>
>> * endian
>> This is a templated struct for reading endian specific data directly
>> from memory. It is typedefed based on the host platform's endianness
>> (well, not yet, because there's no LLVM_IS_BIG_ENDIAN or such yet).
>
> Why do you always ready data unaligned? The aligned case is probably
> the common one, no?

Because it's faster to do an unaligned read than check for alignment
first. The unaligned case is very common for COFF, but non existent
for ELF, which is why ELF uses the aligned variant. *

* I just realized that the aligned version of
packed_endian_specific_integral still ends up doing a potentially
unaligned read. While safe, this should be changed to do a direct
read, but still byteswap.

> No sure if you need the endian_impl indirection. Can you just move the
> definition of host_endianness up and define endian directly?

Duh, thanks. I think when I wrote it I was thinking about someone
wanting to use the other case, but that wouldn't make any sense...

> Also, is
> the definition of unaligned_access_helper portable?

It should work on every compiler LLVM works on and has been tested on
MSVC, clang, and gcc; however, it probably should be partially moved
to compiler.h.

I tried using the strict C/C++ version with bit-shifts, however, no
compiler was able to optimize it to just a load on x86. Pack produces
optimal code on x86 and arm, and I assume others.

>> * packed_endian_specific_integral
>> This is the most important thing included in this patch. This class
>> allows the portable creation of packed structs of any integral integer
>> value_type and endianness. Any struct that would be POD if it used
>> normal integral data types is still POD if it uses these.
>>
>> This is used extensively in the object file library to directly access
>> memory mapped object files. In the case when host and target
>> endianness match (and the compiler inlines), using these should
>> produce identical code to "*reinterpret_cast<value_type*>(memory)".
>
> Is it common to require unaligned access?

Yes, all of COFF, a.out and others. Not ELF. I don't know about MachO
yet. There are also other places in LLVM where this is needed.

> If not I would be somewhat
> inclined to have the clients handle it. I am worried that a "operator
> value_type()" might be a bit too convenient :-). Don't you get cases
> where it is unintentionally called? The code gets too ugly if you have
> to use an explicit get method?

I don't see where you wouldn't want it to be called. It should work
exactly as if you just had a value_type except that you can't assign
to it.

Adding an explicit get method would work, but I feel it would just
clutter the code.

>> - Michael Spencer
>
> Cheers,
> --
> Rafael Ávila de Espíndola

Thanks for the review.

- Michael Spencer