Derecho-Project / derecho

The main code repository for the Derecho project.
BSD 3-Clause "New" or "Revised" License
187 stars 47 forks source link

Inconsistent usage of context_ptr<const T> in SerializationSupport.hpp #204

Closed etremel closed 2 years ago

etremel commented 3 years ago

While debugging issue #203, Weijia and I noticed that the various from_bytes_noalloc functions declared in SerializationSupport.hpp don't seem to have a consistent scheme for how their third parameter is defined. Most from_bytes_noalloc functions have a third parameter whose type is context_ptr<T> and which always has a default value of context_ptr<T>{}, which is why you only need 2 arguments to call from_bytes_noalloc. For example, the function for ByteRepresentable types is defined like this:

https://github.com/Derecho-Project/derecho/blob/32715312a9f80f67bd83d85e91c5e8af241cb285/include/derecho/mutils-serialization/SerializationSupport.hpp#L386-L393

However, in some cases, there is a second version of the from_bytes_noalloc function where the third parameter is of type context_ptr<const T>, like this one for ByteRepresentable types:

https://github.com/Derecho-Project/derecho/blob/32715312a9f80f67bd83d85e91c5e8af241cb285/include/derecho/mutils-serialization/SerializationSupport.hpp#L394-L405

It's hard to tell what this means because it's used inconsistently. Based on the two from_bytes_noalloc functions defined for ByteRepresentables, it seems like the context_ptr<const T> parameter is for the version where the byte buffer is const (note the parameter char const * const v) while the context_ptr<T> parameter is for the version where the byte buffer is non-const. However, the from_bytes_noalloc functions defined for POD types do not use the parameter like this:

https://github.com/Derecho-Project/derecho/blob/32715312a9f80f67bd83d85e91c5e8af241cb285/include/derecho/mutils-serialization/SerializationSupport.hpp#L413-L420

Here, the function that has a const byte buffer parameter (char const * const v) has a third parameter whose type is context_ptr<T>, not context_ptr<const T>. However, it has a return type of context_ptr<const T>, while the "const function" for ByteRepresentables (with a context_ptr<const T> parameter) has a return type of context_ptr<T>. Meanwhile, the function that has a non-const byte buffer parameter has no third parameter at all.

Furthermore, all of the from_bytes_noalloc functions for STL types have both a return type and third parameter of type context_ptr<T>, while requiring a byte buffer parameter that is const, like this:

https://github.com/Derecho-Project/derecho/blob/32715312a9f80f67bd83d85e91c5e8af241cb285/include/derecho/mutils-serialization/SerializationSupport.hpp#L654-L657

They have no version that allows a non-const byte buffer parameter, and no version that returns context_ptr<const T>.

I can't figure out what the original intention was for these unused third parameters, but I think the generic functions for ByteRepresentable and POD types should both work the same way -- we don't want client code to have to use from_bytes_noalloc differently depending on whether the type is POD or not. Also, if the "standard" from_bytes_noalloc has two versions, one const and one non-const, then I think the from_bytes_noalloc we provide for the STL types should have the same two versions.

etremel commented 2 years ago

Update: It turns out this inconsistency is definitely some kind of bug, because it means that deserialize_and_run cannot be called on a function whose arguments include a std::vector (or other STL container). The following simple test does not compile, even though it seems to be a reasonable usage of deserialize_and_run:

int main(int argc, char** argv) {
    std::vector<int> vector_data(256);
    std::iota(vector_data.begin(), vector_data.end(), 1);
    int64_t int_data = 6666666;
    std::size_t buf_size = mutils::bytes_size(int_data) + mutils::bytes_size(vector_data);
    uint8_t* byte_buffer = new uint8_t[buf_size];
    std::size_t buf_offset = 0;
    mutils::to_bytes(int_data, byte_buffer + buf_offset);
    buf_offset += mutils::bytes_size(int_data);
    mutils::to_bytes(vector_data, byte_buffer + buf_offset);
    buf_offset += mutils::bytes_size(vector_data);
    uint8_t const* const read_only_buf_ptr = byte_buffer;
    auto callback_fun = [](int64_t int_arg, const std::vector<int>& vector_arg) {
        std::cout << "Got callback with integer " << int_arg << " and vector: " << std::endl;
        for(std::size_t i = 0; i < vector_arg.size(); ++i) {
            std::cout << vector_arg[i] << " ";
        }
        std::cout << std::endl;
    };
    mutils::deserialize_and_run(nullptr, read_only_buf_ptr, callback_fun);
    delete[] byte_buffer;
}

Specifically, the compiler fails on the deserialize_and_run line with a message that starts with

error: no matching function for call to ‘from_bytes_noalloc<const std::vector<int, std::allocator<int> > >(mutils::DeserializationManager*&, const uint8_t* const&, mutils::context_ptr<const std::vector<int> >)’

and continues with many pages of failed template instantiations. I'll try to explain what I think is going on.

deserialize_and_run accepts a function and a buffer and attempts to deserialize the function's arguments from the buffer by calling from_bytes_noalloc_v, and from_bytes_noalloc_v has two versions, one with a context_ptr<T> parameter and one with a context_ptr<const T> parameter (just like from_bytes_noalloc). However, deserialize_and_run specifically calls the version of from_bytes_noalloc_v that accepts a context_ptr<const T> parameter:

https://github.com/Derecho-Project/derecho/blob/cf71375ba4e8dfeec8fd730dc3cd1141403338dc/include/derecho/mutils-serialization/SerializationSupport.hpp#L1182-L1190

In turn, this version of from_bytes_noalloc_v will specifically call the version of from_bytes_noalloc that has a return type and third parameter of type context_ptr<const T>:

https://github.com/Derecho-Project/derecho/blob/cf71375ba4e8dfeec8fd730dc3cd1141403338dc/include/derecho/mutils-serialization/SerializationSupport.hpp#L1124-L1132

Note that from_bytes_noalloc_v recursively unpacks its template parameter pack, which represents the argument types of the function passed to deserialize_and_run, so from_bytes_noalloc will be called in this way on each argument type. This means that if the function has an argument of type std::vector<int>, it will try to call from_bytes_noalloc<const std::vector<int>>(dsm, buf, context_ptr<const std::vector<int>>{}). But, as I discovered last July in my first post on this topic, SerializationSupport.hpp does not define a from_bytes_noalloc for std::vector that has a context_ptr<const T> argument; it only has one version and it expects a context_ptr<T>:

https://github.com/Derecho-Project/derecho/blob/cf71375ba4e8dfeec8fd730dc3cd1141403338dc/include/derecho/mutils-serialization/SerializationSupport.hpp#L1037-L1042

In order for deserialize_and_run to work on functions that have std::vector arguments, SerializationSupport.hpp will need to define a version of from_bytes_noalloc for std::vector that matches the context_ptr<const T> signature.

This bug is currently preventing me from writing a notification handler for signature messages, since signatures are passed as std::vector<uint8_t>, so I'm going to try to fix it.