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

Change usage example for statically allocated heap #53

Merged
merged 1 commit into from
Feb 12, 2022

Conversation

nviennot
Copy link
Contributor

@nviennot nviennot commented Feb 6, 2022

The linker needs to know how big the heap is, otherwise, it could place some of the global variables in the heap region.
This commit changes the example code to do that.

The linker needs to know how big the heap is, otherwise, it could place
some of the global variables in the heap region.
@rust-highfive
Copy link

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @therealprof (or someone else) soon.

Please see the contribution instructions for more information.

@adamgreig
Copy link
Member

cortex-m-rt's heap_start() is guaranteed to return a heap start address that's after all other allocated objects (after .bss, .data, and .uninit sections); you can see how that's done in the linker script. So, I don't think there's a possibility for the compiler to place some static objects in the heap region.

However, nothing prevents the user specifying a heap that's too large, whereas your example will ensure the heap space is statically allocated and known about by the linker, which I think is basically better in every way. Probably heap_start() in cortex-m could even be removed; I don't know when it would ever make sense to use that instead of statically allocating a heap like this. Perhaps if the heap size is unbounded and you want to let it grow upwards and just hope it never hits the stack, but that seems pretty foolish...

Could you update the example to move use core::mem::MaybeUninit out of the function and with the other use statements, though? And it might be worth adding a comment to explain that you're statically reserving some memory for the heap.

If anyone else @rust-embedded/cortex-m has thoughts about this please shout out though... I think it's mostly just that MaybeUninit and Rust's general concept of uninitialised memory was much less well developed when this was first written.

@thejpster
Copy link
Contributor

I think this also comes from the classic UNIX approach of "heap goes up, stack comes down" and you use the "sbrk" (Stack Break) syscall to move the dividing line between them.

If we don't have that flexibility, then yes, the heap might as well be a static.

@adamgreig
Copy link
Member

I was just about to merge this but I wonder if the example would be clearer with HEAP.len() instead of HEAP_SIZE? Honestly the init method should probably just take a slice or something, since MaybeUninit exists now... so I guess this doesn't really matter.

Anyway, thanks for the PR!

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 12, 2022

👎 Rejected by too few approved reviews

Copy link
Member

@adamgreig adamgreig left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

bors r+

@bors
Copy link
Contributor

bors bot commented Feb 12, 2022

@bors bors bot merged commit a1be51e into rust-embedded:master Feb 12, 2022
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

5 participants