Directly test a stream of opcodes

[martin-hughes]

This PR relies on #293 being merged, otherwise the new test will fail Expect a "pipeline failed" email for this PR.

Adds the ability to specify a stream of opcodes to test. These are wrapped in a DefinitionBlock and MAIN function. The new test method calls MAIN in the same way as existing tests (0 or undefined being "pass" results).

This allows us to regression-test #289.

Since we now need to encode pkglength, I've moved all pkglength code (including decode) into its own module and added some tests for it.

[martin-hughes]

I don’t think I can re-trigger the workflows (or I can’t see the button!) but I think you can @IsaacWoods - it should run OK now

[IsaacWoods]

Nice, thanks for working on this! There are probably a few more places it would be convenient to test against encoded streams.

A few comments re organisation.

Edit: I can re-trigger the workflow, but it looks like it just re-pulls the PR branch. I think to test against the new main including #293, you would need to rebase and force-push.

[IsaacWoods]

This is another tricky case re improving testability vs increasing cognitive overhead to the interpreter.

I don't think the payoff of the separate module + the closure adds enough value vs being able to see this instinctually as a loop munching a variable-size length specifier. Breaking pkglength decoding would so fundamentally break the AML tests that I don't think unit tests add much here.

[IsaacWoods]

I think this probably belongs in test_infra rather than in the interpreter, at which point it can just panic instead of having an error type.

[IsaacWoods]

A nit: commenting on code structure here distracts the reader from the logic itself

[IsaacWoods]

As above, I think we should probably have the logic to encode pkglengths in the testing logic rather than the interpreter. Having this as encode in the namespace is also a bit confusing (you would not know this was for a pkglength if not for the comment).

Idle thoughts: I don't know if we'd want to go this far, but one option would be to have this be a wrapper over Vec<u8> that is a sort-of AML builder pattern. Something like:

builder.push(Opcode::Method);
builder.push_pkglength(x);
builder.push_namestring(b"MAIN");
...

[martin-hughes]

The first part, I can partially fix encode by making it pkglength::encode. See my later comment re organisation.

For the second part: yes, like your thinking for the future. Naturally I'd probably extend it by adding pkglength and Namestring types with a ToOpcodes (or similar) trait, and then having push be generic across impl ToOpcodes. Or something like that.

[martin-hughes]

I can re-trigger the workflow, but it looks like it just re-pulls the PR branch. I think to test against the new main including #293, you would need to rebase and force-push.

Ah cool - live and learn. I can see the workflow succeeds now

[martin-hughes]

I've made a minor update and rebased, but I want to push back on organisation and hear your thoughts.

To be up front, these are my two main points of view - I think you disagree, but please bear with me:

1. The Interpreter module is way too long and complex
2. I think the crate as a whole is very under-tested - but I'm influenced by finding a lot of bugs that seem like they should have popped up pretty quickly

Starting with testing, and in particular your comment

Breaking pkglength decoding would so fundamentally break the AML tests that I don't think unit tests add much here.

I suppose the ideal is that if you fiddled with it as part of a larger project, you'd know straight away that it was wrong, rather than finishing working on everything and only finding out at the end you'd made a bad assumption and had to fix a load of stuff.

But also, it's not a given that "obvious" breaks will get caught quickly - even if in this case it probably would. Allow me to tell you a war-story (sorry, boring neckbeard alert!) Once upon a time, whilst I still coded for a living, I did some "low-risk" refactoring around some password-verification code as part of a project relating to allowing one person to use multiple VoIP phones at once. Long story short, I inverted the result of that check (i.e. wrong password = OK, correct password = Access Denied). This commit survived:

• My own code review
• My own live-testing (fat-fingered a test password or didn't test passwords?)
• Running our unit-test suite (presumably it didn't test passwords? They are optional in the spec...)
• Peer code review
• Peer live-testing (maybe they assumed they couldn't type passwords either?)

and only got caught by a much more in-depth overnight multi-hour test suite. Of which 75%+ of tests now failed. I got an angry call from the QA manager demanding to know why I was such an unbelievable imbecile.

So now I try to make sure even "foundational" code gets tested, so my screw-ups are caught ASAP.

But deeper than this, I'd say it's important to separate responsibilities as much as possible - MethodContext doesn't need to know how to interpret pkglength (or Namestring, or u64, or any of the others, but they're not in scope here) so it shouldn't - single responsibility principle from the SOLID principles. Decoding a pkglength only adjusts the stream pointer, it doesn't affect any other invariant of MethodContext, so why give the decoder that access?

On top of that, when you're working on MethodContext, you don't need to see how those decodes work - they're mostly noise. If the interface works (proven by testing!) you know you're getting the right results in MethodContext. So IMO, much better to hide that noise.

A tangential point on testing and separating responsibilities: some guidance given to me by a mentor was "if you think about how you'll test your code, that will help you design your code".

OK, so that's my argument for why I'd like the decoding to be separate from MethodContext itself. But why another module?

Pragmatically, mod.rs is way too big to absorb fully - you have the advantage of having written it, but for a reader there's a lot to deal with. Especially since there's not a huge amount of documentation about how it fits together. Splitting it into sub-modules has two advantages:

1. Each individual sub-module is easier to parse by itself. You don't have to think about irrelevant context like "does pkglength need ResolveBehaviour?" These relationships become much clearer from the code itself and its import statements.
2. It encourages separation of responsibilities, like "does MethodContext really need to decode pkglength?". There's a lot of private functions in the various impl blocks - which of those are used only by the struct itself, and which by other structs in mod.rs? Splitting them into submodules makes the privacy rules clearer.

Does that make the whole aml module easier to deal with? Almost certainly - you can focus on the high-level relationships because you know the low-level stuff is tucked away - no wondering about which bits are important.

One final specific - why put "encode" and "decode" together? Encoding isn't really a test-specific thing, but it is an AML-specific thing. So IMO it lives somewhere within the aml module. I guess there's a question about whether acpi::aml should be decode-specific... If so, the answer might be to split pkglength and other AML "types" encode/decode into a separate crate or sub-crate.

Plus keeping encode and decode together has made them way easier to test!

So anyway, lots of writing there. Does that change your thinking at all? It is your crate to be the boss of, of course!

[IsaacWoods]

Thanks for taking the time to write your thoughts out - it's always useful to get other perspectives, so I appreciate that.

I think the crate as a whole is very under-tested - but I'm influenced by finding a lot of bugs that seem like they should have popped up pretty quickly

On reflection, this is very fair. I've been surprised (and often slightly embarrassed) at the number of bugs that you've found from your more thorough testing. I do think in this case, we would notice breakage very easily, but there is of course value in testing this regardless.

I still stand by a dislike for testing that necessitates changes to code structure, particularly abstraction that is unnecessary in the base design. In this case, that it needing to abstract getting the next byte of the stream via a passed function, as outside of testing that will always fundamentally be the next byte from the current stream. However, I think we can avoid this and allow unit tests as I'll talk about below, in a way that hopefully we can agree provides value.

---

Pragmatically, mod.rs is way too big to absorb fully - you have the advantage of having written it, but for a reader there's a lot to deal with

This is also fair. At the start of the interpreter rewrite, I was very wary of committing to much of a structure bar "the interpreter" as the thing that hamstrung my previous attempts had been AML's relentless habit of needing to access interpreter state / jump back into interpreting arbitrary AML when you least expect it. I have still managed to fall into this trap (currently, parsing of field lists cannot deal with arbitrary AML that can appear in niche Connection entries).

However, that has also created a familiarity for me that is of course not at all universal - I know my way around the interpreter as is, but I can appreciate that mod.rs is an intimidating file if you're coming at the project without that familiarity.

The MethodContext critique is also very fair - it has grown quite organically and better abstractions are definitely possible that, as you say, could separate stream decoding and operation/argument management. Originally, I think the value I saw in the decoding methods on MethodContext was ergonomic access to decoding an x from the interpreter, and MethodContext already managing where the current stream was.

---

This is more off-topic for this PR, but I may as well set out my thinking so far re where I see breaking up mod.rs given the above.

• As above, MethodContext and Block deal with two fairly contrasting concerns - AML stream management, and management of in-flight operations and resolution behaviour of operands. Decoding logic could move into a sub-module under a RunningAmlStream (all names bike-sheddable) that kept a reference to a stream and a pc. Complexities here would be:
  • Safety around keeping the stream alive long enough. Currently we use raw pointers and an Arc to the method the stream came from (unless it's from a table) in MethodContext (and I thought a lifetime on Block but it looks like that's a figment of my imagination...)
  • We currently use space in Opcode to encode internal opcodes the interpreter uses within itself. Leaking this into a sub-module feels a bit gross but I don't see many alternatives.
  • Ensuring we can still do all the needed control-flow (e.g. for Whiles). I don't think this would pose too much issue.
• Potentially field IO - I'd like to get custom region handlers working at some point, and that does provide an opportunity for potentially simplifying how we do the native field IO bit of the interpreter (do_native_region_read/write). The currently-implemented regions could have default handlers (these could perhaps go in the existing region module) that deal with complexities like PCI addressing (potentially with caching as we've previously discussed). This would require region handlers being passed the Interpreter temporarily, but I think this would be possible.
• Potentially a rework of AmlError, which has become somewhat unwieldy and also often not that useful. I'm not particularly up-to-date on newer error handling in Rust, but I wonder if some sort of generic error interface, perhaps with added metadata such as spans to the AML, would provide more value.
• Potentially some of the complex logic operations - I think there's a tradeoff to exploration here in the extra indirection of realising that e.g. Interpreter::do_concat is just a wrapper for some logic in a totally different place, but I can see myself coming round to it. This could allow easier unit testing of those broken-out functions.

In terms of testing, having some sort of AmlStream type that could be constructed from some bytes and then tests calling a series of decoding methods on it sounds like a fairly good way of testing e.g. pkglengths? I can imagine having region handlers for mocked IO could also be useful for complex tests.

---

One final specific - why put "encode" and "decode" together? Encoding isn't really a test-specific thing, but it is an AML-specific thing. So IMO it lives somewhere within the aml module. I guess there's a question about whether acpi::aml should be decode-specific... If so, the answer might be to split pkglength and other AML "types" encode/decode into a separate crate or sub-crate.

This is the bit I'm not coming round to, I don't think. I think the scope of the acpi crate should be that it is an AML interpreter, and that all patching/construction of AML (outside testing infrastructure) is out-of-scope. (FWIW, I do think there would be value in AML tooling written in Rust, but I don't think I have any capacity to maintain it). I think having any encoding logic in acpi (or a crate brought in as a dependency) muddies that intention.