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

const-eval: error when encountering references to functions / vtables #121199

Closed
wants to merge 2 commits into from

Conversation

RalfJung
Copy link
Member

Such references sound like an exceedingly bad idea... just use raw pointers if you must do things like this.

But we should crater this, to be on the safe side.

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2024

r? @fmease

rustbot has assigned @fmease.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Feb 16, 2024
@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2024

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

@RalfJung
Copy link
Member Author

r? @oli
@bors try

@rustbot
Copy link
Collaborator

rustbot commented Feb 16, 2024

Failed to set assignee to oli: invalid assignee

Note: Only org members with at least the repository "read" role, users with write permissions, or people who have commented on the PR may be assigned.

@RalfJung
Copy link
Member Author

Eh, sorry.
r? @oli-obk

@rustbot rustbot assigned oli-obk and unassigned fmease Feb 16, 2024
bors added a commit to rust-lang-ci/rust that referenced this pull request Feb 16, 2024
const-eval: error when encountering references to functions / vtables

Such references sound like an exceedingly bad idea... just use raw pointers if you must do things like this.

But we should crater this, to be on the safe side.
@bors
Copy link
Contributor

bors commented Feb 16, 2024

⌛ Trying commit 6563788 with merge aface90...

@bors
Copy link
Contributor

bors commented Feb 16, 2024

☀️ Try build successful - checks-actions
Build commit: aface90 (aface909a877127f24c62858518cef2d593e2ea1)

@RalfJung
Copy link
Member Author

@craterbot check

@craterbot
Copy link
Collaborator

👌 Experiment pr-121199 created and queued.
🤖 Automatically detected try build aface90
🔍 You can check out the queue and this experiment's details.

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-crater Status: Waiting on a crater run to be completed. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Feb 16, 2024
@craterbot
Copy link
Collaborator

🚧 Experiment pr-121199 is now running

ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot
Copy link
Collaborator

🎉 Experiment pr-121199 is completed!
📊 3 regressed and 3 fixed (417891 total)
📰 Open the full report.

⚠️ If you notice any spurious failure please add them to the blacklist!
ℹ️ Crater is a tool to run experiments across parts of the Rust ecosystem. Learn more

@craterbot craterbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. and removed S-waiting-on-crater Status: Waiting on a crater run to be completed. labels Feb 26, 2024
@RalfJung
Copy link
Member Author

Okay two of these are legit regressions -- some people are actually creating references to vtables. oO

I will have to check more closely if what they are doing makes any sense.

@RalfJung
Copy link
Member Author

rattish seems to be basically this code:

#![feature(ptr_metadata)]
use std::ptr;

pub type Metadata<U> = <U as ptr::Pointee>::Metadata;
type T = i32;
type U = dyn PartialEq<T>;
const ADDRESS: *mut () = 0xdeadbeef_usize as _;
const METADATA: Metadata<U> = ptr::metadata::<U>(ptr::null::<T>());

I guess that is this reference:

pub struct DynMetadata<Dyn: ?Sized> {
    vtable_ptr: &'static VTable,
    phantom: crate::marker::PhantomData<Dyn>,
}

So... are we sure we want this to be a reference? Can't it be NonNull?

The other one also involves DynMetadata, so that would fix both of them.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 29, 2024

I'm gonna assume no one actually found it important to have it be a reference. It was just natural to assume that since we know the pointer is always valid, we can also just use a reference to statically signal that.

Then again... Why not permit references to vtables as long as the pointee type is an extern type? That seems like the one use case for it that is not going to be biting us

@RalfJung
Copy link
Member Author

Then again... Why not permit references to vtables as long as the pointee type is an extern type? That seems like the one use case for it that is not going to be biting us

I was just thinking since there's no actual memory there (vtables are magic objects that exist outside of addressable memory, there's just a zero-sized "handle" representing them being put at some address so that we can point to them), we shouldn't allow such references.

But maybe it's not worth it...

@RalfJung
Copy link
Member Author

RalfJung commented Feb 29, 2024 via email

@RalfJung RalfJung closed this Feb 29, 2024
@RalfJung RalfJung deleted the ref-to-vtable-fn branch February 29, 2024 17:22
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Feb 29, 2024
add const test for ptr::metadata

rust-lang#121199 uncovered this as a gap in our test suite.

r? `@oli-obk`
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Mar 1, 2024
Rollup merge of rust-lang#121806 - RalfJung:const-metadata, r=oli-obk

add const test for ptr::metadata

rust-lang#121199 uncovered this as a gap in our test suite.

r? `@oli-obk`
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

6 participants