This repository was archived by the owner on Jan 24, 2022. It is now read-only.
[RFC] Keep .stack_sizes
#118
Merged
Conversation
This file contains bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
|
bors r+ |
|
👎 Rejected by label |
|
Oh, I just noticed the little by-text after the break. I really like this idea (and didn't quite know that this is even a thing...); don't see what's wrong with putting it into |
adamgreig
approved these changes
Sep 17, 2018
thejpster
approved these changes
Sep 18, 2018
korken89
approved these changes
Sep 22, 2018
|
bors r+ |
bors bot
added a commit
that referenced
this pull request
Sep 28, 2018
118: [RFC] Keep `.stack_sizes` r=japaric a=japaric PR [rust-lang/rust#51946] will add a `-Z emit-stack-sizes` to `rustc` that can be used to (make LLVM) emit stack usage metadata. Most linkers will discard this metadata by default, however. [rust-lang/rust#51946]: rust-lang/rust#51946 This PR / RFC proposes that we modify our linker script to keep this metadata and place in an output `.stack_sizes` section (the canonical section name that LLVM uses for this metadata). This `.stack_sizes` section will be marked as `(INFO)` meaning that it won't be loaded into the Flash or RAM of the target device. The advantage of doing this is that tools that parse this information will work out of the box. See examples at the bottom. Not doing this means that users will have to mess with linker scripts and tweak the linking process to be able to access the stack usage metadata. See [this example] to get an idea of the required steps to keep the metadata information. [this example]: https://github.com/japaric/stack-sizes#example-usage # Examples Using the [`cargo stack-sizes`] subcommand to build a project and print its stack usage information. Assumes that this PR has been merged. [`cargo stack-sizes`]: https://github.com/japaric/stack-sizes ``` console $ cargo stack-sizes --bin app -v RUSTC=stack-sizes-rustc "cargo" "rustc" "--bin" "app" "--" Finished dev [unoptimized + debuginfo] target(s) in 0.01s "stack-sizes" "target/thumbv7m-none-eabi/debug/app" address stack name 0x00000416 112 <cortex_m_rt::ExceptionFrame as core::fmt::Debug>::fmt::h4362577979112554 0x0000059e 96 <<cortex_m_rt::ExceptionFrame as core::fmt::Debug>::fmt::Hex as core::fmt::Debug>::fmt::hd24e21694d63c2ec 0x0000069c 72 core::fmt::Arguments::new_v1_formatted::hc22bfa5fcec35f0e 0x00000758 48 r0::init_data::hdc91f9605f364aeb 0x00000710 40 r0::zero_bss::h24ed73bae17eec88 0x000007d8 24 core::ptr::<impl *mut T>::offset::h940561014e707589 0x000007fc 24 core::ptr::<impl *const T>::offset::h6847f0bc0fe1f0b5 0x00000820 24 core::ptr::read::ha83613479c1b8688 0x00000858 24 core::sync::atomic::compiler_fence::h89dd3725007a5019 0x00002cb0 24 core::sync::atomic::compiler_fence::h4e6eb311378b7447 0x00000668 16 UserHardFault_ 0x000007be 16 core::ptr::write_volatile::h92be4be56d39953d 0x00000840 16 core::ptr::write::h1bff45e7fbd786f1 0x00002c92 16 rust_begin_unwind 0x00000404 8 main 0x0000061c 8 Reset 0x00000684 8 SysTick 0x000006fc 8 core::ptr::drop_in_place::h96b0344554e68fcb 0x0000089c 8 core::mem::uninitialized::hea41370061be039b 0x000008aa 8 core::mem::zeroed::h87926bff0ea7d7b3 0x00000400 0 app::main::hb1f6f4352b7a35e2 0x0000069a 0 __pre_init ``` ``` console $ cargo stack-sizes --bin app --release -v RUSTC=stack-sizes-rustc "cargo" "rustc" "--bin" "app" "--release" "--" Finished release [optimized + debuginfo] target(s) in 0.01s "stack-sizes" "target/thumbv7m-none-eabi/release/app" address stack name 0x00000400 0 main 0x00000402 0 Reset 0x00000616 0 UserHardFault_ 0x00000618 0 SysTick 0x0000061a 0 __pre_init ``` --- r? @rust-embedded/cortex-m does this seem like a reasonable addition? Or do you think keeping .stack_sizes should *not* be done in this crate? Co-authored-by: Jorge Aparicio <[email protected]>
Build succeeded |
japaric
added a commit
that referenced
this pull request
Mar 24, 2019
this undoes PR #118 I found a problem with keeping this section. Turns out that the input `.stack_sizes` sections contain references to all functions compiled with `-Z emit-stack-sizes` (the section contains the addresses of all those functions after all) so keeping those section prevents the linker from removing *any* of those functions. This is not a problem today because `-Z emit-stack-sizes` is *opt-in* and only used to analyze a program. However, I am proposing a rust-lang/rust PR to build the `compiler-builtins` crate with `-Z emit-stack-sizes`. That change will cause *all* the functions in that crate to be kept in binaries that link to `cortex-m-rt`, regardless of whether the crate author uses `-Z emit-stack-sizes` or not, leading a increase in binary size of about 14 KB (`.text` section). To prevent issues with that rust-lang/rust PR I propose we undo PR #118. On the bright side, the tools that were depending on this (`cargo-stack-sizes` and `cargo-call-stack`) no longer do so in their latest releases so nothing is lost on the tooling front with this change.
bors bot
added a commit
that referenced
this pull request
Mar 24, 2019
186: do not KEEP the .stack_sizes section r=therealprof a=japaric this undoes PR #118 I found a problem with keeping this section. Turns out that the input `.stack_sizes` sections contain references to all functions compiled with `-Z emit-stack-sizes` (the section contains the addresses of all those functions after all) so keeping those section prevents the linker from removing *any* of those functions. This is not a problem today because `-Z emit-stack-sizes` is *opt-in* and only used to analyze a program. However, I am proposing a rust-lang/rust PR to build the `compiler-builtins` crate with `-Z emit-stack-sizes`. That change will cause *all* the functions in that crate to be kept in binaries that link to `cortex-m-rt`, regardless of whether the crate author uses `-Z emit-stack-sizes` or not, leading a increase in binary size of about 14 KB (`.text` section). To prevent issues with that rust-lang/rust PR I propose we undo PR #118. On the bright side, the tools that were depending on this (`cargo-stack-sizes` and `cargo-call-stack`) no longer do so in their latest releases so nothing is lost on the tooling front with this change. r? @rust-embedded/cortex-m Co-authored-by: Jorge Aparicio <[email protected]>
bors bot
added a commit
that referenced
this pull request
Apr 2, 2019
186: do not KEEP the .stack_sizes section r=therealprof a=japaric this undoes PR #118 I found a problem with keeping this section. Turns out that the input `.stack_sizes` sections contain references to all functions compiled with `-Z emit-stack-sizes` (the section contains the addresses of all those functions after all) so keeping those section prevents the linker from removing *any* of those functions. This is not a problem today because `-Z emit-stack-sizes` is *opt-in* and only used to analyze a program. However, I am proposing a rust-lang/rust PR to build the `compiler-builtins` crate with `-Z emit-stack-sizes`. That change will cause *all* the functions in that crate to be kept in binaries that link to `cortex-m-rt`, regardless of whether the crate author uses `-Z emit-stack-sizes` or not, leading a increase in binary size of about 14 KB (`.text` section). To prevent issues with that rust-lang/rust PR I propose we undo PR #118. On the bright side, the tools that were depending on this (`cargo-stack-sizes` and `cargo-call-stack`) no longer do so in their latest releases so nothing is lost on the tooling front with this change. r? @rust-embedded/cortex-m Co-authored-by: Jorge Aparicio <[email protected]>
Sign up for free
to subscribe to this conversation on GitHub.
Already have an account?
Sign in.
Add this suggestion to a batch that can be applied as a single commit.
This suggestion is invalid because no changes were made to the code.
Suggestions cannot be applied while the pull request is closed.
Suggestions cannot be applied while viewing a subset of changes.
Only one suggestion per line can be applied in a batch.
Add this suggestion to a batch that can be applied as a single commit.
Applying suggestions on deleted lines is not supported.
You must change the existing code in this line in order to create a valid suggestion.
Outdated suggestions cannot be applied.
This suggestion has been applied or marked resolved.
Suggestions cannot be applied from pending reviews.
Suggestions cannot be applied on multi-line comments.
Suggestions cannot be applied while the pull request is queued to merge.
Suggestion cannot be applied right now. Please check back later.
PR rust-lang/rust#51946 will add a
-Z emit-stack-sizestorustcthat canbe used to (make LLVM) emit stack usage metadata. Most linkers will discard this
metadata by default, however.
This PR / RFC proposes that we modify our linker script to keep this metadata
and place in an output
.stack_sizessection (the canonical section name thatLLVM uses for this metadata).
This
.stack_sizessection will be marked as(INFO)meaning that it won't beloaded into the Flash or RAM of the target device.
The advantage of doing this is that tools that parse this information will work
out of the box. See examples at the bottom.
Not doing this means that users will have to mess with linker scripts and tweak
the linking process to be able to access the stack usage metadata. See this
example to get an idea of the required steps to keep the metadata information.
Examples
Using the
cargo stack-sizessubcommand to build a project and print itsstack usage information. Assumes that this PR has been merged.
r? @rust-embedded/cortex-m does this seem like a reasonable addition? Or do you think keeping .stack_sizes should not be done in this crate?