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.

Sign up for GitHub

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

Jump to bottom

build(core): export Rust functions' stack sizes #4555

Open
wants to merge 1 commit into
base: main
Choose a base branch
from
Open

build(core): export Rust functions' stack sizes #4555

wants to merge 1 commit into from
+6 −0

Conversation

romanz
Copy link
Contributor

@romanz romanz commented Jan 29, 2025 •
edited
Loading

It is useful to find the top-most stack consuming functions:

$ make build_firmware
$ arm-none-eabi-size -A build/firmware/firmware.elf | grep .stack_sizes
.stack_sizes          7523           0

$ cargo install [email protected]
$ stack-sizes build/firmware/firmware.elf | grep trezor_lib | sort -k2 -n | tail -n10
0x081c1721	3536	trezor_lib::ui::api::firmware_micropython::new_confirm_properties::h2ab0feebaf154486
0x081c0e7d	3560	trezor_lib::ui::api::firmware_micropython::new_confirm_modify_output::h04465b97d57fafb6
0x081c6161	3688	trezor_lib::ui::api::firmware_micropython::new_show_checklist::he16b109bc4dff398
0x081c4089	4240	trezor_lib::ui::api::firmware_micropython::new_request_pin::h3280c5eff8900a22
0x081be3e1	4960	trezor_lib::ui::api::firmware_micropython::new_confirm_action::h860f874d714ace74
0x081bf545	5096	trezor_lib::ui::api::firmware_micropython::new_confirm_emphasized::h9ade56f5c88001c0
0x081c1ded	5736	trezor_lib::ui::api::firmware_micropython::new_confirm_summary::he2e1274bbc07703e
0x081c7ee9	6760	trezor_lib::ui::api::firmware_micropython::new_show_remaining_shares::h1f67cbfdfeb4c683
0x081c127d	6768	trezor_lib::ui::api::firmware_micropython::new_confirm_more::h107a4be9b5431bb4
0x081c5441	8312	trezor_lib::ui::api::firmware_micropython::new_show_address_details::h352e0b87c58914ce

[no changelog]

@romanz romanz added core Trezor Core firmware. Runs on Trezor Model T and T2B1. rust Pull requests that update Rust code labels Jan 29, 2025
@romanz romanz self-assigned this Jan 29, 2025
@github-actions GitHub Actions
Copy link

github-actions bot commented Jan 29, 2025 •
edited
Loading

core UI changes device test click test persistence test
T2T1 Model T test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3B1 Safe 3 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
T3T1 Safe 5 test(screens) main(screens) test(screens) main(screens) test(screens) main(screens)
All main(screens)

@romanz romanz assigned matejcik and unassigned matejcik Jan 29, 2025
@romanz romanz requested review from matejcik and mmilata January 29, 2025 18:54
@romanz romanz marked this pull request as ready for review January 29, 2025 18:54
mmilata
mmilata reviewed Jan 30, 2025
View reviewed changes
Copy link
Member

@mmilata mmilata left a comment

Choose a reason for hiding this comment

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

Is it possible to make Cargo install the stack-sizes binary as part of dev-dependencies? Unfortunately it doesn't seem to be available in nixpkgs.

core/SConscript.firmware Outdated
@@ -794,6 +794,12 @@ def cargo_build():
# see https://nnethercote.github.io/perf-book/type-sizes.html#measuring-type-sizes for more details
env.Append(ENV={'RUSTFLAGS': '-Z print-type-sizes'})

if ARGUMENTS.get('RUST_EMIT_STACK_SIZES', '0') == '1':
Copy link
Member

@mmilata mmilata Jan 30, 2025 •
edited
Loading

Choose a reason for hiding this comment

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

Since it appears cheap I wonder if we should always include the -Z emit-stack-sizes flag? One less build option to be aware of. Downside is it won't work on stable compiler but I think we're currently far from that anyway.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Sounds good - fixed in b508b8a

@romanz
Copy link
Contributor Author

romanz commented Jan 30, 2025 •
edited
Loading

Is it possible to make Cargo install the stack-sizes binary as part of dev-dependencies?

Good question, not sure if it's possible :(
Maybe it would be simpler to implement the parsing using pyelftools? It doesn't require compilation and doesn't have any extra dependencies...
I can try to do it in a separate PR :)

@romanz romanz changed the title build(core): allow exporting Rust functions' stack sizes build(core): export Rust functions' stack sizes Jan 30, 2025
@mmilata
Copy link
Member

mmilata commented Jan 30, 2025

Perhaps it would make sense to add this functionality to https://github.com/trezor/binsize but leave it up to you if you want to do it.

@romanz
build(core): export Rust functions' stack sizes
5da93c0 
It is useful to find the top-most stack consuming functions:

```
$ make build_firmware
$ arm-none-eabi-size -A build/firmware/firmware.elf | grep .stack_sizes
.stack_sizes          7523           0

$ cargo install [email protected]
$ stack-sizes build/firmware/firmware.elf | grep trezor_lib | sort -k2 -n | tail -n10
0x081c1721	3536	trezor_lib::ui::api::firmware_micropython::new_confirm_properties::h2ab0feebaf154486
0x081c0e7d	3560	trezor_lib::ui::api::firmware_micropython::new_confirm_modify_output::h04465b97d57fafb6
0x081c6161	3688	trezor_lib::ui::api::firmware_micropython::new_show_checklist::he16b109bc4dff398
0x081c4089	4240	trezor_lib::ui::api::firmware_micropython::new_request_pin::h3280c5eff8900a22
0x081be3e1	4960	trezor_lib::ui::api::firmware_micropython::new_confirm_action::h860f874d714ace74
0x081bf545	5096	trezor_lib::ui::api::firmware_micropython::new_confirm_emphasized::h9ade56f5c88001c0
0x081c1ded	5736	trezor_lib::ui::api::firmware_micropython::new_confirm_summary::he2e1274bbc07703e
0x081c7ee9	6760	trezor_lib::ui::api::firmware_micropython::new_show_remaining_shares::h1f67cbfdfeb4c683
0x081c127d	6768	trezor_lib::ui::api::firmware_micropython::new_confirm_more::h107a4be9b5431bb4
0x081c5441	8312	trezor_lib::ui::api::firmware_micropython::new_show_address_details::h352e0b87c58914ce
```

[no changelog]
@romanz romanz force-pushed the romanz/emit-stack-sizes branch from b508b8a to 5da93c0 Compare January 30, 2025 19:57
@romanz
Copy link
Contributor Author

romanz commented Jan 30, 2025

Perhaps it would make sense to add this functionality to https://github.com/trezor/binsize

Thanks - I'll prepare a pyelftools-based proof-of-concept and consult with @grdddj.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@mmilata mmilata mmilata approved these changes

@matejcik matejcik Awaiting requested review from matejcik

Assignees

@romanz romanz

Labels
core Trezor Core firmware. Runs on Trezor Model T and T2B1. rust Pull requests that update Rust code
Projects
Firmware
Status: 🔎 Needs review
Milestone
No milestone
Development

Successfully merging this pull request may close these issues.
3 participants
@romanz @mmilata @matejcik