Use <mpark/variant.hpp> indiscriminately

[franzpoeschel]

Our header include/openPMD/auxiliary/VariantSrc.hpp currently inspects the C++ version with a macro and uses either std::variant or mpark::variant depending on the found version. In a public header, that can lead to trouble if the user code uses a different C++ version compared to openPMD-api.

Bug can be reproduced with the following writer:

cmake_minimum_required(VERSION 3.14.0)
set(CMAKE_CXX_STANDARD 17)

project(openpmd_debug)

find_package(openPMD REQUIRED)

add_executable(openpmd_write openpmd_write.cpp)

target_link_libraries(openpmd_write PRIVATE openPMD::openPMD adios2::cxx11)

#include <openPMD/openPMD.hpp>

// example: data handling
#include <numeric>  // std::iota
#include <vector>   // std::vector

/**@file test example for writing histogram woth openPMD-api from c++ code
  *
  */

int main(){
    /// open
    auto series = openPMD::Series(
        "myOutput/data_%05T.bp",
        openPMD::Access::CREATE);

    /// open iteration
    auto iteration = series.iterations[42];

    constexpr int xExtent = 1;
    constexpr int yExtent = 2;
    constexpr int maxNumBins = 3;

    openPMD::Extent extent = {xExtent, yExtent, maxNumBins};

    /// create test data
    // set up is xExtent x yExtent grid points and for each grid point a histogram with maxNumBins bins
    std::vector<float> x_data(
        xExtent * yExtent * maxNumBins);

    x_data = {1., 2., 3., 4., 5., 6.}; // by convention x is fastest changing index

    // create a new record for mesh
    auto histogramMesh = iteration.meshes["adaptiveHistogram"];

    // create 1 record component each corresponding to one single entry n histogram
    auto entry = histogramMesh[openPMD::MeshRecordComponent::SCALAR];

    ///create new dataset, description of how data is to be stored
    openPMD::Datatype dataType = openPMD::determineDatatype<float>();
    openPMD::Dataset dataSet = openPMD::Dataset(dataType, extent);

    /// set what records actually store(what record components actually are)
    entry.resetDataset(dataSet);

    /// actual data is passed
    entry.storeChunk(
        x_data,
        {0, 0, 0}, // offset
        extent); // extent, artificially inflated

    /// actual write happens
    series.flush();

    // destruct series object,
    // e.g. when out-of-scope

    return 0;
}

[ax3l]

I think we might want to store the mpark-variant choice used at openPMD-api compile time in our Config.hpp and then base our chose on that.

[franzpoeschel]

I think we might want to store the mpark-variant choice used at openPMD-api compile time in our Config.hpp and then base our chose on that.

This PR is only going to be a workaround until we switch to C++17 and can use std::optional instead of mpark::optional entirely. So I'm not sure it's worth the effort for that?

[ax3l]

The problem is that our variant is part of the public API, thus it needs to be installed as well.
If we want to make this non-optional, we need to also change the CMake logic that installs mpark-variant currently only conditionally in the default super-build.

[ax3l]

Just for the record: For the reproducer above: which compiler did you use and what's the error message?

[franzpoeschel]

Just for the record: For the reproducer above: which compiler did you use and what's the error message?

I tried this with several compilers, and with different results:

• g++7.5.0, self-built: Result was a segmentation fault
• Spack environment:

  spack find --loaded
  ==> 38 installed packages
  -- linux-ubuntu20.04-skylake_avx512 / [email protected] -----------------
  [email protected]  [email protected]  [email protected]  [email protected]  [email protected]  [email protected]  [email protected]
  
  -- linux-ubuntu20.04-skylake_avx512 / [email protected] ----------------
  [email protected]    [email protected]  [email protected]        [email protected]      [email protected]  [email protected]
  [email protected]     [email protected]       [email protected]       [email protected]       [email protected]
  [email protected]  [email protected]      [email protected]            [email protected]  [email protected]
  [email protected]    [email protected]            [email protected]  [email protected]       [email protected]
  [email protected]     [email protected]         [email protected]          [email protected]      [email protected]
  [email protected]     [email protected]     [email protected]  [email protected]      [email protected]
  

  Error was:

  terminate called after throwing an instance of 'mpark::bad_variant_access'
    what():  bad_variant_access
  Aborted (core dumped)
  

My guess is that mpark::optional and std::optional have different ABI layouts, leading to wrongly interpreted values

The problem is that our variant is part of the public API, thus it needs to be installed as well.

Ok, so memorizing the chosen version with config.hpp could actually be the simpler solution

[ax3l]

I think I see the same problem on Windows with C++17 builds: ECP-WarpX/WarpX#1776 (comment)

[franzpoeschel]

Is there even a way to use configure_file for writing the used C++ standard level to config.hpp? I did not find an obvious way.

If we want to make this non-optional, we need to also change the CMake logic that installs mpark-variant currently only conditionally in the default super-build.

I think that's currently only a todo, mpark is always linked to our built library and either installed from our shipped version or linked from an already-installed version from the system:

# C++11 std::variant (C++17 stdlib preview)
# TODO not needed with C++17 compiler
add_library(openPMD::thirdparty::mpark_variant INTERFACE IMPORTED)
set(openPMD_PC_EXTRA_INCLUDE "")
if(openPMD_USE_INTERNAL_VARIANT)
    target_include_directories(openPMD::thirdparty::mpark_variant SYSTEM INTERFACE
        $<BUILD_INTERFACE:${openPMD_SOURCE_DIR}/share/openPMD/thirdParty/variant/include>
    )
    message(STATUS "MPark.Variant: Using INTERNAL version '1.4.0'")
else()
    find_package(mpark_variant 1.3.0 REQUIRED) # TODO: we want 1.4.1+
    target_link_libraries(openPMD::thirdparty::mpark_variant
        INTERFACE mpark_variant)
    get_target_property(EXTERNAL_MPARK_INCLUDE mpark_variant INTERFACE_INCLUDE_DIRECTORIES)
    if(openPMD_HAVE_PKGCONFIG AND EXTERNAL_MPARK_INCLUDE)
        set(openPMD_PC_EXTRA_INCLUDE "-I${EXTERNAL_MPARK_INCLUDE}")
    endif()
    message(STATUS "MPark.Variant: Found version '${mpark_variant_VERSION}'")
endif()
target_link_libraries(openPMD PUBLIC openPMD::thirdparty::mpark_variant)

[ax3l]

I'll propose something based on try_compile https://cmake.org/cmake/help/latest/command/try_compile.html