-
-
Notifications
You must be signed in to change notification settings - Fork 768
DRAFT rust: disable frame pointers in the Tock kernel #1660
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
Conversation
Disabling frame pointer accounting in rustc has a significant impact on binary size. For ARM cores, this tends to drop size by 2-3%, but on RISC-V cores, we see size improvements of 9-10%. This change disables frame pointers, which appear to be otherwise unused, as a binary sliming measure.
I'm not terribly familiar with what this is doing under the hood, but my
guess is it's part of backtrace generation / a debugging tool?
Is this something we should leave enabled in debug builds and cut in
release builds?
There's some tension to doing things like that in the embedded space where
debug builds become unusable as they end up never fitting on hardware in
practice.
I guess by question is:
which appear to be otherwise unused
What are these normally used for (in Rust more broadly)?
…On Wed, Mar 4, 2020 at 11:25 AM Miguel Young ***@***.***> wrote:
Pull Request Overview
Disabling frame pointer accounting in rustc has a significant impact on
binary size. For ARM cores, this tends to drop text size by 2-3%, but on
RISC-V cores, we see size improvements of 9-10%. Full results below:
imix: 158208/162816; 2.830189% reduction
acd52832: 61440/62976; 2.439024% reduction
nucleo_f446re: 58881/59905; 1.709373% reduction
nucleo_f429zi: 58881/59905; 1.709373% reduction
hifive1: 48128/53760; 10.476190% reduction
nrf52dk: 88064/90624; 2.824859% reduction
nrf52840dk: 112640/115712; 2.654867% reduction
nrf52840_dongle: 108032/110080; 1.860465% reduction
arty-e21: 52224/57856; 9.734513% reduction
opentitan: 57845/63477; 8.872505% reduction
hail: 96768/99840; 3.076923% reduction
launchxl: 62976/64000; 1.600000% reduction
This change disables frame pointers, which appear to be otherwise
unused, as a binary sliming measure.
Testing Strategy
This change is currently untested; I'm not familiar with Tock's debugging
story, so I'm not sure what would be impacted by this change. This is a
draft right now, and I welcome suggestions for ensuring I haven't broken
anything.
TODO or Help Wanted
I'm hoping to test the waters on whether this is tenable, at least for
RISC-V cores, where the impact is outsized. I don't know if frame pointers
are used at all by Tock's debugging strategy, and I'm hoping to get
feedback on that.
At the very least, the OpenTitan project is looking for ways to slim its
use of the Tock kernel, and the 8.8% .text shrinkage is very attractive to
us in production builds.
Documentation Updated
- Updated the relevant files in /docs, or no updates are required.
Formatting
- Ran make formatall.
------------------------------
You can view, comment on, or merge this pull request online at:
#1660
Commit Summary
- rust: disable frame points in the Tock kernel
File Changes
- *M* boards/Makefile.common
<https://github.com/tock/tock/pull/1660/files#diff-f46cc5ec9e0e7eeea97d895b220e512e>
(3)
Patch Links:
- https://github.com/tock/tock/pull/1660.patch
- https://github.com/tock/tock/pull/1660.diff
—
You are receiving this because you are subscribed to this thread.
Reply to this email directly, view it on GitHub
<#1660?email_source=notifications&email_token=AACS3XUDMTJUZADFYOTDR5DRF2TJ3A5CNFSM4LBRJROKYY3PNVWWK3TUL52HS4DFUVEXG43VMWVGG33NNVSW45C7NFSM4ISQNYAQ>,
or unsubscribe
<https://github.com/notifications/unsubscribe-auth/AACS3XUGNHMGBG6MMHCZD6DRF2TJ3ANCNFSM4LBRJROA>
.
|
It depends a lot. The main use is in finding where the stack frames are during debugging, and yeah, this is totally something that can be debug-only. I don't know if Tock actually takes advantage of it, and we should figure out if we'd like to or if we'd like to just discard it all. |
If we aren't using this, and I don't think we are, I would say we just remove it and if someone wants to do something with it in the future we can re-evaluate including the flag on just debug builds or something. |
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'm in agreement with @bradjc, but think we should let this one marinate for a few days in case anyone has arguments against.
I second letting this sit. I also would like to hear feelings about testing the change, and maybe restricting it to RISC-V targets (since ARM targets hardly get any benefit). |
This seems to be the commit that enabled it for Rust: rust-lang/rust#61675 It looks like this is useful for RUST_BACKTRACE which I think we can live without. As long as GDB backtraces still work I'm happy with this change. |
Unclear, that's part of what I'd like to figure out (again, underscoring that I don't have a background in how Tock does debugging). The naive frame-pointer-chasing scheme doesn't work if you don't have frame pointers, obviously, but if that's a problem we can just include frame pointers in debug builds. If you can tell me if this completely breaks your workflow, that would be great. I'd honestly prefer to have a somewhat less blunt tool than "turn off all frame pointers". As I mentioned in the OP, I think we may need to examine other things, such as more aggressive inlining. |
I'm traveling and my setup isn't working at the moment so I can't give this a real test until Tuesday probably. What I'd like to verify is if we still get backtraces with QEMU + gdb (attach via gdb server, break, bt) |
I just did some testing on the FPGA and this patch doesn't seem to negatively affect GDB backtraces. |
These aren't super useful to us, and they make function sizes much larger. Some reports are on the order of 10% larger: tock/tock#1660 Signed-off-by: Sean Cross <sean@xobs.io>
Can we wait on getting an OK from @labbott? I trust that @alistair23's experience with the FPGA will translate to QEMU, but I'd prefer to wait an extra day or two to make sure this doesn't negatively impact development in the short term. |
LGTM (forgot to post this earlier, I appreciate you waiting) |
I resolved the remaining conflict and approved. Waiting for the build to come back green again. |
bors r+ |
Build succeeded |
1688: Revert PR #1660 r=alevy a=bradjc #1660 causes the kernel to panic on Hail. I don't know why, but I git bisected back to this change. ### Testing Strategy This pull request was tested by running Blink on Hail. ### TODO or Help Wanted We should merge this and then re-evaluate #1660. ### Documentation Updated - [x] Updated the relevant files in `/docs`, or no updates are required. ### Formatting - [x] Ran `make formatall`. Co-authored-by: Brad Campbell <bradjc5@gmail.com>
…=hanna-kruppe,Mark-Simulacrum [RISC-V] Do not force frame pointers We have been seeing some very inefficient code that went away when using `-Cforce-frame-pointers=no`. For instance `core::ptr::drop_in_place` at `-Oz` was compiled into a function which consisted entirely of saving registers to the stack, then using the frame pointer to restore the same registers (without any instructions between the prolog and epilog). The RISC-V LLVM backend supports frame pointer elimination, so it makes sense to allow this to happen when using Rust. It's not clear to me that frame pointers have ever been required in the general case. In rust-lang#61675 it was pointed out that this made reassembling stack traces easier, which is true, but there is a code generation option for forcing frame pointers, and I feel the default should not be to require frame pointers, given it demonstrably makes code size worse (around 10% in some embedded applications). The kinds of targets mentioned in rust-lang#61675 are popular, but should not dictate that code generation should be worse for all RISC-V targets, especially as there is a way to use CFI information to reconstruct the stack when the frame pointer is eliminated. It is also a misconception that `fp` is always used for the frame pointer. `fp` is an ABI name for `x8` (aka `s0`), and if no frame pointer is required, `x8` may be used for other callee-saved values. --- I am partly posting this to get feedback from @fintelia who introduced the change to require frame pointers, and @hanna-kruppe who had issues with the original PR. I would understand if we wanted to remove this setting on only a subset of RISC-V targets, but my preference would be to remove this setting everywhere. There are more details on the code size savings seen in Tock here: tock/tock#1660
… r=hanna-kruppe,Mark-Simulacrum [RISC-V] Do not force frame pointers We have been seeing some very inefficient code that went away when using `-Cforce-frame-pointers=no`. For instance `core::ptr::drop_in_place` at `-Oz` was compiled into a function which consisted entirely of saving registers to the stack, then using the frame pointer to restore the same registers (without any instructions between the prolog and epilog). The RISC-V LLVM backend supports frame pointer elimination, so it makes sense to allow this to happen when using Rust. It's not clear to me that frame pointers have ever been required in the general case. In rust-lang#61675 it was pointed out that this made reassembling stack traces easier, which is true, but there is a code generation option for forcing frame pointers, and I feel the default should not be to require frame pointers, given it demonstrably makes code size worse (around 10% in some embedded applications). The kinds of targets mentioned in rust-lang#61675 are popular, but should not dictate that code generation should be worse for all RISC-V targets, especially as there is a way to use CFI information to reconstruct the stack when the frame pointer is eliminated. It is also a misconception that `fp` is always used for the frame pointer. `fp` is an ABI name for `x8` (aka `s0`), and if no frame pointer is required, `x8` may be used for other callee-saved values. --- I am partly posting this to get feedback from @fintelia who introduced the change to require frame pointers, and @hanna-kruppe who had issues with the original PR. I would understand if we wanted to remove this setting on only a subset of RISC-V targets, but my preference would be to remove this setting everywhere. There are more details on the code size savings seen in Tock here: tock/tock#1660
… r=hanna-kruppe,Mark-Simulacrum [RISC-V] Do not force frame pointers We have been seeing some very inefficient code that went away when using `-Cforce-frame-pointers=no`. For instance `core::ptr::drop_in_place` at `-Oz` was compiled into a function which consisted entirely of saving registers to the stack, then using the frame pointer to restore the same registers (without any instructions between the prolog and epilog). The RISC-V LLVM backend supports frame pointer elimination, so it makes sense to allow this to happen when using Rust. It's not clear to me that frame pointers have ever been required in the general case. In rust-lang#61675 it was pointed out that this made reassembling stack traces easier, which is true, but there is a code generation option for forcing frame pointers, and I feel the default should not be to require frame pointers, given it demonstrably makes code size worse (around 10% in some embedded applications). The kinds of targets mentioned in rust-lang#61675 are popular, but should not dictate that code generation should be worse for all RISC-V targets, especially as there is a way to use CFI information to reconstruct the stack when the frame pointer is eliminated. It is also a misconception that `fp` is always used for the frame pointer. `fp` is an ABI name for `x8` (aka `s0`), and if no frame pointer is required, `x8` may be used for other callee-saved values. --- I am partly posting this to get feedback from @fintelia who introduced the change to require frame pointers, and @hanna-kruppe who had issues with the original PR. I would understand if we wanted to remove this setting on only a subset of RISC-V targets, but my preference would be to remove this setting everywhere. There are more details on the code size savings seen in Tock here: tock/tock#1660
Pull Request Overview
Disabling frame pointer accounting in rustc has a significant impact on
binary size. For ARM cores, this tends to drop text size by 2-3%, but on
RISC-V cores, we see size improvements of 9-10%. Full results below:
This change disables frame pointers, which appear to be otherwise
unused, as a binary sliming measure.
The most egregious example is the following before/after pair:
Mind, there is the separate concern that LLVM isn't inlining and eliminating these functions, which is a separate investigation I'm undertaking.
Testing Strategy
This change is currently untested; I'm not familiar with Tock's debugging story, so I'm not sure what would be impacted by this change. This is a draft right now, and I welcome suggestions for ensuring I haven't broken anything.
TODO or Help Wanted
I'm hoping to test the waters on whether this is tenable, at least for RISC-V cores, where the impact is outsized. I don't know if frame pointers are used at all by Tock's debugging strategy, and I'm hoping to get feedback on that.
At the very least, the OpenTitan project is looking for ways to slim its use of the Tock kernel, and the 8.8% .text shrinkage is very attractive to us in production builds.
Documentation Updated
/docs
, or no updates are required.Formatting
make formatall
.