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

Storing a function pointer in a const usize works on nightly #51559

Closed
gnzlbg opened this issue Jun 14, 2018 · 17 comments · Fixed by #62930
Closed

Storing a function pointer in a const usize works on nightly #51559

gnzlbg opened this issue Jun 14, 2018 · 17 comments · Fixed by #62930
Labels
A-const-eval Area: constant evaluation (mir interpretation) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.

Comments

@gnzlbg
Copy link
Contributor

gnzlbg commented Jun 14, 2018

Some people were of the opinion that this shouldn't work, but it currently does. playground:

#![feature(atomic_integers, const_fn)]
use std::sync::atomic::AtomicUsize;
use std::sync::atomic::Ordering;

const unsafe fn transmute(x: *mut u8) -> usize {
    union T {
        a: *mut u8,
        b: usize
    }
    T { a: x }.b
}

const BAR: *mut u8 = ((|| 3) as fn() -> i32) as *mut u8;
const FOO: AtomicUsize = AtomicUsize::new(unsafe { transmute(BAR) });
// static FOO: AtomicUsize = AtomicUsize::new(unsafe { transmute(BAR) }); // ALSO OK
// const BAZ: AtomicUsize = AtomicUsize::new(BAR as usize); ERRORs

fn main() {
    let l = FOO.load(Ordering::Relaxed);
    let l: fn() -> i32 = unsafe { std::mem::transmute(l) };
    assert_eq!(l(), 3);
}

cc @rkruppe @eddyb @oli-obk

@gnzlbg gnzlbg changed the title Storing a function pointer in a static usize works on nightly Storing a function pointer in a const usize works on nightly Jun 14, 2018
@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 14, 2018

cc @RalfJung

@oli-obk
Copy link
Contributor

oli-obk commented Jun 14, 2018

Some people were of the opinion that this shouldn't work

It should work, it's just super scary.

EDIT: it should not and does not work. If we allowed this, various surprising things can happen, especially around implicit promotion. If you want to store pointers and integers, you can use a raw pointer.

@scottmcm
Copy link
Member

It should work

For my own education, can you elaborate on why function pointers are ok? I'd thought that the runtime addresses of things -- including functions -- shouldn't be accessible at const-time since we don't know where they'll live at runtime. Could I make a const array that usize long?

@oli-obk
Copy link
Contributor

oli-obk commented Jun 15, 2018

When compiling to LLVM, the real function handle or pointer address is obtained. If you try doing anything weird with pointers at compile time, you will quickly notice that you are not at runtime. For example, you cannot divide such a usize by anything. You can add or subtract integers, because that's just pointer offsetting, but you can't inspect the address in any way.

This also means that no, you cannot use this usize for array lengths or enum discriminants. This is due to the fact that miri pointers are not just addresses, but abstract pointers that are in a separate layer from the bytes of normal memory like the one of integers.

@scottmcm
Copy link
Member

Thanks for the explanation.

I guess that means that such a usize couldn't be passed to a const generic either?

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 15, 2018

I guess that means that such a usize couldn't be passed to a const generic either?

The thing is const generics don't know where the usize comes from, so they could try to do something with it that is not allowed if it is just a pointer address. Until monomorphization we can't know if there is an issue.

Note that converting a pointer to an usize and back just to be able to put it in const, statics, or use it with const generics, is a real pain. We would be better off in this case with an AtomicPtr<T> type, such that I can write AtomicPtr<fn()->i32> and avoid the conversion to usize.

For const generics, C++ accepts function pointers at the type level and that works just fine with clang, so I expect fn bar<const PTR: fn()->i32>() {} to work fine as well.

@hanna-kruppe
Copy link
Contributor

How would AtomicPtr<T> work?

  • is AtomicPtr<i32> disallowed but AtomicPtr<*const i32> and AtomicPtr<fn(&str) -> &str> work? that seems very weird and special cased (especially for fn types that involve higher rank lifetimes, as in my example)
  • or is AtomicPtr<T> ~= *mut T? then AtomicPtr<fn()> is a double indirection and to use it you need to worry about managing the memory it points to (which contains the fn())

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 15, 2018

How would AtomicPtr work?

I haven't thought this through beyond having used the atomic pointers in the C++ standard library: http://en.cppreference.com/w/cpp/atomic/atomic

One way to implement this could be AtomicPtr<T> where T: AtomicPtrConstraint where we provide blanket impls for &'a, &'a mut, *const, *mut, fn() -> T, fn(T) -> U, fn(T, U) -> V, ... or use some compiler magic to achieve a similar effect...

@eddyb
Copy link
Member

eddyb commented Jun 15, 2018

This is a bit offtopic, but regarding AtomicPtr, @nikomatsakis mentioned at least once the possibility of introducing some new type, to make fn(A) -> B sugar for &ThatType<(A,), B>.
I guess it can't entirely be "sugar", because of existing impls, but at least both could work the same.

@RalfJung
Copy link
Member

Hehe, this is cute. :) And I agree it is working as intended. (Nice that the translation to LLVM actually gets this right, should there be a testcase to make sure it stays that way? :D )

@oli-obk oli-obk added E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added. A-const-eval Area: constant evaluation (mir interpretation) labels Jun 19, 2018
@oli-obk
Copy link
Contributor

oli-obk commented Jun 20, 2018

So... other than testing this, all we need is a PR that adds *const T -> usize casts in constants under a feature gate (and makes those casts unsafe in constants and const fn)

@gnzlbg
Copy link
Contributor Author

gnzlbg commented Jun 20, 2018

I've opened rust-lang/rfcs#2481 to brainstorm how to either add an AtomicFnPtr type or retrofit AtomicPtr to support fn items.

@oli-obk
Copy link
Contributor

oli-obk commented Jan 28, 2019

These kind of operations are now unsafe in const contexts (impl PR: #55009)

@oli-obk oli-obk closed this as completed Jan 28, 2019
@RalfJung
Copy link
Member

So do we have a test that you cannot use such usize as array length? :D

@oli-obk
Copy link
Contributor

oli-obk commented Jan 28, 2019

hmm... apparently not...

Test-instructions:

  1. use transmute (via the const_transmute feature gate) to turn various kinds of references and function pointers into usize
  2. use these expressions in
    • array length,
    • repeat lengths
    • and enum discriminant initializers (e.g. enum Foo { Bar = unsafe { transmute(main) } })

Note: Do not use const items, everything needs to be inline in the use site to be properly tested

@oli-obk oli-obk reopened this Jan 28, 2019
@oli-obk oli-obk added the E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. label Jan 28, 2019
@kazcw
Copy link

kazcw commented May 10, 2019

It seems this is no longer allowed by any means:

#![feature(const_raw_ptr_to_usize_cast)]

const BAR: *mut () = ((|| 3) as fn() -> i32) as *mut ();
pub const FOO: usize = unsafe { BAR as usize };

fn main() {}
   Compiling playground v0.0.1 (/playground)
error[E0080]: it is undefined behavior to use this value
 --> src/main.rs:4:1
  |
4 | pub const FOO: usize = unsafe { BAR as usize };
  | ^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^^ type validation failed: encountered a pointer, but expected initialized plain (non-pointer) bytes
  |
  = note: The rules on what exactly is undefined behavior aren't clear, so this check might be overzealous. Please open an issue on the rust compiler repository if you believe it should not be considered undefined behavior

error: aborting due to previous error

For more information about this error, try `rustc --explain E0080`.
error: Could not compile `playground`.

To learn more, run the command again with --verbose.

@oli-obk
Copy link
Contributor

oli-obk commented May 10, 2019

Yes that is intended. This issue is for adding a regression test testing what happens when you use a pointer casted to a usize in an array length.

iluuu1994 added a commit to iluuu1994/rust that referenced this issue Jul 24, 2019
Centril added a commit to Centril/rust that referenced this issue Jul 25, 2019
bors added a commit that referenced this issue Jul 25, 2019
Rollup of 15 pull requests

Successful merges:

 - #60066 (Stabilize the type_name intrinsic in core::any)
 - #60938 (rustdoc: make #[doc(include)] relative to the containing file)
 - #61884 (Stablize Euclidean Modulo (feature euclidean_division))
 - #61890 (Fix some sanity checks)
 - #62528 (Add joining slices of slices with a slice separator, not just a single item)
 - #62707 (Add tests for overlapping explicitly dropped locals in generators)
 - #62735 (Turn `#[global_allocator]` into a regular attribute macro)
 - #62822 (Improve some pointer-related documentation)
 - #62887 (Make the parser TokenStream more resilient after mismatched delimiter recovery)
 - #62921 (Add method disambiguation help for trait implementation)
 - #62930 (Add test for #51559)
 - #62942 (Use match ergonomics in Condvar documentation)
 - #62977 (Fix inconsistent highlight blocks.)
 - #62978 (Remove `cfg(bootstrap)` code for array implementations)
 - #62981 (Add note suggesting to borrow a String argument to find)

Failed merges:

 - #62964 (clarify and unify some type test names)

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-const-eval Area: constant evaluation (mir interpretation) E-easy Call for participation: Easy difficulty. Experience needed to fix: Not much. Good first issue. E-needs-test Call for participation: An issue has been fixed and does not reproduce, but no test has been added.
Projects
None yet
Development

Successfully merging a pull request may close this issue.

7 participants