Alignment issues in serializers

[codeandroid]

We began using Bond in a project compiled with emscripten. [1]

Running deserializers for CompactBinary, FastBinary and SimpleJson always resulted in an "alignment fault" when executing the resulting asm.js code.

The following exemplary piece of code demonstrates the issue and a possible fix. (There are other similar pieces of code here and there.)

It is located in cpp/inc/bond/stream/input_buffer.h:

template <typename T>
void Read(T& value)
{
    if (sizeof(T) > _blob.length() - _pointer)
    {
        EofException(sizeof(T));
    }
    
    // ORIGINAL
    //value = *reinterpret_cast<const T*>(_blob.content() + _pointer);

    // FIX
    std::memcpy(&value, _blob.content() + _pointer, sizeof(T));

    _pointer += sizeof(T);
}

Emscripten is a platform requiring loads and stores to be aligned (like MIPS, PowerPC, etc. but not like x86 which allows unaligned access though not without impact).

Also, I think, the original code violates the C++ strict aliasing rules...

Using std::memcpy should be safe and most compilers should detect the use of std::memcpy and generate properly optimized code.

I do not understand this part of C++ deep enough to be a 100% sure that this is the right approach as I'm developing for x86 and x86_64 where unaligned access just works. Otherwise I would've prepared a PR already. :)

I also tested SimpleJson (when the others failed) and also ran into alignment issues but this time most likely due to RapidJson itself. A fix for RapidJson ( Tencent/rapidjson#353 ) has been merged into master and should be included in the next release ( http://rapidjson.org/md__c_h_a_n_g_e_l_o_g.html ).

->

If you could provide some guidance on how to deal with this issue, whether that's the right direction or whether there's a different solution... that'd be much appreciated.

[1] This is part of an experimental web app sharing code and data between our desktop applications and existing services talking Bond.Comm.

[chwarr]

std::memcpy is the C++ standards compliant way to do these sort of type punning conversions. We've made similar fixes elsewhere in the code when we've found them. In particular, I in commit bc4f7d8 I made a very similar change in random_protocol.h.

std::memcpy can only be used on trivially copyable items, so we'd need to static assert or constrain the Read overload. Unfortunately, std::is_trivially_copyable is only available in C++11, and we still support C++03. I couldn't find a Boost equivalent for this. (Most implementations use a compiler trait, so it is hard to support this on compilers with out such a trait.) We may be able to use the same std::is_arithmetic assertion in these cases like I did in random_protocol.h.

We'll need to do some research to check whether std::memcpy transparently handles alignment or not.

[codeandroid]

I should've searched for existing memcpy occurrences.

Please, let me know if I can be of help.

[jasonzio]

Consider using boost::has_trivial_copy as a proxy in C++03 and std::is_trivially_copyable in C++11 and beyond. It's not perfect, but it gets you mostly there.

[chwarr]

is_arithmetic || is_enum looks to be enough for Read. Though, I broke some other things in my attempt to fix this and haven't had a chance to figure out what yet... :-)