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

Machineary behind an unreachable panic! ends-up in final optimized binary #121879

Closed
c410-f3r opened this issue Mar 2, 2024 · 6 comments
Closed
Labels
C-bug Category: This is a bug.

Comments

@c410-f3r
Copy link
Contributor

c410-f3r commented Mar 2, 2024

Well, unless I don't know how to read assembly...

#![feature(unchecked_math)]
#![feature(hint_assert_unchecked)]

pub struct BytesVector {
  vec: Vec<u8>,
}

impl BytesVector {
  #[inline(always)]
  pub fn reserve(&mut self, additional: usize) {
    self.vec.reserve(additional);
    unsafe {
      core::hint::assert_unchecked(self.vec.len().unchecked_add(additional) <= self.vec.capacity());
    }
  }

  #[inline]
  pub fn push(&mut self, value: u8) {
    if self.vec.len() >= self.vec.capacity() {
      _unreachable();
    }
    unsafe {
      core::ptr::write(self.vec.as_mut_ptr().add(self.vec.len()), value);
      let new_len = self.vec.len().unchecked_add(1);
      self.vec.set_len(new_len);
    }
  }
}

#[cold]
#[inline(never)]
#[track_caller]
const fn _unreachable() -> ! {
    panic!("Peek a boo!");
}

pub fn foo(v: &mut BytesVector) {
    v.reserve(2);
    v.push(0);
    v.push(1);
}

https://godbolt.org/z/4ca98x5s6

In foo, the assert_unchecked hint correctly tells that push shouldn't invoke _unreachable but the formatting machinery behind panic! still ends-up in the final optimized binary. The removal of cold or inline(never) does not alter this behaviour but regardless, the compiler should infer that no-one is using such thing.

On the other side, ad-hoc panic! in the push function correctly erases everything.

@c410-f3r c410-f3r added the C-bug Category: This is a bug. label Mar 2, 2024
@rustbot rustbot added the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 2, 2024
@c410-f3r
Copy link
Contributor Author

c410-f3r commented Mar 2, 2024

Probably a LLVM thing? cc @nikic

@ProgramCrafter
Copy link

Doesn't reproduce on play.rust-lang.org (release, 1.78.0-nightly) with main function added.
https://play.rust-lang.org/?version=nightly&mode=release&edition=2021&gist=41cee2ecbe0b343ff62b46424ee0e0af

alloc::raw_vec::RawVec<T,A>::reserve::do_reserve_and_handle:
# ...
.LBB6_8:
	callq	*alloc::raw_vec::capacity_overflow@GOTPCREL(%rip)
.LBB6_6:
	movq	24(%rsp), %rsi
	callq	*alloc::alloc::handle_alloc_error@GOTPCREL(%rip)
                                        # -- End function

playground::foo: # @playground::foo
# %bb.0:
	pushq	%rbx
	movq	(%rdi), %rax
	movq	16(%rdi), %rsi
	subq	%rsi, %rax
	cmpq	$1, %rax
	jbe	.LBB7_1

.LBB7_2:
	movq	8(%rdi), %rax
	movw	$256, (%rax,%rsi)               # imm = 0x100
	addq	$2, %rsi
	movq	%rsi, 16(%rdi)
	popq	%rbx
	retq

.LBB7_1:
	movq	%rdi, %rbx
	callq	alloc::raw_vec::RawVec<T,A>::reserve::do_reserve_and_handle
	movq	%rbx, %rdi
	movq	16(%rbx), %rsi
	jmp	.LBB7_2
                                        # -- End function

playground::main: # @playground::main
# ...

@ProgramCrafter
Copy link

@rustbot +requires-nightly

@bjorn3
Copy link
Member

bjorn3 commented Mar 2, 2024

I only see a single call to panic_fmt in the linked godbolt code. This is inside the _unreachable() function, which is never called, but still codegened as it is referenced from a public inlineable function (BytesVector::push) and thus could be referenced if optimizations are disabled for another crate which calls BytesVector::push. If you were to compile it as executable instead of as library (godbolt always compiles as library), then it would be removed after optimizations. Now it merely gets removed during linking due to --gc-sections.

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Mar 2, 2024

but still codegened as it is referenced from a public inlineable function

Oh... That makes sense... I guess a local literal panic! in push won't be generated because it is in the standard library?!

Still not sure why #[inline] const fn _unreachable() -> ! { ... } isn't also generated but it probably isn't a problem with LLVM!?

@c410-f3r
Copy link
Contributor Author

c410-f3r commented Mar 2, 2024

OK, thanks @bjorn3 and @ProgramCrafter

@c410-f3r c410-f3r closed this as completed Mar 2, 2024
@jieyouxu jieyouxu removed the needs-triage This issue may need triage. Remove it if it has been sufficiently triaged. label Mar 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
C-bug Category: This is a bug.
Projects
None yet
Development

No branches or pull requests

5 participants