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

Support -Cforce-frame-pointers=non-leaf #124733

Merged
merged 8 commits into from
Jun 23, 2024
6 changes: 3 additions & 3 deletions compiler/rustc_codegen_cranelift/src/lib.rs
Original file line number Diff line number Diff line change
Expand Up @@ -271,9 +271,9 @@ fn build_isa(sess: &Session, backend_config: &BackendConfig) -> Arc<dyn TargetIs
flags_builder.set("enable_verifier", enable_verifier).unwrap();
flags_builder.set("regalloc_checker", enable_verifier).unwrap();

let preserve_frame_pointer = sess.target.options.frame_pointer
!= rustc_target::spec::FramePointer::MayOmit
|| matches!(sess.opts.cg.force_frame_pointers, Some(true));
let mut frame_ptr = sess.target.options.frame_pointer.clone();
frame_ptr.ratchet(sess.opts.cg.force_frame_pointers);
let preserve_frame_pointer = frame_ptr != rustc_target::spec::FramePointer::MayOmit;
flags_builder
.set("preserve_frame_pointers", if preserve_frame_pointer { "true" } else { "false" })
.unwrap();
Expand Down
5 changes: 3 additions & 2 deletions compiler/rustc_codegen_llvm/src/attributes.rs
Original file line number Diff line number Diff line change
Expand Up @@ -108,9 +108,10 @@ pub fn frame_pointer_type_attr<'ll>(cx: &CodegenCx<'ll, '_>) -> Option<&'ll Attr
let opts = &cx.sess().opts;
// "mcount" function relies on stack pointer.
// See <https://sourceware.org/binutils/docs/gprof/Implementation.html>.
if opts.unstable_opts.instrument_mcount || matches!(opts.cg.force_frame_pointers, Some(true)) {
fp = FramePointer::Always;
if opts.unstable_opts.instrument_mcount {
fp.ratchet(FramePointer::Always);
}
fp.ratchet(opts.cg.force_frame_pointers);
let attr_value = match fp {
FramePointer::Always => "all",
FramePointer::NonLeaf => "non-leaf",
Expand Down
6 changes: 3 additions & 3 deletions compiler/rustc_interface/src/tests.rs
Original file line number Diff line number Diff line change
Expand Up @@ -20,9 +20,9 @@ use rustc_span::source_map::{RealFileLoader, SourceMapInputs};
use rustc_span::symbol::sym;
use rustc_span::{FileName, SourceFileHashAlgorithm};
use rustc_target::spec::{
CodeModel, LinkerFlavorCli, MergeFunctions, OnBrokenPipe, PanicStrategy, RelocModel, WasmCAbi,
CodeModel, FramePointer, LinkerFlavorCli, MergeFunctions, OnBrokenPipe, PanicStrategy,
RelocModel, RelroLevel, SanitizerSet, SplitDebuginfo, StackProtector, TlsModel, WasmCAbi,
};
use rustc_target::spec::{RelroLevel, SanitizerSet, SplitDebuginfo, StackProtector, TlsModel};
use std::collections::{BTreeMap, BTreeSet};
use std::num::NonZero;
use std::path::{Path, PathBuf};
Expand Down Expand Up @@ -605,7 +605,7 @@ fn test_codegen_options_tracking_hash() {
tracked!(debug_assertions, Some(true));
tracked!(debuginfo, DebugInfo::Limited);
tracked!(embed_bitcode, false);
tracked!(force_frame_pointers, Some(false));
tracked!(force_frame_pointers, FramePointer::Always);
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
tracked!(force_unwind_tables, Some(true));
tracked!(inline_threshold, Some(0xf007ba11));
tracked!(instrument_coverage, InstrumentCoverage::Yes);
Expand Down
18 changes: 13 additions & 5 deletions compiler/rustc_session/src/config.rs
Original file line number Diff line number Diff line change
Expand Up @@ -19,7 +19,7 @@ use rustc_macros::{Decodable, Encodable, HashStable_Generic};
use rustc_span::edition::{Edition, DEFAULT_EDITION, EDITION_NAME_LIST, LATEST_STABLE_EDITION};
use rustc_span::source_map::FilePathMapping;
use rustc_span::{FileName, FileNameDisplayPreference, RealFileName, SourceFileHashAlgorithm};
use rustc_target::spec::{LinkSelfContainedComponents, LinkerFeatures};
use rustc_target::spec::{FramePointer, LinkSelfContainedComponents, LinkerFeatures};
use rustc_target::spec::{SplitDebuginfo, Target, TargetTriple};
use std::collections::btree_map::{
Iter as BTreeMapIter, Keys as BTreeMapKeysIter, Values as BTreeMapValuesIter,
Expand Down Expand Up @@ -2524,6 +2524,15 @@ pub fn build_session_options(early_dcx: &mut EarlyDiagCtxt, matches: &getopts::M
}
}

if !nightly_options::is_unstable_enabled(matches)
Copy link
Contributor

Choose a reason for hiding this comment

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

Annoyingly, there seem to be multiple different ways of checking for -Zunstable-options when implementing these ad-hoc value checks:

  • Some code uses nightly_options::is_unstable_enabled(matches), which parses the value independently
  • Some code uses unstable_opts.unstable_options, which uses whatever has already been parsed into the UnstableOptions struct via normal means

In this context, we should be able to just check unstable_opts.unstable_options, which seems like the less hacky variant.

(I assume you probably just copied this from the code immediately below. 🤷)

Copy link
Member Author

Choose a reason for hiding this comment

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

I did check the unstable_options.unstable_options thing, but it says you shouldn't touch it directly, so I avoided poking it directly. idk lol?

I think there should be a different way of specifying "these values are unstable" that uses a pattern or pattern-guard of some kind.

Copy link
Member Author

Choose a reason for hiding this comment

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

I realize there's only a few cases of this so it seems like a waste, but I think a profusion of flags is partly the fault of it not being easy to add such patterns that determine stability.

Copy link
Contributor

Choose a reason for hiding this comment

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

I don’t want to derail this PR too much, but if I get a chance I’ll open a separate issue or thread to discuss improvements to unstable values of stable flags, because the status quo has been annoying me too.

&& cg.force_frame_pointers == FramePointer::NonLeaf
{
early_dcx.early_fatal(
"`-Cforce-frame-pointers=non-leaf` or `always` also requires `-Zunstable-options` \
and a nightly compiler",
)
}

// For testing purposes, until we have more feedback about these options: ensure `-Z
// unstable-options` is required when using the unstable `-C link-self-contained` and `-C
// linker-flavor` options.
Expand Down Expand Up @@ -2966,10 +2975,8 @@ pub(crate) mod dep_tracking {
use rustc_span::edition::Edition;
use rustc_span::RealFileName;
use rustc_target::spec::{
CodeModel, MergeFunctions, OnBrokenPipe, PanicStrategy, RelocModel, WasmCAbi,
};
use rustc_target::spec::{
RelroLevel, SanitizerSet, SplitDebuginfo, StackProtector, TargetTriple, TlsModel,
CodeModel, FramePointer, MergeFunctions, OnBrokenPipe, PanicStrategy, RelocModel,
RelroLevel, SanitizerSet, SplitDebuginfo, StackProtector, TargetTriple, TlsModel, WasmCAbi,
};
use std::collections::BTreeMap;
use std::hash::{DefaultHasher, Hash};
Expand Down Expand Up @@ -3023,6 +3030,7 @@ pub(crate) mod dep_tracking {
lint::Level,
WasiExecModel,
u32,
FramePointer,
RelocModel,
CodeModel,
TlsModel,
Expand Down
22 changes: 17 additions & 5 deletions compiler/rustc_session/src/options.rs
Original file line number Diff line number Diff line change
Expand Up @@ -12,10 +12,9 @@ use rustc_span::edition::Edition;
use rustc_span::RealFileName;
use rustc_span::SourceFileHashAlgorithm;
use rustc_target::spec::{
CodeModel, LinkerFlavorCli, MergeFunctions, OnBrokenPipe, PanicStrategy, SanitizerSet, WasmCAbi,
};
use rustc_target::spec::{
RelocModel, RelroLevel, SplitDebuginfo, StackProtector, TargetTriple, TlsModel,
CodeModel, FramePointer, LinkerFlavorCli, MergeFunctions, OnBrokenPipe, PanicStrategy,
RelocModel, RelroLevel, SanitizerSet, SplitDebuginfo, StackProtector, TargetTriple, TlsModel,
WasmCAbi,
};
use std::collections::BTreeMap;
use std::hash::{DefaultHasher, Hasher};
Expand Down Expand Up @@ -374,6 +373,7 @@ mod desc {
pub const parse_opt_comma_list: &str = parse_comma_list;
pub const parse_number: &str = "a number";
pub const parse_opt_number: &str = parse_number;
pub const parse_frame_pointer: &str = "one of `true`/`yes`/`on`, `false`/`no`/`off`, or (with -Zunstable-options) `non-leaf` or `always`";
pub const parse_threads: &str = parse_number;
pub const parse_time_passes_format: &str = "`text` (default) or `json`";
pub const parse_passes: &str = "a space-separated list of passes, or `all`";
Expand Down Expand Up @@ -672,6 +672,18 @@ mod parse {
}
}

pub(crate) fn parse_frame_pointer(slot: &mut FramePointer, v: Option<&str>) -> bool {
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
let mut yes = false;
match v {
_ if parse_bool(&mut yes, v) && yes => slot.ratchet(FramePointer::Always),
_ if parse_bool(&mut yes, v) => slot.ratchet(FramePointer::MayOmit),
Some("always") => slot.ratchet(FramePointer::Always),
Some("non-leaf") => slot.ratchet(FramePointer::NonLeaf),
_ => return false,
};
true
}

pub(crate) fn parse_passes(slot: &mut Passes, v: Option<&str>) -> bool {
match v {
Some("all") => {
Expand Down Expand Up @@ -1479,7 +1491,7 @@ options! {
"emit bitcode in rlibs (default: yes)"),
extra_filename: String = (String::new(), parse_string, [UNTRACKED],
"extra data to put in each output filename"),
force_frame_pointers: Option<bool> = (None, parse_opt_bool, [TRACKED],
force_frame_pointers: FramePointer = (FramePointer::MayOmit, parse_frame_pointer, [TRACKED],
"force use of the frame pointers"),
#[rustc_lint_opt_deny_field_access("use `Session::must_emit_unwind_tables` instead of this field")]
force_unwind_tables: Option<bool> = (None, parse_opt_bool, [TRACKED],
Expand Down
14 changes: 14 additions & 0 deletions compiler/rustc_target/src/spec/mod.rs
Original file line number Diff line number Diff line change
Expand Up @@ -1413,6 +1413,20 @@ pub enum FramePointer {
MayOmit,
}

impl FramePointer {
/// It is intended that the "force frame pointer" transition is "one way"
/// so this convenience assures such if used
#[inline]
pub fn ratchet(&mut self, rhs: FramePointer) -> FramePointer {
*self = match (*self, rhs) {
(FramePointer::Always, _) | (_, FramePointer::Always) => FramePointer::Always,
(FramePointer::NonLeaf, _) | (_, FramePointer::NonLeaf) => FramePointer::NonLeaf,
_ => FramePointer::MayOmit,
};
*self
}
workingjubilee marked this conversation as resolved.
Show resolved Hide resolved
}

impl FromStr for FramePointer {
type Err = ();
fn from_str(s: &str) -> Result<Self, ()> {
Expand Down
2 changes: 2 additions & 0 deletions src/tools/compiletest/src/header.rs
Original file line number Diff line number Diff line change
Expand Up @@ -779,6 +779,7 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
"ignore-haiku",
"ignore-horizon",
"ignore-i686-pc-windows-msvc",
"ignore-illumos",
"ignore-ios",
"ignore-linux",
"ignore-lldb",
Expand Down Expand Up @@ -843,6 +844,7 @@ const KNOWN_DIRECTIVE_NAMES: &[&str] = &[
"ignore-x32",
"ignore-x86",
"ignore-x86_64",
"ignore-x86_64-apple-darwin",
"ignore-x86_64-unknown-linux-gnu",
"incremental",
"known-bug",
Expand Down
16 changes: 14 additions & 2 deletions tests/codegen/force-frame-pointers.rs
Original file line number Diff line number Diff line change
@@ -1,6 +1,18 @@
//@ compile-flags: -C no-prepopulate-passes -C force-frame-pointers=y -Copt-level=0
//@ revisions: Always NonLeaf
//@ [Always] compile-flags: -Cforce-frame-pointers=yes
//@ [NonLeaf] compile-flags: -Cforce-frame-pointers=non-leaf
//@ compile-flags: -Zunstable-options
//@ compile-flags: -C no-prepopulate-passes -Copt-level=0
//@ [NonLeaf] ignore-illumos
//@ [NonLeaf] ignore-openbsd
//@ [NonLeaf] ignore-x86
//@ [NonLeaf] ignore-x86_64-apple-darwin
//@ [NonLeaf] ignore-windows-gnu
//@ [NonLeaf] ignore-thumb
// result is platform-dependent based on platform's frame pointer settings

#![crate_type = "lib"]

// CHECK: attributes #{{.*}} "frame-pointer"="all"
// Always: attributes #{{.*}} "frame-pointer"="all"
// NonLeaf: attributes #{{.*}} "frame-pointer"="non-leaf"
pub fn foo() {}
Loading