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

Add Option to Force Unwind Tables #69984

Open
wants to merge 1 commit into
base: master
from

Conversation

@lenary
Copy link
Contributor

lenary commented Mar 13, 2020

When panic != unwind, nounwind is added to all functions for a target.
This can cause issues when a panic happens with RUST_BACKTRACE=1, as
there needs to be a way to reconstruct the backtrace. There are three
possible sources of this information: forcing frame pointers (for which
an option exists already), debug info (for which an option exists), or
unwind tables.

Especially for embedded devices, forcing frame pointers can have code
size overheads (RISC-V sees ~10% overheads, ARM sees ~2-3% overheads).
In production code, it can be the case that debug info is not kept, so it is useful
to provide this third option, unwind tables, that users can use to
reconstruct the call stack. Reconstructing this stack is harder than
with frame pointers, but it is still possible.


This came up in discussion on #69890, and turned out to be a fairly simple addition.

r? @hanna-kruppe

@@ -593,6 +593,14 @@ impl Session {
}
}

pub fn must_emit_unwind_tables(&self) -> bool {
if let Some(x) = self.opts.cg.force_unwind_tables {

This comment has been minimized.

Copy link
@hanna-kruppe

hanna-kruppe Mar 13, 2020

Member

As implemented, this allows users to turn off unwinding tables even if the target strictly requires them. That seems like a footgun.

This comment has been minimized.

Copy link
@oli-obk

oli-obk Mar 13, 2020

Contributor

should we just error in case of such a conflict?

This comment has been minimized.

Copy link
@nagisa

nagisa Mar 14, 2020

Contributor

Emitting an error has prior art in rustc so, yes.

This comment has been minimized.

Copy link
@lenary

lenary Mar 15, 2020

Author Contributor

Ok, Will do when I get back to my work computer.

This comment has been minimized.

Copy link
@lenary

lenary Mar 16, 2020

Author Contributor

I believe that I've done this in the right place: validate_commandline_args_with_session_available. This check only takes into account the target definition, not the no_landing_pads that is checked at attribute insertion time.

@hanna-kruppe

This comment has been minimized.

Copy link
Member

hanna-kruppe commented Mar 13, 2020

I'm obviously in favor of having this option in some way, but since -C options are effectively "stable" I believe @rust-lang/compiler should sign off the new option.

@nagisa nagisa added the T-compiler label Mar 14, 2020
@nagisa

This comment has been minimized.

Copy link
Contributor

nagisa commented Mar 14, 2020

@rfcbot fcp merge

This introduces an insta-stable argument to control a fairly well understood behaviour of the compilers (not just rustc, but also clang/gcc (-funwind-tables) and others). While the PR itself has some kinks to iron out, this FCP is specifically to gather consensus on merging an insta-stable flag to control whether unwinding tables are emit.

@rfcbot

This comment has been minimized.

Copy link

rfcbot commented Mar 14, 2020

Team member @nagisa has proposed to merge this. The next step is review by the rest of the tagged team members:

No concerns currently listed.

Once a majority of reviewers approve (and at most 2 approvals are outstanding), this will enter its final comment period. If you spot a major issue that hasn't been raised at any point in this process, please speak up!

See this document for info about what commands tagged team members can give me.

@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Mar 16, 2020

As I commented in #69890:

I personally think that this should be added as a third option to -C panic, called abort-with-backtrace. All it does is enable uwtable to guarantee that backtraces can be reliably generated. The reason for making it separate from -C panic=abort is that this comes at a slight code size increase (due to the need to preserve CSRs even for noreturn functions) and a binary size increase from the .eh_frame section.

On targets which do not support unwinding (e.g. thumb* & riscv*), the standard library would then be built with -C panic=abort-with-backtrace to ensure that reconstructing backtraces is always possible on these targets.

@lenary lenary force-pushed the lenary:lenary/force-uwtables branch from ef00e5c to 18f4afb Mar 16, 2020
@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Mar 16, 2020

The job mingw-check of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.
2020-03-16T16:21:20.3782402Z ========================== Starting Command Output ===========================
2020-03-16T16:21:20.3784856Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/33d15ff7-a5d3-41e6-b2f1-13e71616ed45.sh
2020-03-16T16:21:20.3785130Z 
2020-03-16T16:21:20.3791429Z ##[section]Finishing: Disable git automatic line ending conversion
2020-03-16T16:21:20.3811195Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/69984/merge to s
2020-03-16T16:21:20.3814471Z Task         : Get sources
2020-03-16T16:21:20.3814779Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-03-16T16:21:20.3815077Z Version      : 1.0.0
2020-03-16T16:21:20.3815295Z Author       : Microsoft
---
2020-03-16T16:21:21.6622575Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-03-16T16:21:21.6638527Z ##[command]git config gc.auto 0
2020-03-16T16:21:21.6648495Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-03-16T16:21:22.6585443Z ##[command]git config --get-all http.proxy
2020-03-16T16:21:22.6600588Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/69984/merge:refs/remotes/pull/69984/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @rust-lang/infra. (Feature Requests)

@hanna-kruppe

This comment has been minimized.

Copy link
Member

hanna-kruppe commented Mar 16, 2020

@Amanieu Can you elaborate why you think a new -C panic value is better than the independent flag that is proposed here? I don't see any advantage, and some downsides:

  • The panic=unwind/abort choice ties into several other things (which panic runtime is implicitly linked into leaf crates, which rlibs can be linked together) so adding another panic strategy brings unnecessary questions/baggage along.
  • While the motivation for this feature is backtraces on embedded targets, it's not inherently specific to panic=abort. -C force-unwind-tables=on could also help with generating backtraces outside of panics (e.g., profiling or error logging), since some functions can't unwind and wouldn't get an .eh_frame entry even with panic=unwind.
@lenary

This comment has been minimized.

Copy link
Contributor Author

lenary commented Mar 16, 2020

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Mar 16, 2020

@lenary: 🔑 Insufficient privileges: not in try users

When panic != unwind, `nounwind` is added to all functions for a target.
This can cause issues when a panic happens with RUST_BACKTRACE=1, as
there needs to be a way to reconstruct the backtrace. There are three
possible sources of this information: forcing frame pointers (for which
an option exists already), debug info (for which an option exists), or
unwind tables.

Especially for embedded devices, forcing frame pointers can have code
size overheads (RISC-V sees ~10% overheads, ARM sees ~2-3% overheads).
In code, it can be the case that debug info is not kept, so it is useful
to provide this third option, unwind tables, that users can use to
reconstruct the call stack. Reconstructing this stack is harder than
with frame pointers, but it is still possible.

This commit adds a compiler option which allows a user to force the
addition of unwind tables. Unwind tables cannot be disabled on targets
that require them for correctness.
@lenary lenary force-pushed the lenary:lenary/force-uwtables branch from 18f4afb to 74e911c Mar 16, 2020
@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Mar 16, 2020

One advantage of -C panic is that it can be specified in Cargo.toml.

-C force-unwind-tables=on could also help with generating backtraces outside of panics (e.g., profiling or error logging), since some functions can't unwind and wouldn't get an .eh_frame entry even with panic=unwind.

AFAIK that's not true, when compiling with panic=unwind all functions, regardless of whether they unwind or not, will get the uwtable attribute. The issue with missing backtraces only occurs with panic=abort, hence why I suggested making this an extension of panic=abort.

@hanna-kruppe

This comment has been minimized.

Copy link
Member

hanna-kruppe commented Mar 18, 2020

One advantage of -C panic is that it can be specified in Cargo.toml.

Cargo has its own validation of this Cargo.toml entry, so some Cargo changes are required to enable this. And if Cargo changes are required anyway, I don't think it would be significantly harder to add a new force-unwind-tables Cargo.toml key for this.

-C force-unwind-tables=on could also help with generating backtraces outside of panics (e.g., profiling or error logging), since some functions can't unwind and wouldn't get an .eh_frame entry even with panic=unwind.

AFAIK that's not true, when compiling with panic=unwind all functions, regardless of whether they unwind or not, will get the uwtable attribute. The issue with missing backtraces only occurs with panic=abort, hence why I suggested making this an extension of panic=abort.

Ah, right, this is indeed mostly the current behavior. But it is not true for allocator shims, and I don't think the current behavior for other functions is intended or can be expected to last forever.

For historic reasons, certain code paths in the compiler currently decide some unwinding-related things based on the global no_landing_pads property instead of a more precise per-function predicate that e.g. takes #[unwind] attributes into account. Once we clean up our act around unwinding (see also: #65303), I expect that the logic for uwtable being added will also be based on "can this function unwind / are we adding nounwind to it". Generally, the incentives for dropping uwtable from e.g. #[unwind(aborts)] functions are the same as dropping uwtable from all functions in panic=abort mode, just proportionally smaller.

@Amanieu

This comment has been minimized.

Copy link
Contributor

Amanieu commented Mar 18, 2020

Fair enough, I'll withdraw my objection to making this a -C flag.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Linked issues

Successfully merging this pull request may close these issues.

None yet

9 participants
You can’t perform that action at this time.