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)

[saethlin]

I'm commenting here because I want to be helpful. I'm afraid that the way this issue leaves off without the comment I'm writing makes it sound like the compiler might be changed in order to preserve the previous behavior. I am glad to see that you promptly updated embassy in embassy-rs/embassy#2112.

I do not expect anyone to implement the suggestions you both made; I'll admit I am not the most experienced compiler contributor but they both sound difficult/impossible for rustc-specific reasons.

One problem is that "cross-crate-inlining" is not actual inlining, it is a change to the code generation strategy for a function. A function which is cross-crate-inlinable gets the behavior of #[inline]; we generate code for it lazily, and in every codegen unit that references it, and with weak linkage so that duplicates are permitted. So the address of a cross-crate-inlinable function can become unstable even if the inlining optimization never fires on it.

The other problem is that cross-crate-inlinability is determined early in compilation, because it needs to inform whether other functions and static get internal or external linkage. A function which is cross-crate-inlinable exposes any static or function that it references. Since this whole process happens early, in rustc it happens before monomorphization. Disabling cross-crate-inlining for functions that reference other functions wouldn't just shut it off for all functions with a call, it would also turn it off for any function that takes the address of a generic which we can't prove is not a function.

[DCNick3]

That's completely fair, I don't know that much about rustc, so my suggestions were more like guesses how I thought this could potentially be implemented. Thanks for taking time to point out why is this infeasible, it's definitely helpful

For the problem at hand though, I guess this just means that we shouldn't rely on function address, which has been true already. Just that there's now more optimizations that can lead to problems.

[RalfJung]

Do we have an issue tracking "function pointer comparison is non-deterministic", with the goal of having better docs and lints? Basically the fn ptr equivalent of #117717, #69757, #99388.

[RalfJung]

Would you consider this to be fixed by #120880? That adds a note on the fn type docs indicating that comparing function pointers is unreliable. This is hard to find, but I'm also not sure what would be a better place.

[DCNick3]

Maybe also add a similar note to RawWakerVTable? Otherwise, seems good to me

[RalfJung]

Oh you mean, because it has fn ptr fields? Hm yeah fair, I do wonder why that type implements PartialEq to begin with...

[DCNick3]

Yeah, this is how the issue has started: embassy compares RawWakerVTable from a waker to check if belongs to its own executor.

AFAIU, what it does now is still not guaranteed to work, but replacing const by static stopped the duplicated vtables from appearing in different compilation units.