Prevent some infinite recursion and loops - #352
Conversation
There was a problem hiding this comment.
The actual tests are the same, but now the "shoulds" have become "musts".
| Panicked, | ||
| } | ||
|
|
||
| impl<T> std::fmt::Debug for RunTestResult<T> |
There was a problem hiding this comment.
Needed to allow RunTestResult to be checked in assert_matches
| kind: BlockKind::Method { method_scope: scope.clone() }, | ||
| }; | ||
| let block = | ||
| Block { stream: code.clone(), pc: 0, kind: BlockKind::Method { method_scope: scope.clone() } }; |
There was a problem hiding this comment.
This is a pure formatting change - I'm not sure why the previous version was unacceptable to rustfmt. Maybe it's a new-nightly thing?
|
Whoever merges this PR: it will interact with #345. The second of the two to be merged will need updating to handle the first. |
The motivation is to help stop AML causing a thread (or the system as a whole) to become deadlocked. The first protection adds a maximum method call stack depth. This has been chosen arbitrarily, for now. It does not protect against all possible ways of recursing infinitely. For example, it does not stop recursive table loads. (Although LoadTable is not currently supported) The second protection is a straightforward timeout for while loops, with associated tests. These are both based on the uACPI protections, although they differ in a key way: in uACPI, the method stack is unwound. If a table definition is reached, execution continues with the next statement, but the execution is still flagged as failed. In this crate, execution is stopped with an error.
d4b3c42 to
db22136
Compare
|
Rebased on top of the updated |
IsaacWoods
left a comment
IsaacWoods
left a comment
There was a problem hiding this comment.
Nice, thanks for working on this! I think both limits reasonable - wondered between max loop iterators vs timeout, but I think total time spent in the loop is a better metric for something stuck.
Whether anything reasonable (in the land of AML) would require stack depths >1000... who knows. I'd hope not, so let's go with this initially.
| Ok(PciAddress::new(seg as u16, bus as u8, device as u8, function as u8)) | ||
| } | ||
|
|
||
| /// Return to the beginning of a While loop - either because `Continue` was executed or |
|
|
||
| fn stall(&self, microseconds: u64) { | ||
| // There's no `std` equivalent to stall, and sleep is probably OK for a test environment. | ||
| sleep(Duration::from_micros(microseconds / self.scale_factor)); |
There was a problem hiding this comment.
For testing, I don't think this matters too much and can't remember the details (or when you start worrying; but I do think <1ms is probably about there) but Windows in particular is very bad at short waits.
Potentially the right way to do this is via something like the spin_sleep crate, which manually uses higher-precision timers if available, or alternatively spins for short sleeps that are under the native timer's precision.
There was a problem hiding this comment.
Good shout - I'll bear this in mind if it becomes a problem!
I think Windows rounds delays up to at least a scheduler period because it yields the thread, and the timer resolution is a handful of millis in any case. I think in test land it'll probably be OK unless we're getting down to debugging some kind of race condition.
2 participants
The motivation is to help stop AML causing a thread (or the system as a whole) to become deadlocked.
The first protection adds a maximum method call stack depth. This has been chosen arbitrarily, for now. It does not protect against all possible ways of recursing infinitely. For example, it does not stop recursive table loads. (Although LoadTable is not currently supported)
The second protection is a straightforward timeout for while loops, with associated tests.
These are both based on the uACPI protections, although they differ in a key way: in uACPI, the method stack is unwound. If a table definition is reached, execution continues with the next statement, but the execution is still flagged as failed. In this crate, execution is stopped with an error.
Notes:
unwrap_methods, so I felt that was worth leaving for a separate PR.