Ensure that Locals passed to Methods are preserved - #335
martin-hughes wants to merge 1 commit into
Conversation
| /// Returns a tuple containing: | ||
| /// - The object that should be modified | ||
| /// - A boolean indicating whether an implicit cast should occur before the store | ||
| pub fn unwrap_ref_for_store(self) -> Result<(WrappedObject, bool), AmlError> { |
There was a problem hiding this comment.
I've pulled this function out of do_store for a few reasons:
- It makes it way easier to test in unit tests
- I think it looks nicer sitting next to the other
unwrapfunctions, since it matches a large part of their functionality - It doesn't need to access any
Interpreterfields, nor protect any invariants - so it's worth moving out of thatImpl.
Counter argument could be that it's explicitly "for stores" rather than generic object behaviour. I'm sympathetic to that... but look at my shiny unit tests 😉
There was a problem hiding this comment.
I'm sympathetic to your reasoning here. I don't see a reason an end-user would want to call this - would making it pub(crate) be reasonable?
| // return that instead (a bit like a normal `unwrap_reference`) | ||
| // | ||
| // See issue 313 and the `store.asl` tests for more details. | ||
| let mut found_arg_to_local: Option<Result<(WrappedObject, bool), AmlError>> = None; |
There was a problem hiding this comment.
This does pollute the loop a bit. In my first draft I had a straightforward unwrap_reference and a string type check that returned before the loop. But I wasn't a fan of potentially unwrapping twice. Could change it back if you prefer - my way could be a premature optimisation.
There was a problem hiding this comment.
I think with the above comment this is okay and probably as understandable as possible. I have no idea how NT would have ended up with this behaviour... would be interesting to see.
There was a problem hiding this comment.
This is unchanged (except possibly in the comments) from the file I sent you by PM.
|
@IsaacWoods would you mind taking a look? I'm pretty confident that the logic is correct, but you might be able to think of more edge cases than I can. (Also any other interested party feel free to chip in!) |
|
I spotted some tests from uACPI that fail which I'd like to add, so converted back to draft for now. |
| /// Returns a tuple containing: | ||
| /// - The object that should be modified | ||
| /// - A boolean indicating whether an implicit cast should occur before the store | ||
| pub fn unwrap_ref_for_store(self) -> Result<(WrappedObject, bool), AmlError> { |
There was a problem hiding this comment.
I'm sympathetic to your reasoning here. I don't see a reason an end-user would want to call this - would making it pub(crate) be reasonable?
| // return that instead (a bit like a normal `unwrap_reference`) | ||
| // | ||
| // See issue 313 and the `store.asl` tests for more details. | ||
| let mut found_arg_to_local: Option<Result<(WrappedObject, bool), AmlError>> = None; |
There was a problem hiding this comment.
I think with the above comment this is okay and probably as understandable as possible. I have no idea how NT would have ended up with this behaviour... would be interesting to see.
f8fd56f to
add83ba
Compare
Since both Locals and Args are stored as references internally, a plain `unwrap_reference` was causing Locals that got passed to methods to be treated as though they had been passed by reference, instead of by value. This corrects that logic, and also takes into account the strange behaviour of the Windows NT interpreter - it treats strings passed via Locals into methods as passed by reference, when appropriate.
add83ba to
8ec1ce6
Compare
| Opcode::Increment | Opcode::Decrement => { | ||
| let [Argument::Object(operand)] = &op.arguments[..] else { panic!() }; | ||
| let operand = operand.clone().unwrap_transparent_reference(); | ||
| let operand = operand.clone().unwrap_reference(); |
There was a problem hiding this comment.
Only mildly related, but it enabled the code Arg1-- in one of the tests.
There was a problem hiding this comment.
No longer true - I've updated this. It contains all the tests you saw before, plus a few more (the ones that prompted me to make an update to the PR). I've also made it clearer which tests are derived from the uACPI ones, to make it easier if anyone ever wants to cross-reference between them.
|
Thanks for your patience @IsaacWoods. I've made my updates and it's ready for another look. The broad structure is still the same, but it deals more accurately with the string handling (where my understanding had been too shallow before) and implicit casting. As such the loop conditions are a bit different. |
2 participants
Since both Locals and Args are stored as references internally, a plain
unwrap_referencewas causing Locals that got passed to methods to be treated as though they had been passed by reference, instead of by value.This corrects that logic, and also takes into account the strange behaviour of the Windows NT interpreter - it treats strings passed via Locals into methods as always passed by reference, and never by value.
Fixes #313