cranelift: Add heap support to the interpreter #3302
Conversation
github-actions
bot
added
the
cranelift
afonso360
force-pushed
the
interp-heap
branch
from
acf66fe to
9c97821
Compare
|
@cfallin Would you be able to review this? |
|
@afonso360 this fell off my radar, sorry about that! I'll review it today. |
There was a problem hiding this comment.
Thanks for all of this work!
Most of this looks reasonable, but I'm afraid I don't fully understand how the heap-address resolution w.r.t. heap-base global values is happening. A toplevel comment showing how the heap-base resolution works w.r.t. the addresses (which already have heap and offset parts) would help me immensely!
| let heap_base = self.resolve_global_value(heap_data.base)?; | ||
| let mut addr = Address::try_from(heap_base)?; | ||
| addr.size = size; | ||
| addr.offset += offset; |
There was a problem hiding this 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?
There was a problem hiding this comment.
the heap base is itself an address with a heap and offset component?
Yes, addresses always have those two components. However note that after resolving global values 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.
|
Hey @cfallin , I ended up not having a lot of time for working on this in the past year. I've cleaned up this patch a bit. |
There was a problem hiding this comment.
Hi @afonso360 -- thank you very much for picking this back up; it's close to ready I think, and it will be really valuable for testing once we have it!
I just have some questions below, possibly a result of having forgotten a lot of the discussion around this from before (but I guess that means we should document the answers in comments too!). Nothing major; the overall design is good.
| test_env: &RuntestEnvironment, | ||
| ) -> DataValue { | ||
| let mem = test_env.allocate_memory(); | ||
| let vmctx_struct = mem |
There was a problem hiding this 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!)
There was a problem hiding this 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.
|
|
||
| self.heaps.push(match init { | ||
| HeapInit::Zeroed(size) => iter::repeat(0).take(size).collect(), | ||
| HeapInit::FromBacking(backing) => backing, |
There was a problem hiding this 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?
There was a problem hiding this comment.
This function here is somewhat independent from 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 the heap directives. From there we just pass that memory directly into the interpreter, this happens in test_interpret.rs that's where we enforce that the heaps have the same size as the directives.
This is a leftover from bytecodealliance#3302 where we used to inject a special `vmctx` struct if the test requested it. We've removed that capability in bytecodealliance#5386 but accidentally left this in, which caused some weird handling of these test invocations.
Hey!
This PR adds heap support to the interpreter and support for heap tests for
test interpret.