Allocator api - #329
Draft
SnowCheetos wants to merge 9 commits into
Draft
Conversation
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.
Contributor
|
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) |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
#306 (comment)
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.