[cker] Fix array-bounds build error: Refactor Shape.h to use C++17 std::variant

[ragmani]

This commit improves type-safety and memory management by leveraging modern C++17 features.

• Replace the old union-based storage (_dims and _dims_pointer) with a std::variant that holds either a std::array (for shapes with ≤ 6 dimensions) or a std::vector (for larger shapes)
• Update constructors to initialize and resize dims_ accordingly, ensuring that small shapes use the fixed-size array and larger ones use dynamic allocation
• Implement a custom copy constructor to perform a deep copy based on the current storage type and default the move constructor
• Update various member functions (Dims, SetDim, DimsData, Resize, ReplaceWith, BuildFrom, etc.) to use the new std::variant-based storage
• Minor include reordering and addition of necessary headers (e.g., , , )

ONE-DCO-1.0-Signed-off-by: ragmani ragmani0216@gmail.com

[ragmani]

This PR is for fixing #15030 (comment) and #14938 (comment).
The decision to change to using std::variant was driven by the unpredictable nature of the out-of-bound memory accesses we were encountering. In the previous design using a union, the allocated memory (e.g., a fixed-size array) sometimes did not match the computed size of the shape—particularly when the dimensions were determined at runtime via expressions like old_output_shape.DimensionsCount() + inputs.size(). This mismatch could lead to out-of-bounds accesses in functions such as memcpy, even if it didn't occur consistently.

By using std::variant to hold either a fixed-size std::array (when the number of dimensions is within limits) or a dynamically allocated std::vector (for larger shapes), we ensure that the storage exactly matches the required size at runtime. This type-safe, modern C++ solution eliminates the risks associated with manually managing memory within unions, and it helps prevent unexpected out-of-bounds errors during operations that copy or access shape data.

[ragmani]

I enabled Move Semantics deleted by the custom copy constructor:
It allows instances of Shape to be moved efficiently. Instead of copying potentially large amounts of data (especially for dynamically allocated storage when dimensions exceed kMaxSmallSize), the object’s resources can be transferred (or "moved") to a new object with minimal overhead.

For Performance Benefits:
In modern C++ (C++11 and later), move semantics are essential to optimize performance and resource management. By marking the move constructor as default, we ensure that move operations are available and implemented in the most efficient way possible by the compiler.

[ragmani]

I just fixed this PR errors by modifying Resize to preserve existing dimension values when changing the size.

[glistening]

I don't fully understand the related code:

• // For use only by ExtendedShape(), written to guarantee (return-value) copy
  // elision in C++17.
  // This creates a shape padded to the desired size with the specified value.
  Shape(int new_shape_size, const Shape &shape, int pad_value) : _size(0)
  ...

• Einsum

and why c++17 style fixed the problem.

I guess @hseok-oh would be the right person to review next week.
( It would take me a lot of time to understand the history of the related code. )

[ragmani]

@glistening

The build error occurred only with GCC on Ubuntu Focal, where the compiler detected potential out-of-bounds access during memcpy. Although I tried to fix the issue by minimizing the use of memcpy in there, the compiler still could not properly recognize the problem of the code. This was one of the reasons I decided to adopt a modern C++17-style design.

Originally, the Shape class used a union of a fixed-size array and a dynamic pointer, manually switching storage depending on the dimensions count. However, when the dimensions exceeded the static array size, unsafe memory access could occur, especially in operations like memcpy, triggering errors under the -Werror=array-bounds flag.

To robustly fix this, I restructured the design using std::variant, std::array, and std::vector. This approach guarantees that the storage type always matches the number of dimensions at runtime: small shapes are stored in a lightweight std::array, while larger shapes use dynamically allocated std::vector. As a result, I eliminated manual memory management issues and significantly improved type safety.

I also updated the Resize function to preserve existing dimension values during resizing, ensuring that no data was lost when changing shape sizes. Through these changes, I not only resolved the build error but also modernized the Shape implementation for better safety, clarity, and long-term maintainability.

Since this change was made urgently to prevent CI build errors, if this approach is not acceptable, please propose an alternative solution or directly fix the issue in a better way.

[ragmani]

I ran a benchmark with the following code using the -O3 compiler optimization option.

#include <iostream>
#include <variant>
#include <vector>
#include <array>
#include <chrono>
#include <cassert>

constexpr int kMaxSmallSize = 6;
constexpr int kLoopCount = 100000000; // 100 million iterations

// Legacy style (union-based storage)
struct LegacyShape
{
  int32_t _size;
  union
  {
    int32_t _dims[kMaxSmallSize];
    int32_t *_dims_pointer;
  };

  int32_t get(int i) const
  {
    return _size > kMaxSmallSize ? _dims_pointer[i] : _dims[i];
  }

  void set(int i, int32_t val)
  {
    if (_size > kMaxSmallSize)
      _dims_pointer[i] = val;
    else
      _dims[i] = val;
  }
};

// Variant style (using std::variant)
struct VariantShape
{
  int32_t _size;
  std::variant<std::array<int32_t, kMaxSmallSize>, std::vector<int32_t>> dims_;

  int32_t get(int i) const
  {
    if (_size <= kMaxSmallSize)
    {
      return std::get<std::array<int32_t, kMaxSmallSize>>(dims_)[i];
    }
    else
    {
      return std::get<std::vector<int32_t>>(dims_)[i];
    }
  }

  void set(int i, int32_t val)
  {
    assert(i >= 0 && i < _size);
    if (_size <= kMaxSmallSize)
    {
      std::get<std::array<int32_t, kMaxSmallSize>>(dims_)[i] = val;
    }
    else
    {
      std::get<std::vector<int32_t>>(dims_)[i] = val;
    }
  }
};

int main()
{
  LegacyShape legacy;
  VariantShape variant;

  // Initialization
  legacy._size = 6;
  for (int i = 0; i < 6; ++i) legacy._dims[i] = i;
  legacy._dims_pointer = nullptr; // Not used here

  variant._size = 6;
  variant.dims_ = std::array<int32_t, kMaxSmallSize>{0, 1, 2, 3, 4, 5};

  volatile int sum = 0; // Prevent compiler optimization
  volatile int total = 0; // Prevent compiler optimization

  // Benchmark: LegacyShape get()
  auto start1 = std::chrono::high_resolution_clock::now();
  for (int i = 0; i < kLoopCount; ++i)
  {
    sum += legacy.get(i % 6);
  }
  auto end1 = std::chrono::high_resolution_clock::now();
  std::cout << "LegacyShape get() time: "
            << std::chrono::duration_cast<std::chrono::milliseconds>(end1 - start1).count()
            << " ms" << std::endl;

  sum = 0; // Reset

  // Benchmark: VariantShape get()
  auto start2 = std::chrono::high_resolution_clock::now();
  for (int i = 0; i < kLoopCount; ++i)
  {
    sum += variant.get(i % 6);
  }
  auto end2 = std::chrono::high_resolution_clock::now();
  std::cout << "VariantShape get() time: "
            << std::chrono::duration_cast<std::chrono::milliseconds>(end2 - start2).count()
            << " ms" << std::endl;

  // Benchmark: LegacyShape set()
  auto start3 = std::chrono::high_resolution_clock::now();
  for (int i = 0; i < kLoopCount; ++i)
  {
    legacy.set(i % 6, i);
  }
  // Read back values after setting to prevent optimization
  for (int i = 0; i < 6; ++i)
  {
    total += legacy.get(i);
  }
  auto end3 = std::chrono::high_resolution_clock::now();
  std::cout << "LegacyShape set() time: "
            << std::chrono::duration_cast<std::chrono::milliseconds>(end3 - start3).count()
            << " ms" << std::endl;

  total = 0; // Reset

  // Benchmark: VariantShape set()
  auto start4 = std::chrono::high_resolution_clock::now();
  for (int i = 0; i < kLoopCount; ++i)
  {
    variant.set(i % 6, i);
  }
  // Read back values after setting to prevent optimization
  for (int i = 0; i < 6; ++i)
  {
    total += variant.get(i);
  }
  auto end4 = std::chrono::high_resolution_clock::now();
  std::cout << "VariantShape set() time: "
            << std::chrono::duration_cast<std::chrono::milliseconds>(end4 - start4).count()
            << " ms" << std::endl;

  return 0;
}

LegacyShape get() time: 236 ms
VariantShape get() time: 237 ms
LegacyShape set() time: 89 ms
VariantShape set() time: 90 ms

[ragmani]

I think this problem could have been prevented via #14679. If this change is accepted, I plan to apply the remaining changes modified in #14679.

[ragmani]

@glistening

I have done my best to document the history using the comment above. The reason I selected you as a reviewer is that the error started occurring when your commit (#15002) was merged. You can see the failing commit hash at this link. In my view, you are the most suitable person to review this, given your familiarity with the affected changes. If you feel that the history provided is insufficient, please let me know exactly what additional details you require. I believe it is more beneficial to understand the history and address the build error rather than leaving it unresolved in CI until next week. Please do not dismiss this comment since this PR resolves the build errors occurring in #14938 and #15030.

[ragmani]

@glistening PTAL

[ragmani]

@glistening PTAL

[glistening]

@ragmani

@glistening

I have done my best to document the history using the comment above. The reason I selected you as a reviewer is that the error started occurring when your commit (#15002) was merged.

I still don't understand.

1. My commit has been merged because the CI test passed.

2. I am not sure the recent error is correct. It may be false alarm. (Though I don't put time to understand eigen related code).

If it is urgent, ( though I don't think so ), I prefer to revert my commit #15002.

[glistening]

I think we need to figure out the reason of focal gcc's complain.

1.

As you wrote, (and I also already saw a few days ago),

gcc complains about Einsum::contractOperands.

In member function 'void nnfw::cker::Einsum::contractOperands(std::vector<nnfw::cker::Tensor>&, std::vector<bool>&, nnfw::cker::Tensor*)':

cc1plus: error: 'void* __builtin_memcpy(void*, const void*, long unsigned int)' forming offset [33, 36] is out of the bounds [0, 32] of object 'output_shape' with type 'nnfw::cker::Shape' [-Werror=array-bounds]

In file included from /home/jenkins/jenkins_agent/workspace/nnfw/master/push-nnpackage-test/runtime/onert/backend/cpu/ops/EinsumLayer.cc:19:

/home/jenkins/jenkins_agent/workspace/nnfw/master/push-nnpackage-test/runtime/compute/cker/include/cker/operation/Einsum.h:874:11: note: 'output_shape' declared here

Shape output_shape(old_output_shape.DimensionsCount() + inputs.size());
      |           ^~~~~~~~~~~~

We need to check this function needs to be updated as kMaxSmallSize became 6.

Code history says @hseok-oh wrote or brought ( ? ) the code.

I would like to ask him after next week his return.

2.

If there is nothing to do with eigen, the generated code needs some kind of alignment.

We may try to update kMaxSmallSize to 8, (not 6).

I would like to merge your PR after more seeming related actions and reviews are taken.

If someone is urgent from build break on focal (caused by #15002), we may push reverting PR of #15002 before investigating the above.

[ragmani]

As mentioned above, the issue only occurs on focal, and the PR CI does not include focal testing. Since we can no longer wait, I will proceed with reverting the change.
If that PR resolves the error, would you be willing to accept this PR?

[glistening]

I prefer your code than old style union.

I just want to understand the exact reason. But if we are in hurry, I am okay to merge your PR.

[glistening]

Changed code looks good. Let me check the focal error reason later since it causes build break on focal.