Skip to content
This repository has been archived by the owner on Mar 24, 2022. It is now read-only.

Commit

Permalink
fixup: issue description & todo (empty commit)
Browse files Browse the repository at this point in the history
Fix #433.

---

 ### The Context (for `Context`)

I've had a nagging feeling something is a little "off" about `Context`
since we had to add
[parent_ctx](https://github.com/bytecodealliance/lucet/blob/master/lucet-runtime/lucet-runtime-internals/src/context/mod.rs#L122)..
I said I'd ruminate on this back when @acfoltzer added it to [correct
our `unsafe impl Send for
InstanceHandle`](#342) a
few months ago, and my ruminating has finally yielded fruit (maybe). For
context, this field is necessary because if an instance is moved between
threads when yielded, when we `Context::swap` back into the instance we
need to update this to swap back to the right host context (lest we
return onto the wrong stack and ).

 ### The What

It's seemed to me that `parent_ctx`, `backstop_callback`, and
`backstop_data` shouldn't _really_ be on `Context` particularly because
they should only exist in one `Context`: the guest instance we're
running. The host's `parent_ctx` might end up set to f.ex the guest
`Context` but never used, `backstop_callback` only matters in
`lucet_context_backstop` and so should never be reached in the host,
`backstop_data` is a parameter that only exists for the backstop
callback...

So the realization is this: it seems fair to say these are all
properties of an `Instance`, of a specific `Context`. Further,
`lucet_context_backstop` (and `lucet_context_bootstrap`) really aren't
functions operating on a `Context`, they operate on an `Instance` and
manipulate the guest's `Context`.

I think a good name for `parent_ctx`, `backstop_callback`, and
`backstop_data` might be `InstanceExitData` or something of that nature
- these taken together are necessary for an Instance to correctly exit
its context either in a normal return or in a terminal fault.

 ### The Why

I conceptually treat `Context` as an encapsulation of the ABI around a
function call: arguments, return values, signal handler state, fin.
Overloading it with Instance-specific data seems to go beyond the spirit
of its intent. If this isn't how yall see it, then maybe this doubles as
an argument for it being read that way!

As an additional benefit it moves some instance data off the gnarly
`10*8 + 8*16 + 8*2 + 16`-style offsets into a `Context` where my smart
brain knows we catch errors in tests but my code review brain gets
utterly horrified.

 ### And A How

This functionally would look like
`lucet_context_{activate,backstop,bootstrap}` to
`lucet_instance_{activate,backstop,bootstrap}` (and maybe giving them
their own `.S` or something, I'm less opinionated there), as well as
[a](https://github.com/bytecodealliance/lucet/blob/master/lucet-runtime/lucet-runtime-internals/src/context/context_asm.S#L75),
[few](https://github.com/bytecodealliance/lucet/blob/master/lucet-runtime/lucet-runtime-internals/src/context/context_asm.S#L84),
[instructions](https://github.com/bytecodealliance/lucet/blob/master/lucet-runtime/lucet-runtime-internals/src/context/context_asm.S#L90).
I think it's straightforward overall, if this sounds agreeable - this
issue would basically be the PR text I'd write there, anyway.
  • Loading branch information
Katelyn Martin committed May 14, 2020
1 parent ee990db commit 910c1ac
Showing 0 changed files with 0 additions and 0 deletions.

0 comments on commit 910c1ac

Please sign in to comment.