Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Add raw module #4

Closed
wants to merge 1 commit into from
Closed

Add raw module #4

wants to merge 1 commit into from

Conversation

Ayush1325
Copy link
Contributor

This can be used in places where you need to integrate allocation with existing infrastructure (like in std), and thus do not want extra abstraction.

Signed-off-by: Ayush Singh ayushsingh1325@gmail.com

This can be used in places where you need to integrate allocation with existing infrastructure (like in std), and thus do not want extra abstraction.

Signed-off-by: Ayush Singh <ayushsingh1325@gmail.com>
@Ayush1325
Copy link
Contributor Author

I am thinking of completely removing asserts from the functions in raw since panic should not happen in some contexts. So rather than making debugging easier, it might make things more unpredictable.

@dvdhrm
Copy link
Member

dvdhrm commented Nov 14, 2022

I am thinking of completely removing asserts from the functions in raw since panic should not happen in some contexts. So rather than making debugging easier, it might make things more unpredictable.

Can you elaborate? Why wouldn't you want panics to happen in allocators?

Regarding the change: Is there a reason you prefer exporting raw interfaces rather than a type that implements GlobalAlloc?

@Ayush1325
Copy link
Contributor Author

Ayush1325 commented Nov 14, 2022

Can you elaborate? Why wouldn't you want panics to happen in allocators?

Inside rust-std (probably outside it as well), any errors due to memory allocation should use handle_alloc_error rather than panic. This doesn't mean allocators can never panic, however it is would be better if there are panic-independent raw functions.

Regarding the change: Is there a reason you prefer exporting raw interfaces rather than a type that implements GlobalAlloc?

Well, there are a couple of reasons:

  1. Rust has 2 traits for allocators currently: GlobalAlloc and Allocator. Both of these traits also have slightly different assumptions about the inputs and outputs. For example, their handling of Zero Sized Allocation. Some raw functions like malloc, which don't make these assumptions, would be useful.
  2. Inside the std, I am already storing SystemTable as static and thus don't have much reason to create a static object for the allocator. This makes a wrapper type essentially useless. I think Rust will probably optimize away the local struct I create, but still, it's not particularly useful.
  3. Having independent raw functions allows for having more descriptive function signatures. For example the alloc function can return Resutl<*mut u8, efi::Status> or something else if that makes more sense for UEFI.
  4. Having functions without panics.

@dvdhrm
Copy link
Member

dvdhrm commented Nov 25, 2022

Thanks for the information! I merged this! I also merged a bunch of follow-ups that clean up r-efi-alloc and prepare it for use outside of the test environments. The changes include:

  • alloc::Allocator now has alloc() and dealloc() methods which map to raw::{alloc, dealloc}.
  • alloc::Allocator only implements core::alloc::Allocator if the new feature-flag allocator_api is enabled.
  • The entire crate can now be built on stable, as long as allocator_api is not enabled as feature-flag.
  • global::Bridge now uses the direct alloc() and dealloc() calls on alloc::Allocator, allowing its use without allocator_api.

I will want to prepare a release next week, and probably one for r-efi as well, to get everything out to crates.io again.

Let me know if you have any comments! And thanks for looking into this!

Lastly, even though r-efi-alloc can be built on stable, using it still requires nightly since the allocation-panic handler is still unstable (rust-lang/rust#66741).

@dvdhrm dvdhrm closed this Nov 25, 2022
@Ayush1325 Ayush1325 deleted the raw branch November 25, 2022 15:57
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

2 participants