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

build: Make backtraces useable in release builds #2324

Merged
merged 1 commit into from
Dec 20, 2024

Conversation

szokeasaurusrex
Copy link
Member

@szokeasaurusrex szokeasaurusrex commented Dec 19, 2024

Having panic = "abort" and strip = "symbols" makes backtraces generated with RUST_BACKTRACE=1 completely useless in release builds.

By changing these options to panic = "unwind" and strip = "debuginfo" makes backtraces usable in release builds. This will help us debug issues we are unable to reproduce, since we can ask users to set RUST_BACKTRACE=1 and provide the logs with this option.

This change does somewhat significantly increase release build binary size (from 7.9 MB to 13.9 MB on my system). This is likely acceptable if it improves our ability to debug problems. If folks complain, we can consider distributing a separate "stripped" binary for users sensitive to large binary size.

Closes #2200

Copy link
Member

@Swatinem Swatinem left a comment

Choose a reason for hiding this comment

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

this looks reasonable.

Ideally, instead of enabling unwinding directly, another alternative would be to add -C force-unwind-tables to the RUSTFLAGS (see https://swatinem.de/blog/unwind-tables/#rust)

Another option to consider would be using debug = "line-tables-only", which would give you a full-fidelity backtrace with inline frames, etc, but I bet that would inflate the binary size even more.

@szokeasaurusrex
Copy link
Member Author

Ideally, instead of enabling unwinding directly, another alternative would be to add -C force-unwind-tables to the RUSTFLAGS (see https://swatinem.de/blog/unwind-tables/#rust)

@Swatinem So, if I understand correctly, with this option we would still have panic = "abort"? Would we expect the binary size to be smaller with this option?

Another option to consider would be using debug = "line-tables-only", which would give you a full-fidelity backtrace with inline frames, etc, but I bet that would inflate the binary size even more.

I think let's just try with the current change first, I think it may already be helpful enough for what I need it for (mostly, I just would like to be able to find where errors originating from libraries are coming from. Usually if the error is emitted in Sentry CLI, I can find the error by searching for the error string).

@Swatinem
Copy link
Member

with this option we would still have panic = "abort"? Would we expect the binary size to be smaller with this option?

Yes, that codegen option adds the necessary unwind info to the binary so that backtraces work, but it avoids generating all the code to safely unwind in Rust, like destructors, catch_unwind, etc.

Having `strip = "symbols"` and omitting `-C force-unwind-tables` makes backtraces generated with `RUST_BACKTRACE=1` completely useless in release builds.

By adding the `-C force-unwind-tables` flag and setting `strip = "debuginfo"`, we make backtraces usable in release builds. This will help us debug issues we are unable to reproduce, since we can ask users to set `RUST_BACKTRACE=1` and provide the logs with this option.
@szokeasaurusrex szokeasaurusrex force-pushed the szokeasaurusrex/backtraces branch from e51acd9 to ed19a2b Compare December 20, 2024 15:45
@szokeasaurusrex
Copy link
Member Author

Alright, I went with your suggestion to set the -C force-unwind-tables instead of panic = "unwind". With this change, the release binary size (on my system) only increased to 12.2 MB, instead of 13.9 MB.

@szokeasaurusrex szokeasaurusrex merged commit ed19a2b into master Dec 20, 2024
14 checks passed
@szokeasaurusrex szokeasaurusrex deleted the szokeasaurusrex/backtraces branch December 20, 2024 15:54
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

Remove panic = abort in Cargo.toml
2 participants