Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

StackSpace: use std::aligned_alloc() #907

Merged
merged 1 commit into from
May 28, 2024
Merged
Changes from all commits
Commits
File filter

Filter by extension

Filter by extension

Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
41 changes: 35 additions & 6 deletions lib/evmone/execution_state.hpp
Original file line number Diff line number Diff line change
Expand Up @@ -5,6 +5,7 @@

#include <evmc/evmc.hpp>
#include <intx/intx.hpp>
#include <memory>
#include <string>
#include <vector>

Expand All @@ -27,20 +28,48 @@ using intx::uint256;
/// Provides memory for EVM stack.
class StackSpace
{
static uint256* allocate() noexcept
{
static constexpr auto alignment = sizeof(uint256);
static constexpr auto size = limit * sizeof(uint256);
#ifdef _MSC_VER
// MSVC doesn't support aligned_alloc() but _aligned_malloc() can be used instead.
const auto p = _aligned_malloc(size, alignment);
#else
const auto p = std::aligned_alloc(alignment, size);
#endif
return static_cast<uint256*>(p);
}

struct Deleter
{
// TODO(C++23): static
void operator()(void* p) noexcept
{
#ifdef _MSC_VER
// For MSVC the _aligned_malloc() must be paired with _aligned_free().
_aligned_free(p);
#else
std::free(p);
#endif
}
};

/// The storage allocated for maximum possible number of items.
/// Items are aligned to 256 bits for better packing in cache lines.
std::unique_ptr<uint256, Deleter> m_stack_space;

public:
/// The maximum number of EVM stack items.
static constexpr auto limit = 1024;

StackSpace() noexcept : m_stack_space{allocate()} {}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I wonder if

std::unique_ptr<uint256[]>(new (std::align_val_t(sizeof(uint256)) uint256[limit]);

achieves the same.

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It looks like, however this specifies the array element alignment, so I wonder what happens if you specify 64 for uint256. I plan to align the allocation to 2048*32.


/// Returns the pointer to the "bottom", i.e. below the stack space.
[[nodiscard, clang::no_sanitize("bounds")]] uint256* bottom() noexcept
{
return m_stack_space - 1;
return m_stack_space.get() - 1;
Copy link
Collaborator

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Why does it need to go outside the memory bounds?

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

It is the convention we use currently: we keep the pointer to the top element on the stack. Therefore its starting position (stack empty) is one element below the allocated memory.

}

private:
/// The storage allocated for maximum possible number of items.
/// Items are aligned to 256 bits for better packing in cache lines.
alignas(sizeof(uint256)) uint256 m_stack_space[limit];
};


Expand Down