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

Implement Index<Range<…>> for OpStack #322

Open
jan-ferdinand opened this issue Aug 30, 2024 · 1 comment
Open

Implement Index<Range<…>> for OpStack #322

jan-ferdinand opened this issue Aug 30, 2024 · 1 comment
Labels
✨ enhancement Improvement or new feature 🟢 prio: low Not at all urgent

Comments

@jan-ferdinand
Copy link
Member

The operational stack can be indexed into to fetch individual elements through its implementations of Index<usize> and Index<OpStackElement> (and the corresponding IndexMut<…>). It can be useful to get a slice from the op stack.

Implement the combinations {Index, IndexMut} $\times$ {Range, RangeInclusive} $\times$ {usize, OpStackElement} for OpStack.

@jan-ferdinand jan-ferdinand added 💫 good first issue Good for newcomers ✨ enhancement Improvement or new feature 🟢 prio: low Not at all urgent labels Aug 30, 2024
@jan-ferdinand
Copy link
Member Author

#338 has uncovered that there are some rather fundamental issues when trying to add this feature. Summarizing for posteriority:

Currently, OpStack uses a Vec under the hood. The top of Triton VM's op stack, or op_stack[0], is the last element in the internal vector. To provide semantically correct access to some op_stack[i..j] range, the underlying vector has to be indexed “in reverse”, which does not produce a contiguous slice.

It is possible to allocate a new vector containing the correct stack elements and to then leak that new vector. I feel uncomfortable creating memory leaks like this.

Using a VecDeque, which seemed like the obvious next choice, also does not work, because it is implemented as a ring buffer. This means that a range of elements from a VecDeque are not necessarily contiguous in memory, requiring re-organization within the implementation of the Index trait again, leading back to the same problems the current implementation is facing.

The only real solution I can see is to implement a “reverse” vector, such that op_stack[0] is the always the first element in that data structure.

@jan-ferdinand jan-ferdinand removed the 💫 good first issue Good for newcomers label Jan 8, 2025
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
✨ enhancement Improvement or new feature 🟢 prio: low Not at all urgent
Projects
None yet
Development

Successfully merging a pull request may close this issue.

1 participant