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

Serious regression in binary size when using LTO #43054

Closed
japaric opened this issue Jul 4, 2017 · 4 comments
Closed

Serious regression in binary size when using LTO #43054

japaric opened this issue Jul 4, 2017 · 4 comments
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.

Comments

@japaric
Copy link
Member

japaric commented Jul 4, 2017

STR

$ git clone https://github.com/japaric/2wd

$ cd 2wd/firmware

$ git rev-parse HEAD
3750a542d77a37c099f9cfffe066f7369d6ec9de

$ xargo build --release

$ arm-none-eabi-size arm-none-eabi-size target/thumbv7m-none-eabi/release/firmware
   text    data     bss     dec     hex filename
  10118      24      20   10162    27b2 firmware

With version: rustc 1.20.0-nightly (067971139 2017-07-02)
(nightly-2017-07-03).

This is a 200%+ increase in binary size compared to an older nightly:

$ arm-none-eabi-size arm-none-eabi-size target/thumbv7m-none-eabi/release/firmware
   text    data     bss     dec     hex filename
   3057      24      20    3101     c1d firmware

With version: rustc 1.20.0-nightly (05b579766 2017-07-01)
(nightly-2017-07-02).

The number gets worst with simpler programs. I've seen increases of up to 600%
(1KB -> 7KB).

The compiler seems to have lost the ability to optimize away the formatting
machinery -- the program shown here doesn't do any formatting on panics.
Compare the disassembly of the normal program vs the one of the "bloated"
program.

PRs between the good and bad version:

cc @alexcrichton @brson @est31

@Mark-Simulacrum Mark-Simulacrum added the T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. label Jul 4, 2017
@est31
Copy link
Member

est31 commented Jul 4, 2017

PR #42938 by me looks like the most suspicious suspect. It stores the column numbers of each possible panic location in addition to the line number. Its possible that there is some additional effect that bloats up this generally very tiny increase to the reported increase.

@est31
Copy link
Member

est31 commented Jul 4, 2017

Okay, found the culprit. It was my PR indeed. It broke the ABI of panic_fmt.

@japaric I can reproduce the sizes you reported locally.

$ arm-none-eabi-size target/thumbv7m-none-eabi/release/firmware
   text    data     bss     dec     hex filename
  10102      24      20   10146    27a2 target/thumbv7m-none-eabi/release/firmware

If I change the function signature in lang_items.rs in cortex-m-rt to the following (one line change, addition of col param):

#[lang = "panic_fmt"]
#[linkage = "weak"]
unsafe extern "C" fn panic_fmt(
    _args: ::core::fmt::Arguments,
    _file: &'static str,
    _line: u32,
    _col: u32,
) -> ! {

Then its back to normal levels:

$ arm-none-eabi-size target/thumbv7m-none-eabi/release/firmware
   text    data     bss     dec     hex filename
   3065      24      20    3109     c25 target/thumbv7m-none-eabi/release/firmware

So, summarizing I'd say its legal breakage. Lang items are unstable so this is okay to do. Maybe we should do some messaging to alert people? Optimally rustc could simply deny compilation if the abi doesn't match.

@nagisa
Copy link
Member

nagisa commented Jul 4, 2017

Duplicate of #9307 then, I think.

@japaric
Copy link
Member Author

japaric commented Jul 4, 2017

@est31 Thanks for checking. 👍 Indeed changing the ABI fixes the problem.

Optimally rustc could simply deny compilation if the abi doesn't match.

I certainly would have preferred a compilation error over silent bloat.

Closing as a duplicate of #9307.

@japaric japaric closed this as completed Jul 4, 2017
kennytm added a commit to kennytm/rust that referenced this issue Sep 7, 2018
…imulacrum

stabilize #[panic_handler]

closes rust-lang#44489

### Update(2018-09-07)

This was proposed for stabilization in rust-lang#44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in rust-lang#44489 (comment)

Documentation PRs:

- Reference. rust-lang/reference#362
- Nomicon. rust-lang/nomicon#75

---

`#[panic_implementation]` was implemented recently in rust-lang#50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. rust-lang#44489) but this PR is meant to start a discussion about those issues / questions with the language team.

(\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in rust-lang#43054

Some unresolved questions from rust-lang#44489:

> Should the Display of PanicInfo format the panic information as "panicked at 'reason',
> src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason".

The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable.

> Is this design compatible, or can it be extended to work, with unwinding implementations for
> no-std environments?

I believe @whitequark made more progress with unwinding in no-std since their last comment in rust-lang#44489. Perhaps they can give us an update?

---

Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.

cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp
kennytm added a commit to kennytm/rust that referenced this issue Sep 8, 2018
…imulacrum

stabilize #[panic_handler]

closes rust-lang#44489

### Update(2018-09-07)

This was proposed for stabilization in rust-lang#44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in rust-lang#44489 (comment)

Documentation PRs:

- Reference. rust-lang/reference#362
- Nomicon. rust-lang/nomicon#75

---

`#[panic_implementation]` was implemented recently in rust-lang#50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. rust-lang#44489) but this PR is meant to start a discussion about those issues / questions with the language team.

(\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in rust-lang#43054

Some unresolved questions from rust-lang#44489:

> Should the Display of PanicInfo format the panic information as "panicked at 'reason',
> src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason".

The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable.

> Is this design compatible, or can it be extended to work, with unwinding implementations for
> no-std environments?

I believe @whitequark made more progress with unwinding in no-std since their last comment in rust-lang#44489. Perhaps they can give us an update?

---

Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.

cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp
bors added a commit that referenced this issue Sep 8, 2018
stabilize #[panic_handler]

closes #44489

### Update(2018-09-07)

This was proposed for stabilization in #44489 (comment) and its FCP with disposition to merge / accept is nearly over. The summary of what's being stabilized can be found in #44489 (comment)

Documentation PRs:

- Reference. rust-lang/reference#362
- Nomicon. rust-lang/nomicon#75

---

`#[panic_implementation]` was implemented recently in #50338. `#[panic_implementation]` is basically the old `panic_fmt` language item but in a less error prone (\*) shape. There are still some issues and questions to sort out around this feature (cf. #44489) but this PR is meant to start a discussion about those issues / questions with the language team.

(\*) `panic_fmt` was not type checked; changes in its function signature caused serious, silent binary size regressions like the one observed in #43054

Some unresolved questions from #44489:

> Should the Display of PanicInfo format the panic information as "panicked at 'reason',
> src/main.rs:27:4", as "'reason', src/main.rs:27:4", or simply as "reason".

The current implementation formats `PanicInfo` as the first alternative, which is how panic messages are formatted by the `std` panic handler. The `Display` implementation is more than a convenience: `PanicInfo.message` is unstable so it's not possible to replicate the `Display` implementation on stable.

> Is this design compatible, or can it be extended to work, with unwinding implementations for
> no-std environments?

I believe @whitequark made more progress with unwinding in no-std since their last comment in #44489. Perhaps they can give us an update?

---

Another unresolved question is where this feature should be documented. The feature currently doesn't have any documentation.

cc @rust-lang/lang
cc @jackpot51 @alevy @phil-opp
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

No branches or pull requests

4 participants