Automatically size tasks #595
Conversation
mkeeter
added
developer-experience
mkeeter
force-pushed
the
autosize-build-redux
branch
from
3c003a5
to
04eed99
Compare
| t => panic!("Unknown mpu requirements for target '{}'", t), | ||
| }; | ||
|
|
||
| let power_of_two_required = toml.mpu_power_of_two_required(); |
There was a problem hiding this comment.
Given that this routine is already consulting the MPU requirement data, could you move the computation of align into here, rather than consulting the MPU requirement data outside this function and then calling it?
There was a problem hiding this comment.
I don't quite understand this suggestion, since this function is called after tasks are placed in memory (which is the alignment-sensitive part).
I think the right fix may be to remove the task power-of-two check from this function entirely, since autosizing should enforce MPU requirements! (This would also address your comment below about checking flash / ram but not any other memories...)
build/xtask/src/dist.rs
Outdated
| if power_of_two_required && !task.requires["flash"].is_power_of_two() { | ||
| panic!("Flash for task '{}' is required to be a power of two, but has size {}", task.name, task.requires["flash"]); | ||
| if power_of_two_required | ||
| && !task_allocations[name]["flash"].len().is_power_of_two() |
There was a problem hiding this comment.
This is not a new bug, but: these conditions need to be applied to all regions, not just "flash" and "ram". For instance the sram1 area used by the ethernet driver still needs to be pow2.
build/xtask/src/sizes.rs
Outdated
| .map(|(name, range)| (name, range.end - range.start)) | ||
| .collect(); | ||
|
|
||
| // XXX: are there alignment issues with the stack here? |
There was a problem hiding this comment.
Yes, the stack is 8-byte aligned on all of our current targets -- so, technically, this could underestimate RAM usage by 7 bytes in corner cases.
|
I'm playing around with this branch, and I'm confused by the new xtask sizes output. (Yeah, I know, I read the code, I should probably understand it.) What's this mean? 12608 is 77% of 16384, so this is fine. (The yellow numbers used to indicate cases where space is being wasted; this is not one.) Is it suggesting that the |
|
Okay, how about this: The use case for Changes I'm suggesting there:
Thoughts? |
build/xtask/src/sizes.rs
Outdated
| for (mem, &used) in sizes.iter() { | ||
| let size = match requires.get(&mem.to_string()) { | ||
| Some(s) => *s, | ||
| _ => continue, |
There was a problem hiding this comment.
this kind of thing is the one time I wish Rust had Ruby like semantics for closures; this kind of code has to be written as a match rather than as a method chain because you can't continue into the parent scope. oh well :)
| panic!("Unknown target: {}", toml.target); | ||
| /// Loads the size of the given task (or kernel) | ||
| pub fn load_task_size<'a>( | ||
| toml: &'a Config, |
There was a problem hiding this comment.
I am not sure i prefer toml as a name, given that the old name was config because it's a Config, but I don't feel strongly enough to argue that you should change it back :)
There was a problem hiding this comment.
yeah, this is a little awkward, but matches the naming in dist.rs (where config is a BuildConfig and toml is the Config inside it 😱 )
| }; | ||
|
|
||
| let check_task = move |name: &str, |
There was a problem hiding this comment.
i am glad this monstrosity is leaving, haha. i almost wanted to turn it into a free function, but it captured one or two variables and that was convenient.
|
|
||
| // If the VirtAddr disagrees with the PhysAddr, then this is a | ||
| // section which is relocated into RAM, so we also accumulate | ||
| // its FileSiz in the physical address (which is presumably |
There was a problem hiding this comment.
is FileSiz a typo? Shouldn't it be "file size" or FileSz or something? an extremely minor thing.
There was a problem hiding this comment.
heh, the names in this section based on the output of readelf, so it's intentional
Program Headers:
Type Offset VirtAddr PhysAddr FileSiz MemSiz Flg Align
LOAD 0x000120 0x08039400 0x08039400 0x00060 0x00060 R E 0x20
LOAD 0x000180 0x08039460 0x08039460 0x00020 0x00020 R E 0x20
GNU_STACK 0x000000 0x00000000 0x00000000 0x00000 0x00000 RW 0x0
81706ba
to
7b6cf9d
Compare
|
Alright, I've refactored the Here's an example: |
mkeeter
force-pushed
the
autosize-build-redux
branch
from
15185b8
to
63e23ad
Compare
This PR implements task autosizing, at long last!
It builds on the previous work with relocatable task builds (#584). After
building the relocatable task ELF file, it runs a "dummy link" against a
linker script with "infinite" memory (in practice, the entirety of memory
available on the chip). It then parses the resulting (static) binary to extract
sizes.
After finding sizes for every task, it runs the same memory packer as before,
then relinks each task with the resulting memory.
Task sizes are based on the target microcontroller, with a new
alignmentparameter passed to
allocate_one.There are extensive changes to
cargo xtask sizesto make it more genericallyuseful, decoupling the suggestions from the "find the size of a static ELF".
WARNING: this changes the format of the exported JSON files!
In addition, there are a bunch of new helper functions in
Configto help withtask and memory sizing / alignment.
This fixes #474 and maybe #439, and deprecates #476