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.

Sign up for GitHub

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

Jump to bottom

Adjust OperandStack allocation size when no locals #630

Merged
merged 2 commits into from
Jan 21, 2021
Merged

Adjust OperandStack allocation size when no locals #630

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
2 commits
Select commit Hold shift + click to select a range
cb0d9ff
Refactor the constructor of OperandStack
chfast Nov 2, 2020
4c9677f
Adjust OperandStack allocation size when no locals
chfast Nov 2, 2020
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
8 changes: 6 additions & 2 deletions lib/fizzy/stack.hpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -100,7 +100,11 @@ class OperandStack
OperandStack(
const Value* args, size_t num_args, size_t num_local_variables, size_t max_stack_height)
{
const auto storage_size_required = num_args + num_local_variables + max_stack_height;
const auto num_locals = num_args + num_local_variables;
// To avoid potential UB when there are no locals and the stack pointer is set to
// m_bottom - 1 (i.e. before storage array), we allocate one additional unused stack item.
const auto num_locals_adjusted = num_locals + (num_locals == 0); // Bump to 1 if 0.
gumb0 marked this conversation as resolved.
Show resolved Hide resolved
const auto storage_size_required = num_locals_adjusted + max_stack_height;

if (storage_size_required <= small_storage_size)
{
Expand All @@ -112,7 +116,7 @@ class OperandStack
m_locals = &m_large_storage[0];
}

m_bottom = m_locals + num_args + num_local_variables;
m_bottom = m_locals + num_locals_adjusted;
m_top = m_bottom - 1;

const auto local_variables = std::copy_n(args, num_args, m_locals);
Expand Down
15 changes: 15 additions & 0 deletions test/unittests/stack_test.cpp
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -231,6 +231,7 @@ TEST(operand_stack, large)
constexpr auto max_height = 33;
OperandStack stack(nullptr, 0, 0, max_height);
ASSERT_GT(address_diff(&stack, stack.rbegin()), 100) << "not allocated on the heap";
EXPECT_EQ(stack.size(), 0);

for (unsigned i = 0; i < max_height; ++i)
stack.push(i);
Expand Down Expand Up @@ -326,3 +327,17 @@ TEST(operand_stack, to_vector)
EXPECT_EQ(result[1].i32, 2);
EXPECT_EQ(result[2].i32, 3);
}

TEST(operand_stack, hidden_stack_item)
{
constexpr auto max_height = 33;
OperandStack stack(nullptr, 0, 0, max_height);
ASSERT_GT(address_diff(&stack, stack.rbegin()), 100) << "not allocated on the heap";
EXPECT_EQ(stack.size(), 0);
EXPECT_EQ(stack.rbegin(), stack.rend());

// Even when stack is empty, there exists a single hidden item slot.
const_cast<fizzy::Value*>(stack.rbegin())->i64 = 1;
EXPECT_EQ(stack.rbegin()->i64, 1);
Copy link
Member

Choose a reason for hiding this comment

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

Hm, I think we should document this behaviour in the header.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

No. This is a logical error to do this.

Copy link
Member

Choose a reason for hiding this comment

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

The documentation (https://en.cppreference.com/w/cpp/iterator/begin) doesn't state anywhere that begin() would be UB in case of size() == 0.

Copy link
Member

Choose a reason for hiding this comment

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

@chfast any feedback?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Taking the iterator is fine, but you cannot dereference it when it is the end iterator.

Copy link
Member

Choose a reason for hiding this comment

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

But here you are dereferencing it?

To be honest at this stage I lost what the conversation is about.

Copy link
Member

Choose a reason for hiding this comment

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

Ah okay, we do ensure that begin() == end(), just that we know begin() is always valid.

EXPECT_EQ(stack.rend()->i64, 1);
}