Сompiler can't remove panic locations if they are not used in panic handler

Code

Code is minimized as much as possible and here is a demo repository: https://github.com/StackOverflowExcept1on/rust-regression

program - this is smart contract that panics and terminates with an error
project - some intermediate directory that is generated by our utility

git clone https://github.com/StackOverflowExcept1on/rust-regression.git
cd rust-regression/project
./check.sh

/home/user/rust-regression/project
   Compiling rustversion v1.0.17
   Compiling arrayvec v0.7.6
   Compiling wasm-program v0.1.0 (/home/user/rust-regression/wasm-program)
   Compiling wasm-project v0.1.0 (/home/user/rust-regression/wasm-project)
    Finished `release` profile [optimized] target(s) in 0.59s
grep: ./target/wasm32-unknown-unknown/release/wasm_program.wasm: binary file matches
/home/user/... found in WASM :(

I expected to see this happen: /home/user/... removed from wasm

Instead, this happened: /home/user/... is not removed from wasm

We want /home/user/... to always be removed from smart contracts, otherwise it reveals some information about creator name and also increases cost of uploading to blockchain. We tried using --remap-path-prefix, but it results in longer build times. On the old version, rustc compiler was so smart that it figured out how to remove all panic locations from the binary file.

Maybe related to #115974, but I haven't checked every commit. Although it is worth noting that #115974 is very useful as we can get panic message and remove panic location in stable rust.

Version it worked on

It most recently worked on: nightly-2024-06-12

$ rustc +nightly-2024-06-12 --version --verbose
rustc 1.81.0-nightly (d0227c6a1 2024-06-11)
binary: rustc
commit-hash: d0227c6a19c2d6e8dceb87c7a2776dc2b10d2a04
commit-date: 2024-06-11
host: x86_64-unknown-linux-gnu
release: 1.81.0-nightly
LLVM version: 18.1.7

Version with regression

Starting from nightly-2024-06-13, behavior has changed

$ rustc +nightly-2024-06-13 --version --verbose
rustc 1.81.0-nightly (8337ba918 2024-06-12)
binary: rustc
commit-hash: 8337ba9189de188e2ed417018af2bf17a57d51ac
commit-date: 2024-06-12
host: x86_64-unknown-linux-gnu
release: 1.81.0-nightly
LLVM version: 18.1.7

Related to #129080

@StackOverflowExcept1on

@StackOverflowExcept1on can you please elaborate about the longer build times you mention? Can you provide some numbers?

thanks

@apiraino

@apiraino we use the gear-wasm-builder utility to build our smart contracts. It is quite suboptimal because we have about 30-35 demo crates in the workspace, but only 1 crate can be built at a time. We had problem gear-tech/gear#2890, when the test build time increased from 45 sec to 9 minutes.

But we'd like to solve the problem described above, rather than using --remap-path-prefix. The compiler has been able to remove panic locations if they are not used in panic handler for last 2 years (I checked this on 2022 rustc). Now we have panic_info_message (since 1.81), but for some reason compiler can't strip panic locations.

@StackOverflowExcept1on

@StackOverflowExcept1on thanks for the reply.

I'm sorry to ask again, I still fail to see some data about increased build times. Do you have some CI logs? Some public code? A reproducible that shows a before and an after? Thanks again for your patience.

Does using cargo build -Zbuild-std=core,alloc -Zbuild-std-features=panic_immediate_abort --release --target wasm32-unknown-unknown work? panic_immediate_abort ensures that all panics are replaced with an aborting instruction, causing the panic handler to be skipped and panic locations to be omitted if you enable optimizations.

@bjorn3

@bjorn3 it works, but it's not what we want. Previously, no build flags were needed, only the panic_info_message feature was needed.

--remap-path-prefix, panic_immediate_abort and -Zlocation-detail=none are the intended options to keep panic message paths out of the binary. If anything else worked, that it by accident and likely not something we will try to restore. Also can you provide more details on

We tried using --remap-path-prefix, but it results in longer build times.

? It shouldn't affect build times at all.

@apiraino

@apiraino remapping was added to the gear-wasm-builder utility here: https://github.com/gear-tech/gear/pull/2865/files. I also want to show how to reproduce slow build with --remap-path-prefix, but without minimal example.

git clone https://github.com/gear-tech/gear.git
git checkout e0a7e979055508648832c4ada1ccf4cf31f9a3bf

# clean build

cargo test -p pallet-gear -r --no-run
    Finished `release` profile [optimized] target(s) in 7m 13s

GEAR_WASM_BUILDER_PATH_REMAPPING=1 cargo test -p pallet-gear -r --no-run
    Finished `release` profile [optimized] target(s) in 14m 02s

# rebuild

cargo test -p pallet-gear -r --no-run
    Finished `release` profile [optimized] target(s) in 1m 21s

GEAR_WASM_BUILDER_PATH_REMAPPING=1 cargo test -p pallet-gear -r --no-run
    Finished `release` profile [optimized] target(s) in 9m 33s

It may be the case that RUSTFLAGS was forgotten to be set somewhere, causing unnecessary rebuilds that cause the total build time to be higher.

@bjorn3

@bjorn3 CARGO_ENCODED_RUSTFLAGS will be set whenever GEAR_WASM_BUILDER_PATH_REMAPPING was passed to env. It's also worth noting that we're using cargo toolchain run cargo rustc -- rustc_flags and maybe this is some kind of flags caching bug? CARGO_ENCODED_RUSTFLAGS is used because it is global, unlike cargo rustc -- rustc_flags, and --remap-path-prefix works with it.

Ok, I probably found the reason #129330 (comment). The thing is that #115974 passes fmt::Arguments by value instead of reference and it seems to lead to worse optimization when using panic=abort, which also uses const_eval_select.

rust/library/core/src/panicking.rs

Lines 88 to 123 in eef00c8

    
           pub const fn panic_nounwind_fmt(fmt: fmt::Arguments<'_>, force_no_backtrace: bool) -> ! { 
        
               #[inline] // this should always be inlined into `panic_nounwind_fmt` 
        
               #[track_caller] 
        
               fn runtime(fmt: fmt::Arguments<'_>, force_no_backtrace: bool) -> ! { 
        
                   if cfg!(feature = "panic_immediate_abort") { 
        
                       super::intrinsics::abort() 
        
                   } 
        
                   // NOTE This function never crosses the FFI boundary; it's a Rust-to-Rust call 
        
                   // that gets resolved to the `#[panic_handler]` function. 
        
                   extern "Rust" { 
        
                       #[lang = "panic_impl"] 
        
                       fn panic_impl(pi: &PanicInfo<'_>) -> !; 
        
                   } 
        
                   // PanicInfo with the `can_unwind` flag set to false forces an abort. 
        
                   let pi = PanicInfo::new( 
        
                       fmt, 
        
                       Location::caller(), 
        
                       /* can_unwind */ false, 
        
                       force_no_backtrace, 
        
                   ); 
        
                   // SAFETY: `panic_impl` is defined in safe Rust code and thus is safe to call. 
        
                   unsafe { panic_impl(&pi) } 
        
               } 
        
               #[inline] 
        
               #[track_caller] 
        
               const fn comptime(fmt: fmt::Arguments<'_>, _force_no_backtrace: bool) -> ! { 
        
                   // We don't unwind anyway at compile-time so we can call the regular `panic_fmt`. 
        
                   panic_fmt(fmt); 
        
               } 
        
               super::intrinsics::const_eval_select((fmt, force_no_backtrace), comptime, runtime); 
        
           }

Here is my fix. I'll put this in the PR and it would be nice to run some benchmarks on this patch.

diff --git a/library/core/src/panic/panic_info.rs b/library/core/src/panic/panic_info.rs
index e4d0c897b65..af2c83b5460 100644
--- a/library/core/src/panic/panic_info.rs
+++ b/library/core/src/panic/panic_info.rs
@@ -12,7 +12,7 @@
 #[stable(feature = "panic_hooks", since = "1.10.0")]
 #[derive(Debug)]
 pub struct PanicInfo<'a> {
-    message: fmt::Arguments<'a>,
+    message: &'a fmt::Arguments<'a>,
     location: &'a Location<'a>,
     can_unwind: bool,
     force_no_backtrace: bool,
@@ -26,13 +26,13 @@ pub struct PanicInfo<'a> {
 /// See [`PanicInfo::message`].
 #[stable(feature = "panic_info_message", since = "1.81.0")]
 pub struct PanicMessage<'a> {
-    message: fmt::Arguments<'a>,
+    message: &'a fmt::Arguments<'a>,
 }
 
 impl<'a> PanicInfo<'a> {
     #[inline]
     pub(crate) fn new(
-        message: fmt::Arguments<'a>,
+        message: &'a fmt::Arguments<'a>,
         location: &'a Location<'a>,
         can_unwind: bool,
         force_no_backtrace: bool,
@@ -146,7 +146,7 @@ fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
         formatter.write_str("panicked at ")?;
         self.location.fmt(formatter)?;
         formatter.write_str(":\n")?;
-        formatter.write_fmt(self.message)?;
+        formatter.write_fmt(*self.message)?;
         Ok(())
     }
 }
@@ -177,7 +177,7 @@ pub const fn as_str(&self) -> Option<&'static str> {
 impl Display for PanicMessage<'_> {
     #[inline]
     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
-        formatter.write_fmt(self.message)
+        formatter.write_fmt(*self.message)
     }
 }
 
@@ -185,6 +185,6 @@ fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
 impl fmt::Debug for PanicMessage<'_> {
     #[inline]
     fn fmt(&self, formatter: &mut fmt::Formatter<'_>) -> fmt::Result {
-        formatter.write_fmt(self.message)
+        formatter.write_fmt(*self.message)
     }
 }
diff --git a/library/core/src/panicking.rs b/library/core/src/panicking.rs
index 7affe638257..87eb9eefb54 100644
--- a/library/core/src/panicking.rs
+++ b/library/core/src/panicking.rs
@@ -64,7 +64,7 @@ pub const fn panic_fmt(fmt: fmt::Arguments<'_>) -> ! {
     }
 
     let pi = PanicInfo::new(
-        fmt,
+        &fmt,
         Location::caller(),
         /* can_unwind */ true,
         /* force_no_backtrace */ false,
@@ -102,7 +102,7 @@ fn runtime(fmt: fmt::Arguments<'_>, force_no_backtrace: bool) -> ! {
 
         // PanicInfo with the `can_unwind` flag set to false forces an abort.
         let pi = PanicInfo::new(
-            fmt,
+            &fmt,
             Location::caller(),
             /* can_unwind */ false,
             force_no_backtrace,

@rustbot

@rustbot modify labels: +regression-from-stable-to-beta -regression-untriaged

Auto merge of rust-lang#129491 - StackOverflowExcept1on:master, r=<try>

@rustbot

@rustbot modify labels: -regression-from-stable-to-beta +regression-from-stable-to-stable
(Actually from just stabilized nightly feature to stable)

Auto merge of rust-lang#129491 - StackOverflowExcept1on:master, r=m-o…

Navigation Menu

Repository navigation

Сompiler can't remove panic locations if they are not used in panic handler #129330

Code

Version it worked on

Version with regression

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

	pub const fn panic_nounwind_fmt(fmt: fmt::Arguments<'_>, force_no_backtrace: bool) -> ! {
	#[inline] // this should always be inlined into `panic_nounwind_fmt`
	#[track_caller]
	fn runtime(fmt: fmt::Arguments<'_>, force_no_backtrace: bool) -> ! {
	if cfg!(feature = "panic_immediate_abort") {
	super::intrinsics::abort()
	}

	// NOTE This function never crosses the FFI boundary; it's a Rust-to-Rust call
	// that gets resolved to the `#[panic_handler]` function.
	extern "Rust" {
	#[lang = "panic_impl"]
	fn panic_impl(pi: &PanicInfo<'_>) -> !;
	}

	// PanicInfo with the `can_unwind` flag set to false forces an abort.
	let pi = PanicInfo::new(
	fmt,
	Location::caller(),
	/* can_unwind */ false,
	force_no_backtrace,
	);

	// SAFETY: `panic_impl` is defined in safe Rust code and thus is safe to call.
	unsafe { panic_impl(&pi) }
	}

	#[inline]
	#[track_caller]
	const fn comptime(fmt: fmt::Arguments<'_>, _force_no_backtrace: bool) -> ! {
	// We don't unwind anyway at compile-time so we can call the regular `panic_fmt`.
	panic_fmt(fmt);
	}

	super::intrinsics::const_eval_select((fmt, force_no_backtrace), comptime, runtime);
	}

Сompiler can't remove panic locations if they are not used in panic handler #129330

Description

Code

Version it worked on

Version with regression

Activity

apiraino commented on Aug 21, 2024

apiraino commented on Aug 21, 2024

StackOverflowExcept1on commented on Aug 21, 2024

apiraino commented on Aug 21, 2024

bjorn3 commented on Aug 22, 2024

StackOverflowExcept1on commented on Aug 22, 2024

bjorn3 commented on Aug 22, 2024

StackOverflowExcept1on commented on Aug 23, 2024

bjorn3 commented on Aug 23, 2024

StackOverflowExcept1on commented on Aug 23, 2024

StackOverflowExcept1on commented on Aug 23, 2024

StackOverflowExcept1on commented on Aug 23, 2024

StackOverflowExcept1on commented on Sep 5, 2024

Metadata

Metadata

Assignees

Labels

Type

Projects

Milestone

Relationships

Development

Issue actions