afonso360 opened PR #3302 from interp-heap
to main
:
Hey!
This PR adds heap support to the interpreter and support for heap tests for
test interpret
.
afonso360 submitted PR review.
afonso360 created PR review comment:
I'm not entirely sure this belongs here.
I had this separately in
InterpreterState::get_heap_address
, but I didn't want to duplicate the bounds checking done on this function.Since bounds checking is only required as part of the runtime guarantees (for
heap_address
in addressable memory), I think it would make sense to put this back inget_heap_address
, but also means we then haveInterpreterState::get_heap_address
andInterpreterState::heap_address
which do something different.
afonso360 submitted PR review.
afonso360 created PR review comment:
I'm not too happy with this interface as a way to register a VMContext struct.
We need to have a way to address this struct, and currently we only have Stack and Heap addresses. Adding a whole new region just for VMContext structs seems a bit much, but this is also not very clear.
afonso360 edited PR review comment.
afonso360 edited PR review comment.
afonso360 deleted PR review comment.
afonso360 edited PR review comment.
afonso360 updated PR #3302 from interp-heap
to main
.
cfallin submitted PR review.
cfallin created PR review comment:
At a high level, I'm not sure I fully grok what's going on here: the heap base is itself an address with a heap and offset component? In other words, do heaps resolve into regions within other heaps? If so, could you add a comment here (or a toplevel one somewhere) describing how the resolution works?
cfallin created PR review comment:
Should this be
!= 0
? (I haven't fully internalized the interpreter state data structures so I could be misunderstanding whatframe_stack
actually means.)
cfallin submitted PR review.
cfallin created PR review comment:
Is there a reason for the trailing underscore here? Otherwise
static_heap_i64
seems fine to me.
cfallin created PR review comment:
Ah -- does this have to do with the maximum test identifier length? If so, just choosing shorter names (rather than truncating them) seems like a good solution.
afonso360 submitted PR review.
afonso360 created PR review comment:
Oh wow, yes it should. And the branches in the if are also swapped. :sweat_smile:
afonso360 updated PR #3302 from interp-heap
to main
.
afonso360 updated PR #3302 from interp-heap
to main
.
afonso360 submitted PR review.
afonso360 created PR review comment:
the heap base is itself an address with a heap and offset component?
Well, yes it could be. Resolving global values means that that address could be anywhere in memory, even in the stack.In other words, do heaps resolve into regions within other heaps?
Yes, we can have multiple CLIF Heaps that resolve to the same heap in the interpreter.This function was quite a bit confusing because we used the same
Heap
type to mean both a CLIF Heap and a Interpreter Heap. I've now separated those two into separate types, and the function now
only does CLIF Heap resolution.
afonso360 edited PR review comment.
afonso360 edited PR review comment.
afonso360 edited PR review comment.
cfallin created PR review comment:
If I understand this right, it is building a vmctx out of a linear layout of (base, length) tuples, but don't the heap directives at least for the runtests allow us to put the base and length pointers at arbitrary vmctx offsets? Do we need to consider that here? (If not, a comment would help!)
cfallin created PR review comment:
Do we need to check here that the length of the backing vec is at least as large as the size that was claimed in the
; heap: ...
directives, or somehow connect to that? Or is there an invariant that we will extend it if needed?
cfallin created PR review comment:
s/its/it's/
cfallin created PR review comment:
s/essential/essentially/
cfallin submitted PR review.
cfallin created PR review comment:
stray type annotation in comment -- remove?
cfallin submitted PR review.
afonso360 submitted PR review.
afonso360 created PR review comment:
We had that design at the beginning but eventually settled on having the annotations in the
; heap
directive but forcing them to a linear layout of tuples. This is enforced when parsing the heap directives.I've added a comment clarifying this.
afonso360 submitted PR review.
afonso360 created PR review comment:
This function here is somewhat independent of the testing infrastructure. And the interpreter doesn't really care about the size of the backing memory, since out of bounds writes/reads generate a trap.
The memory is allocated by the
RuntestEnvironment
struct which is the same struct that parses theheap
directives. From there we just pass that memory directly into the interpreter, this happens intest_interpret.rs
that's where we enforce that the heaps have the same size as the directives.
afonso360 edited PR review comment.
afonso360 updated PR #3302 from interp-heap
to main
.
cfallin submitted PR review.
cfallin merged PR #3302.
Last updated: Dec 23 2024 at 12:05 UTC