fix cast-align error

[neheb]

Non x86 platforms are more strict about alignment and thus throw a Wcast-align warning on these two reinterpret_casts. Use memcpy to avoid.

[mbunkus]

Eh, neither the old nor the current version works on Big Endian systems. Not that there are many of those around, but still, we should write portable code.

The EbmlCrc32 element always stores data in Little Endian format. Therefore a Big Endian processor cannot just memcpy Little Endian data into a memory region & then access it as an integer.

The correct code to use would take this into account. For example (untested):

  auto buf = reinterpret_cast<const std::uint8_t *>(input);
  std::uint32_t tmp{};
  for (int idx = 3; idx >= 0; --idx)
    tmp = (tmp << 8) | buf[idx];
  crc ^= tmp;

[neheb]

That looks like it would still throw a cast-align error.

big endian systems are available on cfarm.

[mbunkus]

No, it wouldn't. Alignment only matters when accessing structures bigger than a single byte in one instruction, e.g. reading a 32 bit integer. Accessing single bytes one by one is always OK and never a problem (though with performance penalties, obviously).

[neheb]

@mbunkus without this PR on a big endian machine:

cfarm231$ ctest
Test project /home/mangix/devstuff/libebml/c
    Start 1: test_id
1/8 Test #1: test_id ..........................   Passed    0.03 sec
    Start 2: test_header
2/8 Test #2: test_header ......................   Passed    0.04 sec
    Start 3: test_infinite
3/8 Test #3: test_infinite ....................   Passed    0.03 sec
    Start 4: test_utfstring
4/8 Test #4: test_utfstring ...................   Passed    0.03 sec
    Start 5: test_macros
5/8 Test #5: test_macros ......................   Passed    0.03 sec
    Start 6: test_crc
6/8 Test #6: test_crc .........................   Passed    0.04 sec
    Start 7: test_missing
7/8 Test #7: test_missing .....................   Passed    0.03 sec
    Start 8: test_versioning
8/8 Test #8: test_versioning ..................   Passed    0.03 sec

100% tests passed, 0 tests failed out of 8

Total Test time (real) =   0.35 sec

same as with. test_crc needs work looks like.

[robUx4]

The memcpy is equivalent to the cast that was there before and is the proper way to get around such cast issues. If the code was wrong before, it's wrong now.

This is what we use in VLC and some byte swap when endianness needs to be changed. libavutil seems to have the concept of unaligned integers (that depends on compiler support).

[mbunkus]

@neheb The Crc test only tests creation & reading on the same architecture. It doesn't test creating on BE & reading on LE & vice versa. Endian issues only show up if both sides are different, or if the test compares created values against fixed, known-good values. In other words, the current test is insufficient for detecting Endian issues.

[mbunkus]

If the code was wrong before, it's wrong now.

Exactly. It was wrong before for Big Endian machines.

This is what we use in VLC and some byte swap when endianness needs to be changed.

Yes. The memcpy plus byte swapping would be OK, too. The effect will be the same as the solution I've proposed. I do not know which one is faster; we'd need real benchmarks for it (my guess would be: memcpy+swapping might be faster).

[robUx4]

Se do have SafeReadIOCallback::GetUInt32BE() but it's hidden in there and/or would be overkill. It's using the same byte shifting as you do/

[neheb]

anything to do here?

[neheb]

Looking at this further, I can't say I like this. Makes more sense to byteswap on big endian.