SST Engine with MarshalMethod BP5: Loses content of vector<string>-type attributes

[franzpoeschel]

Describe the bug
When using the MarshalMethod BP5 in the SST engine, the content of attributes containing vectors of strings are lost. The attribute can be inquired by the reader, but the vector is empty.

To Reproduce
Writer:

#include <adios2.h>
#include <numeric>
#include <vector>

int main(int argsc, char **argsv)
{
    std::string engine_type = "sst";

    adios2::ADIOS adios{};
    adios2::IO IO = adios.DeclareIO("IO");
    IO.SetParameter("DataTransport", "WAN");
    IO.SetParameter("MarshalMethod", "bp5");
    IO.SetEngine(engine_type);
    adios2::Engine engine = IO.Open("stream", adios2::Mode::Write);
    std::vector<int> v(10, 17);
    auto var = IO.DefineVariable<int>("var", {10}, {0}, {10});
    std::vector<std::string> vecstring{"x", "y", "z"};
    IO.DefineAttribute("vecstring", vecstring.data(), vecstring.size());

    for (unsigned step = 0; step < 10; ++step)
    {
        engine.BeginStep();
        engine.Put(var, v.data());
        engine.EndStep();
    }
    engine.Close();
}

Reader:

#include <adios2.h>
#include <iostream>
#include <string>
#include <vector>

int main(int argsc, char **argsv)
{
    std::string engine_type = "sst";

    adios2::ADIOS adios{};
    adios2::IO IO = adios.DeclareIO("IO");
    IO.SetParameter("DataTransport", "WAN");
    IO.SetParameter("MarshalMethod", "bp5");
    IO.SetEngine(engine_type);
    adios2::Engine engine = IO.Open("stream", adios2::Mode::Read);

    std::vector<int> v(10);

    while (engine.BeginStep() == adios2::StepStatus::OK)
    {
        auto var = IO.InquireVariable<int>("var");
        var.SetSelection({{0}, {10}});
        engine.Get(var, v.data());
        engine.EndStep();
        std::cout << "In Step " << engine.CurrentStep() << ": ";
        for(auto val : v)
        {
            std::cout << val << ", ";
        }
        std::cout << std::endl;
        auto attr = IO.InquireAttribute<std::string>("vecstring");
        std::cout << "Data in vecstring: ";
        for (auto const & str : attr.Data())
        {
            std::cout << str << ", ";
        }
        std::cout << std::endl;
    }
    engine.Close();
}

cmake_minimum_required(VERSION 3.12.0)

project(adios_testing)

find_package(ADIOS2 REQUIRED)

add_executable(stream_write stream_write.cpp)
add_executable(stream_read stream_read.cpp)

target_link_libraries(stream_write PRIVATE adios2::cxx11)
target_link_libraries(stream_read PRIVATE adios2::cxx11)

Running this:

> ./stream_write & ./stream_read & wait
[1] 751972
[2] 751973
In Step 0: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: , 
In Step 1: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: , 
In Step 2: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: , 
In Step 3: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: , 
In Step 4: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: , 
In Step 5: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: , 
In Step 6: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: , 
In Step 7: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: , 
In Step 8: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: , 
In Step 9: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: , 
[1]-  Done                    ./stream_write
[2]+  Done                    ./stream_read

Expected behavior
Same as when using the default MarshalMethod:

> ./stream_write & ./stream_read & wait
[1] 752977
[2] 752978
In Step 0: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: x, y, z, 
In Step 1: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: x, y, z, 
In Step 2: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: x, y, z, 
In Step 3: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: x, y, z, 
In Step 4: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: x, y, z, 
In Step 5: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: x, y, z, 
In Step 6: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: x, y, z, 
In Step 7: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: x, y, z, 
In Step 8: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: x, y, z, 
In Step 9: 17, 17, 17, 17, 17, 17, 17, 17, 17, 17, 
Data in vecstring: x, y, z, 
[1]-  Done                    ./stream_write
[2]+  Done                    ./stream_read

Desktop (please complete the following information):

• OS/Platform: Ubuntu 22.04.1, Intel(R) Xeon(R) W-2175 CPU
• Build: g++-11 (Ubuntu 11.3.0-1ubuntu1~22.04) 11.3.0, most recent master branch (bae863b), but behavior already observed for a while

Additional context
@pnorbert This reproduces the issue mentioned yesterday

Following up
Was the issue fixed? Please report back.

[eisenhauer]

Ah, nevermind, I have an explanation. If you add IO.SetParameter("UseOneTimeAttributes", "true") to your writer code, I think you'll find that the problem goes away. The original BP5 approach to attributes was borrowed from the FFS marshaling method, which was adapted directly from the approach to variables. But attributes and variables don't really have the same properties WRT how they appear in metadata, so it was always more of a "until we do this better" solution. The "OneTimeAttributes" approach is a reconsidered solution. It's used by default in BP5 files, but we hadn't changed the default in SST. Given that the old approach doesn't properly implement arrays of strings, it's probably time to change the default over to the new technique.

[franzpoeschel]

Hey Greg,
with this parameter, the writer actually dies with a segmentation fault for me:

> valgrind ./stream_write
==947025== Memcheck, a memory error detector
==947025== Copyright (C) 2002-2017, and GNU GPL'd, by Julian Seward et al.
==947025== Using Valgrind-3.18.1 and LibVEX; rerun with -h for copyright info
==947025== Command: ./stream_write
==947025== 
==947025== Invalid read of size 8
==947025==    at 0x5434AE4: adios2::format::BP5Serializer::OnetimeMarshalAttribute(char const*, adios2::DataType, unsigned long, void const*) (in /home/franzpoeschel/singularity_build/local/lib/libadios2_core.so.2.8.3)
==947025==    by 0x551157E: adios2::core::engine::SstWriter::NotifyEngineAttribute(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, adios2::core::AttributeBase*, void*) (in /home/franzpoeschel/singularity_build/local/lib/libadios2_core.so.2.8.3)
==947025==    by 0x50D5362: adios2::core::Attribute<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >& adios2::core::IO::DefineAttribute<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*, unsigned long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool) (in /home/franzpoeschel/singularity_build/local/lib/libadios2_core.so.2.8.3)
==947025==    by 0x4922DC1: adios2::Attribute<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > adios2::IO::DefineAttribute<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*, unsigned long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool) (in /home/franzpoeschel/singularity_build/local/lib/libadios2_cxx11.so.2.8.3)
==947025==    by 0x10AE66: main (in /home/franzpoeschel/src/adios_testing/build/stream_write)
==947025==  Address 0x80 is not stack'd, malloc'd or (recently) free'd
==947025== 
==947025== 
==947025== Process terminating with default action of signal 11 (SIGSEGV)
==947025==    at 0x4CDEA7C: __pthread_kill_implementation (pthread_kill.c:44)
==947025==    by 0x4CDEA7C: __pthread_kill_internal (pthread_kill.c:78)
==947025==    by 0x4CDEA7C: pthread_kill@@GLIBC_2.34 (pthread_kill.c:89)
==947025==    by 0x4C8A475: raise (raise.c:26)
==947025==    by 0x4C8A51F: ??? (in /usr/lib/x86_64-linux-gnu/libc.so.6)
==947025==    by 0x5434AE3: adios2::format::BP5Serializer::OnetimeMarshalAttribute(char const*, adios2::DataType, unsigned long, void const*) (in /home/franzpoeschel/singularity_build/local/lib/libadios2_core.so.2.8.3)
==947025==    by 0x551157E: adios2::core::engine::SstWriter::NotifyEngineAttribute(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, adios2::core::AttributeBase*, void*) (in /home/franzpoeschel/singularity_build/local/lib/libadios2_core.so.2.8.3)
==947025==    by 0x50D5362: adios2::core::Attribute<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >& adios2::core::IO::DefineAttribute<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*, unsigned long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool) (in /home/franzpoeschel/singularity_build/local/lib/libadios2_core.so.2.8.3)
==947025==    by 0x4922DC1: adios2::Attribute<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > > adios2::IO::DefineAttribute<std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > >(std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const*, unsigned long, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> > const&, std::__cxx11::basic_string<char, std::char_traits<char>, std::allocator<char> >, bool) (in /home/franzpoeschel/singularity_build/local/lib/libadios2_cxx11.so.2.8.3)
==947025==    by 0x10AE66: main (in /home/franzpoeschel/src/adios_testing/build/stream_write)

[eisenhauer]

Hmm. I wasn't working with your simple examples, but instead uncommented the "array of strings" attribute portion of TestCommon{Write/Read}Attrs. Not sure why the smaller case is different, but I'm not sure it matters. The deal here is that the older way of handling attributes in BP5 (which apparently doesn't handle arrays of strings) creates a record containing all attributes anytime one is added or changes. This is handy in SST, because if a reader arrives in the middle of a stream we want to give him all the active attributes and we only have to send him the most recent record to do that. But it's terrible if there are a lot of attributes and they change a lot. In the "OneTimeAttr" approach, each attribute is only encoded when it is defined or updated. This is more efficient, because we only encode changes, but it makes things more awkward for SST to support new readers getting all the previously-defined attributes. Thus we had left SST on the old mechanism until that could be reworked. So, to fix this we either have to implement array of strings in the old way, or fixup SST to work with attribute diffs rather than complete attribute blocks. The former might be quicker, but the latter is likely more forward-looking.