Merge pull request #3199 from JasonRuonanWang/struct

make struct size parameter compulsory to ensure padding correctness
ornladios · May 4, 2022 · f003876 · f003876
2 parents 445f4ed + a1672d1
commit f003876
Show file tree

Hide file tree

Showing 11 changed files with 24 additions and 24 deletions.
diff --git a/bindings/CXX11/adios2/cxx11/ADIOS.cpp b/bindings/CXX11/adios2/cxx11/ADIOS.cpp
@@ -85,11 +85,11 @@ Operator ADIOS::InquireOperator(const std::string name)
     }
 }
 
-StructDefinition ADIOS::DefineStruct(const std::string &name)
+StructDefinition ADIOS::DefineStruct(const std::string &name, const size_t size)
 {
     CheckPointer("for struct name " + name +
                  ", in call to ADIOS::DefineStruct");
-    return StructDefinition(&m_ADIOS->DefineStruct(name));
+    return StructDefinition(&m_ADIOS->DefineStruct(name, size));
 }
 
 StructDefinition ADIOS::InquireStruct(const std::string &name)

diff --git a/bindings/CXX11/adios2/cxx11/ADIOS.h b/bindings/CXX11/adios2/cxx11/ADIOS.h
@@ -214,7 +214,7 @@ class ADIOS
      */
     Operator InquireOperator(const std::string name);
 
-    StructDefinition DefineStruct(const std::string &name);
+    StructDefinition DefineStruct(const std::string &name, const size_t size);
     StructDefinition InquireStruct(const std::string &name);
 
     /**

diff --git a/source/adios2/core/ADIOS.cpp b/source/adios2/core/ADIOS.cpp
@@ -194,15 +194,17 @@ ADIOS::InquireOperator(const std::string &name) noexcept
     }
 }
 
-StructDefinition &ADIOS::DefineStruct(const std::string &name)
+StructDefinition &ADIOS::DefineStruct(const std::string &name,
+                                      const size_t size)
 {
     if (m_StructDefinitions.find(name) != m_StructDefinitions.end())
     {
         helper::Throw<std::invalid_argument>("Core", "ADIOS", "CheckOperator",
                                              "Struct " + name +
                                                  " defined twice");
     }
-    return m_StructDefinitions[name];
+    return m_StructDefinitions.emplace(name, StructDefinition(size))
+        .first->second;
 }
 
 StructDefinition *ADIOS::InquireStruct(const std::string &name)

diff --git a/source/adios2/core/ADIOS.h b/source/adios2/core/ADIOS.h
@@ -132,7 +132,7 @@ class ADIOS
     std::pair<std::string, Params> *
     InquireOperator(const std::string &name) noexcept;
 
-    StructDefinition &DefineStruct(const std::string &name);
+    StructDefinition &DefineStruct(const std::string &name, const size_t size);
     StructDefinition *InquireStruct(const std::string &name);
 
     /**

diff --git a/source/adios2/core/IO.cpp b/source/adios2/core/IO.cpp
@@ -964,10 +964,10 @@ VariableStruct *IO::InquireStructVariable(const std::string &name,
     return ret;
 }
 
-StructDefinition &IO::DefineStruct(const std::string &name)
+StructDefinition &IO::DefineStruct(const std::string &name, const size_t size)
 {
-    m_StructDefinitions[name] = StructDefinition();
-    return m_StructDefinitions[name];
+    return m_StructDefinitions.emplace(name, StructDefinition(size))
+        .first->second;
 }
 
 StructDefinition *IO::InquireStruct(const std::string &name)

diff --git a/source/adios2/core/IO.h b/source/adios2/core/IO.h
@@ -505,7 +505,7 @@ class IO
     /** Inform about computation block through User->ADIOS */
     void ExitComputationBlock() noexcept;
 
-    StructDefinition &DefineStruct(const std::string &name);
+    StructDefinition &DefineStruct(const std::string &name, const size_t size);
     StructDefinition *InquireStruct(const std::string &name);
 
 private:

diff --git a/source/adios2/core/VariableStruct.cpp b/source/adios2/core/VariableStruct.cpp
@@ -17,7 +17,7 @@ namespace adios2
 namespace core
 {
 
-void StructDefinition::SetStructSize(const size_t size) { m_StructSize = size; }
+StructDefinition::StructDefinition(const size_t size) : m_StructSize(size) {}
 
 void StructDefinition::AddItem(const std::string &name, const size_t offset,
                                const DataType type, const size_t size)
@@ -28,19 +28,18 @@ void StructDefinition::AddItem(const std::string &name, const size_t offset,
                                              "VariableStruct::StructDefinition",
                                              "AddItem", "invalid data type");
     }
-    if (offset < m_StructSize)
+    if (offset + helper::GetDataTypeSize(type) * size > m_StructSize)
     {
         helper::Throw<std::invalid_argument>("core",
                                              "VariableStruct::StructDefinition",
-                                             "AddItem", "invalid offset");
+                                             "AddItem", "exceeded struct size");
     }
     m_Definition.emplace_back();
     auto &d = m_Definition.back();
     d.Name = name;
     d.Offset = offset;
     d.Type = type;
     d.Size = size;
-    m_StructSize = offset + helper::GetDataTypeSize(type) * size;
 }
 
 size_t StructDefinition::StructSize() const noexcept { return m_StructSize; }

diff --git a/source/adios2/core/VariableStruct.h b/source/adios2/core/VariableStruct.h
@@ -30,7 +30,7 @@ class StructDefinition
         size_t Size;
     };
 
-    void SetStructSize(const size_t size);
+    StructDefinition(const size_t size);
     void AddItem(const std::string &name, const size_t offset,
                  const DataType type, const size_t size = 1);
     size_t StructSize() const noexcept;

diff --git a/source/adios2/engine/ssc/SscHelper.cpp b/source/adios2/engine/ssc/SscHelper.cpp
@@ -348,8 +348,7 @@ void DeserializeVariable(const Buffer &input, const ShapeID shapeId,
                 }
                 if (b.shapeId == ShapeID::GlobalArray)
                 {
-                    auto def = io.DefineStruct(b.name);
-                    def.SetStructSize(b.elementSize);
+                    auto def = io.DefineStruct(b.name, b.elementSize);
                     io.DefineStructVariable(b.name, def, vShape, vStart,
                                             vShape);
                 }

diff --git a/testing/adios2/engine/ssc/TestSscStruct.cpp b/testing/adios2/engine/ssc/TestSscStruct.cpp
@@ -63,16 +63,16 @@ void Writer(const Dims &shape, const Dims &start, const Dims &count,
     auto varIntScalar = io.DefineVariable<int>("varIntScalar");
     auto varString = io.DefineVariable<std::string>("varString");
 
-    auto particleDef = adios.DefineStruct("particle");
-    particleDef.AddItem("a", 0, adios2::DataType::Int8, 1);
-    particleDef.AddItem("b", 4, adios2::DataType::Int32, 4);
-    auto varStruct =
-        io.DefineStructVariable("particles", particleDef, shape, start, count);
     struct particle
     {
         int8_t a;
         int b[4];
     };
+    auto particleDef = adios.DefineStruct("particle", sizeof(particle));
+    particleDef.AddItem("a", 0, adios2::DataType::Int8, 1);
+    particleDef.AddItem("b", 4, adios2::DataType::Int32, 4);
+    auto varStruct =
+        io.DefineStructVariable("particles", particleDef, shape, start, count);
     std::vector<particle> myParticles(datasize);
     for (size_t i = 0; i < datasize; ++i)
     {

diff --git a/testing/adios2/interface/TestADIOSDefineVariable.cpp b/testing/adios2/interface/TestADIOSDefineVariable.cpp
@@ -661,11 +661,11 @@ TEST_F(ADIOSDefineVariableTest, DefineStructVariable)
     const adios2::Dims start = {0};
     const adios2::Dims count = {10};
 
-    auto def1 = adios.DefineStruct("def1");
+    auto def1 = adios.DefineStruct("def1", 24);
     def1.AddItem("a", 0, adios2::DataType::Int8, 1);
     def1.AddItem("b", 4, adios2::DataType::Int32, 5);
 
-    auto def2 = adios.DefineStruct("def2");
+    auto def2 = adios.DefineStruct("def2", 28);
     def2.AddItem("a", 0, adios2::DataType::Int8, 1);
     def2.AddItem("b", 4, adios2::DataType::Int32, 5);
     def2.AddItem("c", 24, adios2::DataType::Int32);