Allow `aml_tester` to continue after an interpreter panic by martin-hughes · Pull Request #277 · rust-osdev/acpi

martin-hughes · 2026-03-17T13:57:39Z

The motivation here is to allow testing of large numbers of AML files - before this PR, a panic (probably from the Interpreter) would stop testing of all remaining files as aml_tester would exit.

It has evolved into a slightly larger PR than that, with a grab-bag of other minor updates listed below.

The main bit of this PR all flows from changes to run_test:

To allow testing to continue after a panic, I use catch_unwind.
If an unwind is caught, the Interpreter will not necessarily be in a valid state
We therefore don't want to re-use it for future tests
run_tests needs to take ownership of the interpreter so it can enforce this rule

This leads to a variety of parameter and return type changes throughout. (And explains why there's a bunch of new enums in the code)

Then I made a few other changes to improve consistency:

resolve_and_compile doesn't return io::Result any more - that level of detail isn't needed by the caller
This helped neaten up the output if there was a compilation error

Finally, because they were looking a bit messy:

I removed the %FN% interpolation - actually resolve_and_compile already relied on the iasl file-naming behaviour, so the only valid thing that could go in %FN% was what resolve_and_compile would already need. So it added no real value.
aml_tester::main now returns an exit code, so that if there are any test failures the user can see it reflected there. This might be useful for CI pipelines etc and reflects other testing tools (e.g. cargo test)

martin-hughes · 2026-03-17T14:02:23Z

The exit code change already showing its value by breaking the checks it seems!

Not sure why yet, I'll fix and update the PR - it works for me on my machine 👀

martin-hughes · 2026-03-17T14:20:44Z

Well I can see to_x.asl actually silently fails on at least PR #274 (CI tests are green despite test failure) - some testing with WSL suggests it's a difference in the output from different iasl versions.

WSL Ubuntu apt install acpica-tools version: 20200925 - generates to_x.aml that fails when run in aml_tester
Windows reasonably recent version directly downloaded: 20251212 - generates to_x.aml that succeeds.

I'm not sure where to go from there really.

martin-hughes · 2026-03-17T20:17:08Z

Alright, I've figured out the problem. iasl was folding out the calls to ToHexString in the optimizer, and different versions of iasl produced different results for that - the new one produces (e.g.) "0x7b" whereas the old one produces "0000007b".

So I've added the -oa flag to the call to iasl and now it doesn't do that folding - I think this is the correct flag, just in case any optimisation hides the test we're trying to do. (Not just the constant folding optimisations)

This requires a minor fix to ToHexString to match the expected upper case, but it has exposed a bug in ToInteger that I've raised #278 rather than fixing here.

martin-hughes · 2026-03-17T21:09:39Z

I've added the fix for #278 here to simplify testing it, let me know if you'd prefer it as a separate PR - I know this has a lot going on now... (Would close #278 if merged)

IsaacWoods

Thanks! This is a nice addition - one of my next goals with the library is to effectively eliminate panics, as a nice guarantee to users would be that the interpreter will never panic a kernel, but in the medium-term it would be nice to be able to recover from them in tests.

I've left a few comments with some refactoring nits, but otherwise this looks good. I'm fine with including the ToInteger changes here - thanks for working on that! It didn't occur to me that iasl would be applying optimisations (maybe when I looked it didn't do them if there have been changes in the last couple years?).

History wise, would it be possible to rebase to merge the first four commits into one? I think that makes intentions clearer with the intermediary changes eliminated.

IsaacWoods · 2026-03-21T14:00:19Z

tools/aml_test_tools/src/lib.rs


+/// The result of a call to [`run_test`]. This is a bit of a combination - it contains the actual
+/// result of the test, but also the interpreter - if it is still valid.
+pub enum RunTestResult<T>


Having these two enums split is a little confusing I think. I wonder if it would be cleaner to have a return type of (TestResult, Option<Interpreter>) from the relevant functions to replace RunTestResult??

We could then optionally merge TestFailureReason into this too (i.e. have multiple FailedX variants in here. We could always have a TestResult::is_failure method if needed. I do like including the AmlError if applicable though.

The reason I didn't go for the tuple approach is that the combined enum enforces the correct handling of the Interpreter. That is, it's not possible to get a (TestResult::Pass, None) or (TestResult::Panicked, Some(...)) result, so no code is needed to deal with it - not even an unwrap().

Just a personal style thing I guess - I'm a big fan of type-based enforcement of things - so if you prefer I'll take it out. Let me know if my argument sways you or not 😄

I don't feel too strongly about merging TestFailure reason into the enum - I guess if RunTestResult stays then it reduces duplication... I'll play it by ear based on what you say.

That seems fair enough on second look - I think this is fine!

IsaacWoods · 2026-03-21T14:04:46Z

tools/aml_test_tools/src/lib.rs

+                Object::Integer(other) => {
+                    error!("Test _MAIN returned non-zero exit code: {}", other);
+                    // TODO: wrong error - this should probs return a more complex err type
+                    Err(AmlError::NoCurrentOp)


I am not sure what we should use for this, but I feel leaving it like this could be confusing. I wonder if we should introduce an AmlError::HostError(String) (bikeshedding of name welcome) for this + errors that occur during handler operations?

Yes - good shout. As you can see from the comment, totally meant to do something like this! My original thought was something more complex, but I think you've got a good solution.

IsaacWoods · 2026-03-21T14:04:55Z

tools/aml_test_tools/src/lib.rs

+                _ => {
+                    error!("Test _MAIN returned unexpected object type: {}", *result);
+                    // TODO: wrong error
+                    Err(AmlError::NoCurrentOp)


Same as above.

martin-hughes · 2026-03-21T20:56:28Z

Cheers Isaac. Let me know about RunTestResult - I'll deal with it whichever way you say, then deal with squashing the first 4 commits later.

one of my next goals with the library is to effectively eliminate panics, as a nice guarantee to users would be that the interpreter will never panic a kernel, but in the medium-term it would be nice to be able to recover from them in tests.

Good plan, that'd be a nice guarantee to have - I guess most of the todo could be replaced by AmlError::LibUnimplemented or similar?

O/T really, but some good news: from a play with the uACPI tests and a not-yet-ready update to aml_tester it only looks like there's a couple of todo that get hit regularly. From memory I don't think I saw any of the unwrap calls get triggered.

Fix accidentally broken tests Exit with a failure code if needed This makes it easier for any user to see if a test has failed. Which may be useful for example in CI environments. Remove AML filename replacement `resolve_and_compile` relies on the current `iasl` way of naming its output, there's no need to duplicate that in `create_script_file`

This was masking some bugs in `ToHexString` and `ToInteger` because those calls were being folded out by the iasl optimizer.

martin-hughes · 2026-03-22T11:57:57Z

Cheers Isaac. First 4 commits squashed, so should be good to go 🤞

martin-hughes mentioned this pull request Mar 17, 2026

ToInteger doesn't handle all expected cases. #278

Closed

IsaacWoods requested changes Mar 21, 2026

View reviewed changes

martin-hughes added 4 commits March 22, 2026 11:56

Disable iasl optimisations for tests

b880f70

This was masking some bugs in `ToHexString` and `ToInteger` because those calls were being folded out by the iasl optimizer.

Fix ToInteger handling

93d46d2

A more appropriate test failure error

92db67c

martin-hughes force-pushed the aml-tester-tolerate-panic branch from 2a1f120 to 92db67c Compare March 22, 2026 11:56

martin-hughes requested a review from IsaacWoods March 22, 2026 11:58

IsaacWoods approved these changes Mar 22, 2026

View reviewed changes

IsaacWoods merged commit a516e3c into rust-osdev:main Mar 22, 2026
5 checks passed

martin-hughes deleted the aml-tester-tolerate-panic branch March 22, 2026 19:59

Uh oh!

Conversation

martin-hughes commented Mar 17, 2026

Uh oh!

martin-hughes commented Mar 17, 2026

Uh oh!

martin-hughes commented Mar 17, 2026

Uh oh!

martin-hughes commented Mar 17, 2026

Uh oh!

martin-hughes commented Mar 17, 2026

Uh oh!

IsaacWoods left a comment

Choose a reason for hiding this comment

Uh oh!

IsaacWoods Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

martin-hughes Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

IsaacWoods Mar 22, 2026

Choose a reason for hiding this comment

Uh oh!

IsaacWoods Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

martin-hughes Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

IsaacWoods Mar 21, 2026

Choose a reason for hiding this comment

Uh oh!

martin-hughes commented Mar 21, 2026

Uh oh!

martin-hughes commented Mar 22, 2026

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

2 participants