Auto merge of #69890 - lenary:lenary/riscv-frame-pointers, r=hanna-kr…

…uppe,Mark-Simulacrum

[RISC-V] Do not force frame pointers

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

---

I am partly posting this to get feedback from @fintelia who introduced the change to require frame pointers, and @hanna-kruppe who had issues with the original PR. I would understand if we wanted to remove this setting on only a subset of RISC-V targets, but my preference would be to remove this setting everywhere.

There are more details on the code size savings seen in Tock here: tock/tock#1660

src/bootstrap/compile.rs

@@ -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)]

src/librustc_target/spec/riscv32i_unknown_none_elf.rs

@@ -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()
         },
     })

src/librustc_target/spec/riscv32imac_unknown_none_elf.rs

@@ -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()
         },
     })

src/librustc_target/spec/riscv32imc_unknown_none_elf.rs

@@ -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()
         },
     })

src/librustc_target/spec/riscv64gc_unknown_none_elf.rs

@@ -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()
         },
     })

src/librustc_target/spec/riscv64imac_unknown_none_elf.rs

@@ -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()
         },
     })