[lenary]

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

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

should we just error in case of such a conflict?

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

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

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.

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.

@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.

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

• @Zoxc
• @cramertj
• @eddyb
• @estebank
• @matthewjasper
• @michaelwoerister
• @nagisa
• @nikomatsakis
• @oli-obk
• @petrochenkov
• @pnkfelix
• @varkor
• @wesleywiser

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.

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.

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)

@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.

@bors retry

@lenary: 🔑 Insufficient privileges: not in try users

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.

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.

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

@rfcbot reviewed

Should we avoid emitting unwind tables by default on Windows (and let people opt-in with this flag)? If not, we probably want an option to turn off unwind tables which this doesn't enable.

Given the many different mechanisms and tools that rely on SEH, and Windows gurus stating that pretty much all functions should have unwinding information on x64 Windows, it does not seem very wise to ever omit unwinding tables. Even an opt-out seems dubious to me, and as far as I can tell it's not a feature anyone has ever requested. In fact, after poking at it for a little bit, I can't find any such opt out in MSVC, even when compiling plain C code.

In any case, should we ever change our mind about this, we can still change -Cforce-unwind-tables=no from reporting an error if the target has them enabled by default to override the target's default instead. This is backwards compatible, so it can (and should) be a separate discussion at any point after the flag lands.

My understanding is that the self.no_landing_pads() will always be false on windows (that code used to have windows-specific comments by it, which were out of date), so this patch should ensure that you cannot remove unwind tables on windows.

I don't know if i should include that check in the error reporting here: https://github.com/rust-lang/rust/pull/69984/files#diff-eadaa1c9e135801995ae29eaec608049R1173 - happy to update the patch to do so if needed, not sure how it will affect testing.

My understanding is that the self.no_landing_pads() will always be false on windows (that code used to have windows-specific comments by it, which were out of date)

I'm pretty sure this is wrong. Session::no_landing_pads() (while it still exists; #70175 removes it) only looks at -C panic and -Z no-landing-pads, neither of which has Windows-related restrictions, so it can easily return true on Windows targets. The target spec field requires_uwtable is taken into account independently of what no_landing_pads() says. This PR even refactors that condition to introduce the must_emit_uwtable.

However, your conclusion (that users can't remove unwinding information on Windows targets) is correct as far as I can tell.

⌛ Testing commit cda9946 with merge ac9bfbf737b9450ab6f6e632406e3a403b4e958a...

@bors retry

⌛ Testing commit cda9946 with merge f8d394e...