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

Add Allocation to SMIR #114466

Merged
merged 1 commit into from
Aug 6, 2023
Merged

Add Allocation to SMIR #114466

merged 1 commit into from
Aug 6, 2023

Conversation

ouz-a
Copy link
Contributor

@ouz-a ouz-a commented Aug 4, 2023

As it's discussed here this is an attempt to cover Constants for smir in a different way compared to #114342

cc rust-lang/project-stable-mir#15

r? @oli-obk

@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 Aug 4, 2023
@rustbot
Copy link
Collaborator

rustbot commented Aug 4, 2023

This PR changes Stable MIR

cc @oli-obk, @celinval, @spastorino

Some changes occurred to the CTFE / Miri engine

cc @rust-lang/miri

Comment on lines 52 to 54
pub fn bytes(&self) -> &Option<Box<SortedMap<Size, Prov>>> {
&self.bytes
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

we can ignore this, this will always be empty except for miri

@@ -185,7 +193,7 @@ impl InitMask {
// Note: for performance reasons when interning, some of the fields can be partially
// hashed. (see the `Hash` impl below for more details), so the impl is not derived.
#[derive(Clone, Debug, Eq, PartialEq, HashStable)]
struct InitMaskMaterialized {
pub struct InitMaskMaterialized {
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep this private. Instead add an iterator over u64 that allows getting the blocks out directly (and just produces the same value over and over in case it was lazy). If you need to box those iterators, that's fine.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We can just store a Vec<u64> in SMIR, we don't need to use this optimization for now.

Copy link
Contributor Author

@ouz-a ouz-a Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep this private.

if I make it private I would have to remove Stable implementation for it. Can't implement trait for private type. (can I?)

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

you can't, but you can add a function returning the iterator I described to InitMask

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

keep this private. Instead add an iterator over u64 that allows getting the blocks out directly (and just produces the same value over and over in case it was lazy). If you need to box those iterators, that's fine.

but don't we lose information if it's InitMaskBlocks::Lazy{true} or InitMaskBlocks::Lazy(false), how would someone know if it's lazy or not, since I can't match private value. Maybe something like this ? So people who use smir could identify it it's by checking repeating 0's or 1's

vec![1;10].iter() // lazy ?
vec![0;10].iter() // not lazy ?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Lazy is not information anyone cares about. It is just an internal representation detail. it's a optimization to avoid allocating a vec if all entries are true or all entries are false

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Ah I get it now, I didn't know it wasn't useful for smir

mod init_mask;
mod provenance_map;
pub mod init_mask;
pub mod provenance_map;
Copy link
Member

@RalfJung RalfJung Aug 4, 2023

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This was all quite deliberately sealed away. Do we really have to make this public? :(

Not even the interpreter or codegen need that kind of access. I don't think anything can legitimately require it.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We're gonna make it private before this PR lands. We're not exposing the internals, but adding helper methods that avoid having to expose them.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

You don't need to add any helper methods. The existing methods are sufficient for the interpreter, codegen, and MiniRust.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yes, but these do random access, we want to duplicate the information. Iterating over all bytes is... not great.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

No they don't. They build a full model of the allocation contents in MiniRust / LLVM. I really can't see how you would need more APIs than what is required to build a fully accurate model of Rust semantics aka MiniRust.

Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Yea, I assumed that was too expensive for big allocations, but we can optimize when we see that being an issue

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Made them private, I think in future PR we should def get inspiration from MiniRust.


pub type Bytes = Vec<u8>;
pub type Size = usize;
pub type Prov = usize;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This should be AllocId. How is that exposed to smir?

Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Similar to how we have a table for DefIds, we should add something like it for AllocIds (instead of exposing the AllocIds internals). Since we don't actually need this in this PR yet, we can make Prov just be Opaque and stick a stringified version of the AllocId in it.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I made it Opaque

Comment on lines 252 to 258
#[derive(Clone, Debug)]
pub struct ProvenanceMap {
pub ptrs: Vec<(Size, Prov)>,
}
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This will not be very useful without doc comments explaining what this is all about.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I added comments from rustc, I don't know specifics of what smir user would want to know from this. I think it should suffice for now? If not I would love to learn more and add more information/documentation.

@@ -64,6 +64,13 @@ impl InitMask {
}
}

pub fn get_blocks(&self) -> Box<std::slice::Iter<'_, u64>> {
match &self.blocks {
InitMaskBlocks::Lazy { state: _ } => Box::new([].iter()),
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

this is wrong, it's not zero sized, but repeating state for the length of the entire bytes buffer.

But, as Ralf mentioned, we can avoid this entirely by encoding bytes as Vec<Option<u8>> and using https://github.com/RalfJung/minirust/blob/196da9640a4cd608405ad40110d29930827a99a5/tooling/minimize/src/constant.rs#L132-L145 to generate that Vec. Then we don't need an init_mask field at all.

provenance: {
let mut ptrs = Vec::new();
for (size, prov) in self.provenance().ptrs().iter() {
ptrs.push((size.bytes_usize(), opaque(&prov.0.get())));
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Suggested change
ptrs.push((size.bytes_usize(), opaque(&prov.0.get())));
ptrs.push((size.bytes_usize(), opaque(prov)));

when we render it opaquely, just render the full thing instead of its internal information

Comment on lines 1072 to 1075
let mut blocks = Vec::new();
for i in self.init_mask().get_blocks() {
blocks.push(*i);
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This and get_blocks is now dead code

Comment on lines 260 to 269
#[derive(Clone, Debug)]
pub enum InitMaskBlocks {
Materialized(InitMaskMaterialized),
}

#[derive(Clone, Debug)]
pub struct InitMask {
pub blocks: InitMaskBlocks,
pub len: Size,
}
Copy link
Contributor

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

THis isn't needed anymore either

@oli-obk
Copy link
Contributor

oli-obk commented Aug 6, 2023

Please squash the commits after all comments have been addressed

@ouz-a ouz-a force-pushed the smir_allocation branch 2 times, most recently from c2c3c54 to fc63e74 Compare August 6, 2023 11:11
use super::{
read_target_uint, write_target_uint, AllocId, InterpError, InterpResult, Pointer, Provenance,
ResourceExhaustionInfo, Scalar, ScalarSizeMismatch, UndefinedBehaviorInfo, UninitBytesAccess,
UnsupportedOpInfo,
};
use crate::ty;
use init_mask::*;
use provenance_map::*;
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Looks like some spurious remainders, these are now just unnecessary changes.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 6, 2023

@bors r+ rollup

@bors
Copy link
Contributor

bors commented Aug 6, 2023

📌 Commit b9a539e has been approved by oli-obk

It is now in the queue for this repository.

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 6, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 6, 2023
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 6, 2023
…iaskrgr

Rollup of 5 pull requests

Successful merges:

 - rust-lang#114466 (Add Allocation to SMIR)
 - rust-lang#114505 (Add documentation to has_deref)
 - rust-lang#114519 (use offset_of! to calculate dirent64 field offsets)
 - rust-lang#114537 (Migrate GUI colors test to original CSS color format)
 - rust-lang#114539 (linkchecker: Remove unneeded FIXME about intra-doc links)

Failed merges:

 - rust-lang#114485 (Add trait decls to SMIR)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 92c0421 into rust-lang:master Aug 6, 2023
11 checks passed
@rustbot rustbot added this to the 1.73.0 milestone Aug 6, 2023
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Aug 9, 2023
Convert Const to Allocation in smir

Continuation of previous pr rust-lang#114466

cc rust-lang/project-stable-mir#15

r? `@oli-obk`
lnicola pushed a commit to lnicola/rust-analyzer that referenced this pull request Apr 7, 2024
Convert Const to Allocation in smir

Continuation of previous pr rust-lang/rust#114466

cc rust-lang/project-stable-mir#15

r? `@oli-obk`
RalfJung pushed a commit to RalfJung/rust-analyzer that referenced this pull request Apr 27, 2024
Convert Const to Allocation in smir

Continuation of previous pr rust-lang/rust#114466

cc rust-lang/project-stable-mir#15

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-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

5 participants