Skip to content

Commit

Permalink
[RISC-V] Do not force frame pointers
Browse files Browse the repository at this point in the history
We have been seeing some very inefficient code that went away when using
`-Cforce-frame-pointers=no`. For instance `core::ptr::drop_in_place` at
`-Oz` was compiled into a function which consisted entirely of saving
registers to the stack, then using the frame pointer to restore the same
registers (without any instructions between the prolog and epilog).

The RISC-V LLVM backend supports frame pointer elimination, so it makes
sense to allow this to happen when using Rust. It's not clear to me that
frame pointers have ever been required in the general case.

In rust-lang#61675 it was pointed out that this made reassembling
stack traces easier, which is true, but there is a code generation
option for forcing frame pointers, and I feel the default should not be
to require frame pointers, given it demonstrably makes code size worse
(around 10% in some embedded applications).

The kinds of targets mentioned in rust-lang#61675 are popular, but
should not dictate that code generation should be worse for all RISC-V
targets, especially as there is a way to use CFI information to
reconstruct the stack when the frame pointer is eliminated. It is also
a misconception that `fp` is always used for the frame pointer. `fp` is
an ABI name for `x8` (aka `s0`), and if no frame pointer is required,
`x8` may be used for other callee-saved values.

This commit does ensure that the standard library is built with unwind
tables, so that users do not need to rebuild the standard library in
order to get a backtrace that includes standard library calls (which is
the original reason for forcing frame pointers).
  • Loading branch information
lenary committed May 30, 2020
1 parent 91fb72a commit 3da3d15
Show file tree
Hide file tree
Showing 6 changed files with 10 additions and 5 deletions.
10 changes: 10 additions & 0 deletions src/bootstrap/compile.rs
Original file line number Diff line number Diff line change
Expand Up @@ -244,6 +244,16 @@ pub fn std_cargo(builder: &Builder<'_>, target: Interned<String>, stage: u32, ca
if stage >= 1 {
cargo.rustflag("-Cembed-bitcode=yes");
}

// By default, rustc does not include unwind tables unless they are required
// for a particular target. They are not required by RISC-V targets, but
// compiling the standard library with them means that users can get
// backtraces without having to recompile the standard library themselves.
//
// This choice was discussed in https://github.com/rust-lang/rust/pull/69890
if target.contains("riscv") {
cargo.rustflag("-Cforce-unwind-tables=yes");
}
}

#[derive(Debug, Copy, Clone, PartialEq, Eq, Hash)]
Expand Down
1 change: 0 additions & 1 deletion src/librustc_target/spec/riscv32i_unknown_none_elf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ pub fn target() -> TargetResult {
relocation_model: RelocModel::Static,
emit_debug_gdb_scripts: false,
abi_blacklist: super::riscv_base::abi_blacklist(),
eliminate_frame_pointer: false,
..Default::default()
},
})
Expand Down
1 change: 0 additions & 1 deletion src/librustc_target/spec/riscv32imac_unknown_none_elf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ pub fn target() -> TargetResult {
relocation_model: RelocModel::Static,
emit_debug_gdb_scripts: false,
abi_blacklist: super::riscv_base::abi_blacklist(),
eliminate_frame_pointer: false,
..Default::default()
},
})
Expand Down
1 change: 0 additions & 1 deletion src/librustc_target/spec/riscv32imc_unknown_none_elf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -25,7 +25,6 @@ pub fn target() -> TargetResult {
relocation_model: RelocModel::Static,
emit_debug_gdb_scripts: false,
abi_blacklist: super::riscv_base::abi_blacklist(),
eliminate_frame_pointer: false,
..Default::default()
},
})
Expand Down
1 change: 0 additions & 1 deletion src/librustc_target/spec/riscv64gc_unknown_none_elf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ pub fn target() -> TargetResult {
code_model: Some(CodeModel::Medium),
emit_debug_gdb_scripts: false,
abi_blacklist: super::riscv_base::abi_blacklist(),
eliminate_frame_pointer: false,
..Default::default()
},
})
Expand Down
1 change: 0 additions & 1 deletion src/librustc_target/spec/riscv64imac_unknown_none_elf.rs
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,6 @@ pub fn target() -> TargetResult {
code_model: Some(CodeModel::Medium),
emit_debug_gdb_scripts: false,
abi_blacklist: super::riscv_base::abi_blacklist(),
eliminate_frame_pointer: false,
..Default::default()
},
})
Expand Down

0 comments on commit 3da3d15

Please sign in to comment.