Skip to content

Miscompile when comparing function pointers #54696

New issue

Have a question about this project? # for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “#”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? # to your account

Closed
dtolnay opened this issue Sep 30, 2018 · 7 comments
Closed

Miscompile when comparing function pointers #54696

dtolnay opened this issue Sep 30, 2018 · 7 comments
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another.

Comments

@dtolnay
Copy link
Member

dtolnay commented Sep 30, 2018

From rustc 1.4.0 through 1.25.0, the following code compiles and prints true. This is the way I would expect it to work.

fn main() {
    println!("{}", main as fn() == main as fn());
}

On 1.26.0 and 1.27.0, compilation emits a warning and we get a segfault at runtime.

warning: constant evaluation error
 --> src/main.rs:2:20
  |
2 |     println!("{}", main as fn() == main as fn());
  |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ "Pointer arithmetic or comparison" needs an rfc before being allowed inside constants
  |
  = note: #[warn(const_err)] on by default
Segmentation fault (core dumped)

On 1.28.0 the code fails to compile.

error: this expression will panic at runtime
 --> src/main.rs:2:20
  |
2 |     println!("{}", main as fn() == main as fn());
  |                    ^^^^^^^^^^^^^^^^^^^^^^^^^^^^ "Pointer arithmetic or comparison" needs an rfc before being allowed inside constants
  |
  = note: #[deny(const_err)] on by default

On 1.29.0 through rustc 1.30.0-nightly (bb0896a 2018-09-29) the code compiles but is not correct.

Illegal instruction (core dumped)

Mentioning @oli-obk because your name is on this line so you may know what's up:

ConstEvalError::NeedsRfc("Pointer arithmetic or comparison".to_string()).into(),

@dtolnay dtolnay added the regression-from-stable-to-stable Performance or correctness regression from one stable version to another. label Sep 30, 2018
@arielb1
Copy link
Contributor

arielb1 commented Sep 30, 2018

This emits an llvm.trap even in debug mode. cc @RalfJung

@arielb1
Copy link
Contributor

arielb1 commented Sep 30, 2018

Looks like promotion is promoting the equality comparison, which then causes a const-eval error because the comparison is not actually valid in constants.

Minified:

fn main() {
    &(main as fn() == main as fn());
}

@RalfJung
Copy link
Member

Uh-oh. Yeah we shouldn't promote this. I hope. The segfaults are likely from UB; we fixed "promotion causes UB" by emitting a trap instead for what should be unreachable code... but this reaches that code. :/

Cc @eddyb @oli-obk

@RalfJung
Copy link
Member

This is not as easy as I thought... we can probably easily rule out promoting comparing things of function type (though somehow my first attempt does not work), but when comparing arbitrary types that may contain a function, things get much harder.

@RalfJung
Copy link
Member

Fix submitted at #54702

@oli-obk
Copy link
Contributor

oli-obk commented Oct 1, 2018

We don't need to do anything special about aggregates, as their comparisons won't get promoted since they depend on trait impls and aren't builtin.

@RalfJung
Copy link
Member

RalfJung commented Oct 1, 2018

Ah, great. I entirely forgot that this is pre-desugaring so we are seeing binary operators that will become method calls...

pietroalbini added a commit to pietroalbini/rust that referenced this issue Oct 2, 2018
do not promote comparing function pointers

This *could* break existing code that relied on fn ptr comparison getting promoted to `'static` lifetime.

Fixes rust-lang#54696
pietroalbini added a commit to pietroalbini/rust that referenced this issue Oct 2, 2018
do not promote comparing function pointers

This *could* break existing code that relied on fn ptr comparison getting promoted to `'static` lifetime.

Fixes rust-lang#54696
# for free to join this conversation on GitHub. Already have an account? # to comment
Labels
regression-from-stable-to-stable Performance or correctness regression from one stable version to another.
Projects
None yet
Development

No branches or pull requests

4 participants