Allocator api by SnowCheetos · Pull Request #329 · rust-osdev/acpi

SnowCheetos · 2026-08-31T02:00:39Z

Long overdue 😅 this is an initial attempt at breaking down the monstrosity in #306. I am keeping this as a draft just in case I need to break it down further.

@IsaacWoods @martin-hughes care to take a look when you got the time? If the size is good I could do the rest in similar manners.

Fix: Add allocator to registers across 2 files. ### Changes - [`src/lib.rs`] Support `aml` feature: Enable the `aml` feature in the library - [`src/platform/mod.rs`] Add allocator to registers: Use the provided `allocator` for `registers` to avoid potential memory issues ### Version Bumps - [`Cargo.toml`] Rust / Cargo: 6.1.1 -> 6.1.2 (patch); code changes detected; suggest a patch project version bump ### Risk - Level: low

### Changes - [`src/platform/numa.rs`] Use &AcpiTables: Allow passing `AcpiTables` as a reference to the `new` method ### Risk - Level: low

This commit introduces `AmlString` for string handling and parses `_SI` within the `namespace` to improve string manipulation capabilities. ### Changes - [`src/aml/mod.rs`] Use AmlString for string handling: Introduce `AmlString` for safer string handling and improve error reporting - [`src/aml/namespace.rs`] Parse `_SI` in namespace: Allow parsing of `_SI` in namespaces using `AmlName::parse_in` for improved flexibility - [`src/aml/mod.rs`] Implement push method in AmlString: Add `push` method to `AmlString` to support allocator-parameterized string manipulation - [`src/aml/mod.rs`] Use Allocator for generic resource handling: Update `PciRouteType` to use `Allocator` for generic resource handling - [`src/lib.rs`] Enable allocator features: Enable `btreemap_alloc` and `allocator_api` for better memory management - [`tests/bank_fields.rs`] Update test infrastructure and files - [`tools/aml_test_tools/src/handlers/check_cmd_handler.rs`] Use Global allocator for interpreter: Use the global allocator for the interpreter to improve performance and avoid allocation issues ### Version Bumps - [`Cargo.toml`] Rust / Cargo: 6.1.2 -> 6.2.0 (minor); manifest changed without an explicit project version bump; suggest minor bump - [`tools/aml_test_tools/Cargo.toml`] Rust / Cargo: 0.1.0 -> 0.2.0 (minor); code changes detected; suggest a minor project version bump ### Risk - Level: low - No security risks are introduced by using `AmlString` and parsing `_SI`.

This commit introduces `MethodContext` to provide method arguments, improving code clarity and maintainability. ### Changes - [`src/aml/mod.rs`] Use MethodContext for method arguments: Use `MethodContext` to correctly pass method arguments to the interpreter - [`src/platform/interrupt.rs`] Add `hw_id` to Gic struct - [`tests/bank_fields.rs`] Update imports: Refactor imports to align with updated dependencies and improve code clarity - [`tools/aml_test_tools/src/handlers/check_cmd_handler.rs`] Prevent null check when handler is null: Ensure the `check_cmd_handler` function doesn't panic when passed a null handler ### Risk - Level: low - No new security risks introduced. - No data loss or corruption is expected.

…sourceDescriptor Update formatting of `AmlString` and add IRQ information to `ResourceDescriptor` in `aml` module ### Changes - [`src/aml/mod.rs`] Update AmlString formatting: Change `AmlString` to use `A: Allocator + Clone` for formatting, improving flexibility and avoiding dynamic context info loss - [`src/aml/resource.rs`] Add IRQ information to ResourceDescriptor - [`src/lib.rs`] Remove unused comment: Remove a comment that discusses unused code and potential future changes ### Version Bumps - [`Cargo.toml`] Rust / Cargo: 6.2.0 -> 6.2.1 (patch); manifest changed without an explicit project version bump; suggest patch bump ### Risk - Level: low - No security implications. - Formatting changes are for code consistency. - IRQ information is for debugging and monitoring purposes.

# Conflicts: # src/aml/mod.rs # src/aml/object.rs # src/aml/resource.rs # tools/aml_test_tools/src/lib.rs

### Changes - [`.gitignore`] Add `.DS_Store` and .rs.bk to: Exclude unnecessary files from Git tracking - [`src/aml/mod.rs`] Use Global allocator for Interpreter: Change `Interpreter`'s default allocator to `Global` for better portability and performance ### Risk - Level: low - No immediate risk identified. - Global allocator usage is generally safe. - Potential for increased memory usage if not managed carefully.

The AML interpreter allocated exclusively through the global allocator, so a host that manages its own memory (or has no global allocator at all) could not use it. Parameterise the interpreter's own storage over `Allocator` so that allocator can be supplied instead. `Interpreter`, `Object`, `Namespace`, `AmlName` and `OpRegion` gain an allocator parameter defaulting to `Global`. Every existing constructor keeps its meaning and an `_in` counterpart takes the allocator explicitly, so callers that do not care are unaffected: `Interpreter::new`, `Namespace::new`, `AmlName::root`, `AmlName::from_name_seg`, `Object::wrap` and the `FromStr` impl all still resolve to the global allocator. The test suite is unchanged by this commit, which is the intended evidence that the existing API still works. `String` is not parameterised over an allocator, so AML string objects move to `AmlString` (aml::string), a `Vec<u8, A>` newtype holding a UTF-8 invariant. It provides `from_utf8_lossy_in`, since `String::from_utf8_lossy` would otherwise reintroduce a global allocation when converting a buffer to a string. Three things deliberately keep the global allocator, because they are handed to the interpreter or returned to the caller rather than being interpreter storage: - `AmlError`, which already allocates upstream (`String` payloads built with `alloc::format!`, and names held as `Vec<NameComponent>`). Parameterising it spread `A` across every signature in the crate for no gain, so errors copy any name they report via `AmlName::to_global`. Making errors allocation-free is worth doing, but it is a change to what they carry, not to who allocates them, and it belongs with the wider `AmlError` rework rather than here. - `PciRoutingTable`, whose public API is unchanged. - `FixedRegisters`, which arrives from `AcpiPlatform` already allocated. `Allocator` is required to be `Clone` rather than borrowed. A caller who cannot clone their allocator can instantiate these types with `&MyAlloc`, which implements `Allocator` through the blanket impl in `core::alloc` and is `Copy`, giving the allocator the lifetime of the interpreter without a lifetime parameter on every type. The obligation that clones behave as one allocator is already imposed by the `Allocator` safety contract, which names a misbehaving `Clone` as a violation. The previous `'static` bound is dropped, since it would have ruled that out.

martin-hughes · 2026-09-01T13:39:26Z

I'm looking - it's going to take me a while to get through it all though! Since I only get chunks of time here and there, it might need a few days. Bear with me.

At some point this will need rebasing to deal with the conflicts, but personally I'd prefer you to wait until I've reviewed it fully, so that I can check the rebase diff just once. (Hopefully that suits you too @IsaacWoods)

martin-hughes · 2026-09-01T20:41:51Z

I'm working on my review, but I have a question nearly straight away...

Is the objective to be able to use the whole Interpreter without a global allocator? (that is, to allow separation of the alloc and aml features)

I ask partly because it looks as though AmlError is tied to the Global allocator, but in #158 you mention wanting to strictly control the crate's memory usage.

Separately, to me this PR looks more sensible to review - large, but at least without the unrelated changes that are in #306. I'm not sure there'd be a way to make it more focussed. It'll take me a while, but I can manage - although I'm certain Isaac will want to look too!

Thanks again for continuing to work on it.

SnowCheetos · 2026-09-02T17:28:41Z

I'm working on my review, but I have a question nearly straight away...

Is the objective to be able to use the whole Interpreter without a global allocator? (that is, to allow separation of the alloc and aml features)

I ask partly because it looks as though AmlError is tied to the Global allocator, but in #158 you mention wanting to strictly control the crate's memory usage.

Separately, to me this PR looks more sensible to review - large, but at least without the unrelated changes that are in #306. I'm not sure there'd be a way to make it more focussed. It'll take me a while, but I can manage - although I'm certain Isaac will want to look too!

Thanks again for continuing to work on it.

Whoops I just read the second half without realizing there is a question. Yes eventually that is the goal, but obviously that is not something I should fully decide. AmlError wise, I kinda just left it alone in this PR since I see there's an effort on largely redesigning it.

IsaacWoods · 2026-09-02T19:29:24Z

Thanks for working on this and for the new PR. I'll close #306 as this supersedes that.

I feel some of that feedback re our ability to review still applies:

Commit messages are still quite confusing and don't align very well with changes. This is a PR I'd like to merge with multiple commits (I'd usually squash and rewrite commit messages myself if I don't like history, to be honest).
There are merge commits in the branch. It's better to rebase a feature branch onto the base branch
Your tool is still making semver changes for every commit. This is superfluous.

Quick thoughts on the content:

I think using string_alloc instead of rolling an AmlString ourselves would be better. It's a shame it's needed at all but that's out of our control.
AmlError may be reworked in the future but this PR should still do the work to parameterise it over A if the intention is to not use Global by default.

IsaacWoods · 2026-09-02T19:30:21Z

-                line!(),
-            )));
+            return Err(AmlError::InternalError(
+                concat!("Operation has invalid argument types at ", file!(), ":", line!()).to_string(),


Losing the arguments is a really unfortunate debugging regression here - is there no way to stringify them?

IsaacWoods · 2026-09-02T19:30:31Z

@@ -66,47 +61,56 @@ use op_region::{OpRegion, RegionHandler, RegionSpace};
 use pci_types::PciAddress;
 use spinning_top::Spinlock;

-/// Helper macro to extract an expected set of [`Argument`]s from the given [`OpInFlight`]. Use


Why is this removed?

That was a brute copy paste went south, I will restore it.

IsaacWoods · 2026-09-02T19:37:10Z

@@ -2444,7 +2562,9 @@ where
                    }
                }
            }
-            Object::Debug => self.handler.handle_debug(&object),
+            Object::Debug => {
+                // TODO: Route Debug stores through Handler once Handler can accept allocator-aware objects.


This is a significant regression - is the plan to parameterise Handler with an allocator in the future? Why not in this PR?

I ask partly because that's an important architectural decision - if Handler needs to know about the allocator, could it potentially provide it as an associated type? This may well be a cleaner solution to providing A to many types that already interact with Handler?

Yeah, that's partially another in-house concept that made it into this PR. More on that, yes, in my use case I borderline handed the Handler an Allocator, I couldn't do that exact form because of unique constraints. But I didn't even think about an associated type, I could experiment with it.

IsaacWoods · 2026-09-02T19:40:52Z

        match level.values.insert(last_seg, (ObjectFlags::new(true), object)) {
            None => Ok(()),
-            Some(_) => Err(AmlError::NameCollision(path)),
+            Some(_) => Err(AmlError::NameCollision(path.to_global())),


Do these convert from the allocator the user wants to just use Global because AmlError cannot use the allocator??? This feels like the PR doesn't do what it intends to at this stage?

IsaacWoods · 2026-09-02T19:41:53Z

+
+// only Clone is derived. PartialEq/Debug get manual impls
+// below to avoid the derive macro's auto-added `A: PartialEq` / `A: Debug`
+// bounds - `&'static BumpArena<N>` satisfies neither, and the bounds aren't


Is BumpArena a type in your crate? It probably shouldn't be mentioned in our documentation?

Yeah, my bad. I will go through and remove all of these stale docstrings

IsaacWoods · 2026-09-02T19:43:41Z

-            })
-            .trim_end_matches('.')
-            .to_string()
+    pub fn root_in(alloc: A) -> AmlName<A> {


There's a lot of cruft in this PR's impact where things are made a lot less clear with allocator cloning. I wonder if sites like this (across the crate) could take a &A and if needed clone at the use-site? I don't know if that would be preferable - just an idea to try?

Also - if there is no version that takes Global, I wonder if we can lose the _in just to shorten names

martin-hughes

Thanks again @SnowCheetos. Please take note of my general comments as well, they're written in a spirit of personal development. You're clearly a talented & motivated developer, and I think they would help you improve your practice.

General comments:

Isaac's review overlapped mine in time. I haven't deleted any comments where there's overlap, so you may see some duplication.
It's very confusing to see the commit history from #306 in this PR. It'd be better to have squashed or rebased those out of the way before opening this PR.
There are signs that you may have relied too heavily on AI to review the code without doing a thorough review yourself. For example, the references to BumpArena or the inconsistent comment line lengths. This is somewhat frustrating given that it was mentioned before, and the amount of time Isaac and I invest in reviewing the code.
- If this is not the case then I apologise profusely!
- I'm not averse to AI assisted development - far from it, it is a very useful tool - but it's a tool and you are the ultimate authority, not it.
Be careful including unrelated changes. I flagged a couple - they are probably good changes, but we've both indicated that we'd like a tightly scoped PR (due to the size of the change), so it gives a bad impression.
I feel less strongly than Isaac about AmlError - this PR is a step towards a full allocator aware Interpreter - but it does feel weird to not include it given your objectives. Personally I'd have mentioned it as an explicit exclusion in your pull request summary as well as the commit message

martin-hughes · 2026-09-01T20:12:42Z

Why create a new type instead of using string_alloc?

You did mention it didn't ya... my gold fish memory forgot about it 😅

That being said though, I was in a 'crate scare' for the past few weeks where https://rustsec.org/advisories/RUSTSEC-2026-0260 almost made it into a project through very legitimate crate dependencies (saved by the lock file, otherwise it'd have ran the build).

Nonetheless string_alloc seems to be a better option indeed. Is there a specific version you'd prefer we pin?

I wonder when nightly's native string will finally become allocator backed, that would be the best.

martin-hughes · 2026-09-01T20:21:50Z


-        Ok(())
+        fn push_utf8_lossy_until_nul<A2: Allocator + Clone>(target: &mut AmlString<A2>, bytes: &[u8]) {
+            let mut remaining = bytes.split(|byte| *byte == b'\0').next().unwrap_or_default();


Why does this go until NULL?

I ask because Rust strings are not null-terminated, and from a glance it looks like AmlString is not null-terminated either.

martin-hughes · 2026-09-01T20:26:07Z

@@ -3547,6 +3753,8 @@ pub enum AmlError {
    InternalError(String),
 }

+


nitpick: no need to add blank lines

martin-hughes · 2026-09-01T20:29:48Z

@@ -2444,7 +2562,9 @@ where
                    }
                }
            }
-            Object::Debug => self.handler.handle_debug(&object),
+            Object::Debug => {
+                // TODO: Route Debug stores through Handler once Handler can accept allocator-aware objects.


What work is needed here? I don't feel comfortable breaking something that currently works.

martin-hughes · 2026-09-01T20:32:25Z

    ByteData(u8),
    DWordData(u32),
    TrackedPc(usize),
    PkgLength(usize),
 }

-impl OpInFlight {
+// Manual Debug impl - derive auto-adds `A: Debug` which `&'static BumpArena<N>`


What is a BumpArena? I suspect that might be from one of your other projects...

martin-hughes · 2026-09-03T09:02:14Z

+        f.debug_struct("FieldUnit")
+            .field("flags", &self.flags)
+            .field("bit_length", &self.bit_length)
+            .finish_non_exhaustive()


Why not include the other two fields like the derived Debug trait would have?

martin-hughes · 2026-09-03T09:03:27Z

+        match self {
+            Self::Normal { .. } => f.write_str("Normal { .. }"),
+            Self::Bank { .. } => f.write_str("Bank { .. }"),
+            Self::Index { .. } => f.write_str("Index { .. }"),


Similarly here, this seems to lose a lot of info the previous derived Debug would have included

martin-hughes · 2026-09-03T09:07:45Z

-                    &handler,
-                )
-                .unwrap()
+    let fake_registers = Arc::new_in(


No need to use new_in with Global.

martin-hughes · 2026-09-03T09:07:58Z

        handler: handler.clone(),
    };
-    Interpreter::new(handler, 2, fake_registers, Some(fake_facs_mapping))
+    Interpreter::new_in(handler, 2, fake_registers, Some(fake_facs_mapping), Global)


Same again.

martin-hughes · 2026-09-03T09:08:39Z

 pub fn run_test_for_string<T>(
    asl: &'static str,
-    interpreter: Interpreter<T>,
+    interpreter: Interpreter<T, Global>,


Removing the Global throughout this file would be a good test that you haven't changed the interface for people who don't want to use allocators.

SnowCheetos · 2026-09-03T16:38:37Z

@martin-hughes @IsaacWoods

Thanks for the thorough reviews. My bad for the persistent slop comments and commits, I will try take care of them over the weekend.

SnowCheetos · 2026-09-03T17:20:57Z

@martin-hughes @IsaacWoods

Thanks for the thorough reviews. My bad for the persistent slop comments and commits, I will try take care of them over the weekend.

I do want to elaborate on one design decision that seems to be a common topic of friction here - ::new_in() vs ::new(). I think I really should have clarified my final intent instead of just leaving them around for you guys to get confused by (and please correct me if I missed any important details that'd make this not feasible):

Is the objective to be able to use the whole Interpreter without a global allocator? (that is, to allow separation of the alloc and aml features)

@martin-hughes you caught it, the answer is yes. Most dependents will still want Global working out of the box, but I think a long term scalable way to manage that is not to default Global in the instantiations but to thread <Allocator> all the way through, only with global defaults at the top level. These new_in()s are more so aspirational since it's a lot easier to plug in the global allocator here than the other way around.

This gets philosophical at the boom of it - I personally believe explicit allocator passing is going to become mainstream, and systems programming is one area that could benefit massively from it (I assume that is the reason for bringing in allocator_api in the first place here, right?). What are you guys' thoughts?

SnowCheetos · 2026-09-03T17:31:39Z

Thanks again @SnowCheetos. Please take note of my general comments as well, they're written in a spirit of personal development. You're clearly a talented & motivated developer, and I think they would help you improve your practice.

General comments:

Isaac's review overlapped mine in time. I haven't deleted any comments where there's overlap, so you may see some duplication.

It's very confusing to see the commit history from Make AML interpreter storage allocator-api compatible #306 in this PR. It'd be better to have squashed or rebased those out of the way before opening this PR.

There are signs that you may have relied too heavily on AI to review the code without doing a thorough review yourself. For example, the references to BumpArena or the inconsistent comment line lengths. This is somewhat frustrating given that it was mentioned before, and the amount of time Isaac and I invest in reviewing the code.

If this is not the case then I apologise profusely!

I'm not averse to AI assisted development - far from it, it is a very useful tool - but it's a tool and you are the ultimate authority, not it.

Be careful including unrelated changes. I flagged a couple - they are probably good changes, but we've both indicated that we'd like a tightly scoped PR (due to the size of the change), so it gives a bad impression.

I feel less strongly than Isaac about AmlError - this PR is a step towards a full allocator aware Interpreter - but it does feel weird to not include it given your objectives. Personally I'd have mentioned it as an explicit exclusion in your pull request summary as well as the commit message

Very insightful, I certainly needed that! If you have time, would you care to take a peek at some of my projects? Could certainly use some of your expertise in this field and best practices! (only if time/effort permits, no pressure, ofc)

SnowCheetos added 9 commits May 11, 2026 23:07

style(platform): use &AcpiTables

841c15e

### Changes - [`src/platform/numa.rs`] Use &AcpiTables: Allow passing `AcpiTables` as a reference to the `new` method ### Risk - Level: low

Merge remote-tracking branch 'upstream/main'

78b539f

# Conflicts: # src/aml/mod.rs # src/aml/object.rs # src/aml/resource.rs # tools/aml_test_tools/src/lib.rs

fix(aml): balance global lock mutex

559b4a2

SnowCheetos marked this pull request as ready for review September 1, 2026 21:08

IsaacWoods mentioned this pull request Sep 2, 2026

Make AML interpreter storage allocator-api compatible #306

Closed

IsaacWoods reviewed Sep 2, 2026

View reviewed changes

martin-hughes requested changes Sep 3, 2026

View reviewed changes

		@@ -3547,6 +3753,8 @@ pub enum AmlError {
		InternalError(String),
		}

Uh oh!

Uh oh!

Conversation

SnowCheetos commented Aug 31, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

martin-hughes commented Sep 1, 2026

Uh oh!

martin-hughes commented Sep 1, 2026

Uh oh!

SnowCheetos commented Sep 2, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

IsaacWoods commented Sep 2, 2026

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

martin-hughes left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

SnowCheetos commented Sep 3, 2026

Uh oh!

SnowCheetos commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

SnowCheetos commented Sep 3, 2026 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Reviewers

Assignees

Labels

Projects

Milestone

Development

Uh oh!

3 participants

SnowCheetos commented Aug 31, 2026 •

edited

Loading

SnowCheetos commented Sep 2, 2026 •

edited

Loading

SnowCheetos commented Sep 3, 2026 •

edited

Loading

SnowCheetos commented Sep 3, 2026 •

edited

Loading