-
Notifications
You must be signed in to change notification settings - Fork 58
Add a macro to make initialization easier #103
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
base: master
Are you sure you want to change the base?
Conversation
6bca433
to
6335bb8
Compare
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds good to me, thanks! Just a bit of nitpicks and it should be mergable.
9951db6
to
080581e
Compare
@zeenix I have made improvements based on the suggestions, and I added a |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
LGTM, thanks!
@JalonWong I'll merge soon but I want to first give others a chance to have a look. CC @adamgreig @sgued |
My only thought is that people are likely to want to configure where the backing store for the heap is located in memory using
|
How about a combination of these two: we add the optional argument for the linker section and if people need to add other attributes, they just don't use the macro? |
src/lib.rs
Outdated
unsafe { | ||
$heap.init(&raw mut HEAP_MEM as usize, $size) | ||
} |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This macro should not generate an unsafe
block, unsafe
should be required at the call site. With the current state, it's possible to cause UB without using unsafe
:
#[global_allocator]
static HEAP: Heap = Heap::empty();
#[entry]
fn main() -> ! {
{ embedded_alloc::init!(HEAP, 1024); }
let data = Box::new(1);
{ embedded_alloc::init!(HEAP, 1024); }
// Here we drop the `data` box. Its pointer points to the first heap,
// but this will try to drop it with the second heap.
drop(data);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You did not fix the issue. Multiple invocations of the macro will generate multiple versions of the static
so the init
call can run multiple times.
Using Atomics also prevents from compiling on some platforms that could use this crate.
I don't see a way other than not including unsafe
in the generated code and require the user to wrap the macro in unsafe:
unsafe {
embedded_alloc::init!(HEAP, 1024);
}
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
At present, I only think of a way to ensure it is called once at runtime, which is using an atomic. Is it appropriate?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think Rust provides a safe way to ensure that efficiently. One way would be to use a global static (defined once in embedded-alloc
, not in the code generated by the macro), and check/update it with critical_section. The other way would be to check that the heap is not already allocated using APIs currently not provided by the upstream implementations.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
As your suggestion, I added a call_once
function. It uses a global static critical_section::Mutex
to ensure it is called once.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Multiple invocations of the macro will generate multiple versions of the static so the init call can run multiple times.
Using Atomics also prevents from compiling on some platforms that could use this crate.
I'm not sure why we're being very strict here. The macro is only replicating code that we currently recommend people to write. I think we mainly just need to document all the possible caveats and safety concerns so users are aware. The macro really shouldn't be making things expensive+complicated with atomics IMO.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If UB can be reached from user code without the user having unsafe
anywhere, the macro is unsound.
So either the unsafe
must be removed from the generated block and the user should wrap the macro call in unsafe
themselves, or the macro should be made fully safe.
(And I think for this case it makes more sense to require downstreams to use unsafe
to use that macro).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If UB can be reached from user code without the user having
unsafe
anywhere, the macro is unsound.
I think it's more a Rust limitation that it's not possible to have unsafe macros. However, what you propose sounds good to me. User having to unsafe { }
wrap the macro call, isn't a lot to ask for and they'd have to do that if this was a function (because it would be an unsafe fn
). I just think we don't need to bring atomics and other complications here.
6d4171b
to
1e17f59
Compare
@zeenix @adamgreig If someone want to use |
f447854
to
d47bd8a
Compare
No description provided.