Skip to content

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

Closed
RalfJung wants to merge 2 commits into
rust-lang:masterfrom
RalfJung:ref-to-vtable-fn
Closed

const-eval: error when encountering references to functions / vtables#121199
RalfJung wants to merge 2 commits into
rust-lang:masterfrom
RalfJung:ref-to-vtable-fn

Conversation

@RalfJung

Copy link
Copy Markdown
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

rustbot commented Feb 16, 2024

Copy link
Copy Markdown
Collaborator

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

rustbot commented Feb 16, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
Member Author

r? @oli
@bors try

@rustbot

rustbot commented Feb 16, 2024

Copy link
Copy Markdown
Collaborator

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
Copy Markdown
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

bors commented Feb 16, 2024

Copy link
Copy Markdown
Collaborator

⌛ Trying commit 6563788 with merge aface90...

@bors

bors commented Feb 16, 2024

Copy link
Copy Markdown
Collaborator

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

@RalfJung

Copy link
Copy Markdown
Member Author

@craterbot check

@craterbot

Copy link
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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
Copy Markdown
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

oli-obk commented Feb 29, 2024

Copy link
Copy Markdown
Contributor

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
Copy Markdown
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

RalfJung commented Feb 29, 2024 via email

Copy link
Copy Markdown
Member Author

@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