cross-crate inlining breaks compare of functions passed between crates

[AzazKamaz]

This code panics when compiled with #116505 with cross-crate inlining being turned on:

// src/main.rs
use lib;
fn main() { lib::verify(lib::create()); }

// src/lib.rs
fn inner() {}
pub fn create() -> fn() { inner }
pub fn verify(value: fn()) { assert_eq!(value, inner as fn()) }

# Cargo.toml
[package]
name = "lib"
version = "0.1.0"
edition = "2021"

cargo +nightly-2023-10-18 run --release - ok
cargo +nightly-2023-10-19 run --release - panics
cargo +nightly-2023-10-19 run - ok (opt-level = 2 incremental = false to panic)

It is clearly caused by #116505 because:

• it is ok on ca89f73 but panics on 5d5edf0
• it requires opt-level = 2, incremental = false and not -Zcross-crate-inline-threshold=0 (feature to be enabled)

Ways to fix this minimal example:

• make create() inline(never) - probably this is not a root cause fix
• set -Zcross-crate-inline-threshold=0

Originally I found this problem in Embassy (embedded framework using async). They are using such a comparison to ensure that passed Waker was created by the Embassy executor. In Embassy it is possible to fix it by either:

• making from_task() inline(never)
• or making VTABLE static

Meta

rustc +nightly --version --verbose (bug still here):

rustc 1.75.0-nightly (1c05d50c8 2023-10-21)
binary: rustc
commit-hash: 1c05d50c8403c56d9a8b6fb871f15aaa26fb5d07
commit-date: 2023-10-21
host: aarch64-apple-darwin
release: 1.75.0-nightly
LLVM version: 17.0.3

Precise versions (just skip, too much extra info)

rustup build - ok

rustc 1.75.0-nightly (09df6108c 2023-10-17)
binary: rustc
commit-hash: 09df6108c84fdec400043d99d9ee232336fd5a9f
commit-date: 2023-10-17
host: aarch64-apple-darwin
release: 1.75.0-nightly
LLVM version: 17.0.2

custom build - ok

rustc 1.75.0-nightly (ca89f732e 2023-10-18)
binary: rustc
commit-hash: ca89f732ec0f910fc92111a45dd7e6829baa9d4b
commit-date: 2023-10-18
host: aarch64-apple-darwin
release: 1.75.0-nightly
LLVM version: 17.0.3

custom build - panics

rustc 1.75.0-nightly (5d5edf024 2023-10-18)
binary: rustc
commit-hash: 5d5edf0248d967baa6ac5cbea09b91c7c9947942
commit-date: 2023-10-18
host: aarch64-apple-darwin
release: 1.75.0-nightly
LLVM version: 17.0.3

rustup build - panics

rustc 1.75.0-nightly (0039d739d 2023-10-18)
binary: rustc
commit-hash: 0039d739d40a076334e111488946441378d11cd7
commit-date: 2023-10-18
host: aarch64-apple-darwin
release: 1.75.0-nightly
LLVM version: 17.0.3

Backtrace

stack backtrace:
   0: _rust_begin_unwind
   1: core::panicking::panic_fmt
   2: core::panicking::assert_failed_inner
   3: core::panicking::assert_failed
   4: lib::verify

Example more like in Embassy

// src/lib.rs
#![feature(waker_getters)]

use core::task::{RawWaker, RawWakerVTable, Waker};

const VTABLE: RawWakerVTable = RawWakerVTable::new(clone, wake, wake, drop);

unsafe fn clone(p: *const ()) -> RawWaker { RawWaker::new(p, &VTABLE) }
unsafe fn wake(_: *const ()) {}
unsafe fn drop(_: *const ()) {}

pub fn create() -> Waker {
    unsafe { Waker::from_raw(clone(core::ptr::null())) }
}

pub fn verify(waker: Waker) {
    assert_eq!(waker.as_raw().vtable(), &VTABLE);
}

[AzazKamaz]

This may be related to rust-lang/unsafe-code-guidelines#239 but here are some points on this case:

• it also panics with codegen-units = 1
• PartialEq for RawWakerVTable makes no sense?

[asquared31415]

rust-lang/miri#2882 (comment)

Basically Rust makes no guarantees at all when it comes to fn ptr equality: the 'same' function can have multiple addresses (e.g. if it is called from multiple codegen units / crates, leading to multiple monomorphizations) and different functions can have the same address.

This probably isn't a bug, but deserves some extra docs and maybe a lint.

[AzazKamaz]

Agree, it's not a bug, but it really does look like one without a lint when it breaks in release..
With such a description of what happens we can I guess construct a valid fix to original code:

// src/lib.rs
static INNER: fn() = inner;
fn inner() {}
pub fn create() -> fn() { INNER }
pub fn verify(value: fn()) { assert_eq!(value, INNER) }

In this code we are using static global variable so any access to inner() address happens from single place and we can actually compare such a pointers. (static variables are guaranteed to have a single fixed location in memory as stated in the reference, in the first edition of the book and in the current one)

On the other hand, making create() inline(never) isn't really a valid fix since it is about inlining and doesn't ensure/guarantee that the function will exists only once in the memory

[AzazKamaz]

As for a possible place to document such a thing I see Advanced Functions and Closures section of the book and I guess Items/Functions page of the reference

[DCNick3]

Briefly looking at LLVM IR (RUSTFLAGS="--emit llvm-ir" cargo +nightly-2023-10-19 rustc --release, look inside target/release/deps) this is what seems to be happening:

1. create is inlined from the lib to the bin crate
2. inner is also inlined from the lib to the bin crate
3. verify does NOT get inlined (because it does not follow the "no function calls" heuristic)
4. two copies of inner now: one in the bin crate for create, another in the lib crate for verify
5. compare pointers => boom

This seems reasonable, albeit a bit surprising

[DCNick3]

One way to stop this surprising behavior might be to not inline functions that take address of other functions? Might be too strict though, forfeiting inlining just because somebody might want to compare function pointers doesn't seem reasonable

[AzazKamaz]

Another way to stop this behavior is to not inline functions when their addresses are taken and only inline in case of a call. It is more complicated I guess but seems more sensible. There is no point in inlining a function that is referenced by address (because it needs to be separate) or duplicating it (it will be the same)