-
Notifications
You must be signed in to change notification settings - Fork 353
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
Rustup #364
Rustup #364
Conversation
that function has been removed
so now we're hitting some alignment issue in the initialization... I'll have to analyze this in detail |
miri/intrinsic.rs
Outdated
let src = self.into_ptr(args[0].value)?; | ||
let src_align = self.layout_of(args[0].ty)?.align; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
so now we're hitting some alignment issue in the initialization... I'll have to analyze this in detail
This takes the alignment of the pointer itself, instead of what it points to.
#364 (comment) is the root cause of the alignment issue, temporarily worked around it. I am getting this error now:
|
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I did a quick skim, though I didn't pay attention to which changes were yours or from the other PR.
miri/fn_call.rs
Outdated
let def_id = instance.def_id(); | ||
let item_path = self.tcx.absolute_item_path_str(def_id); | ||
if item_path.starts_with("std::") { | ||
println!("{}", item_path); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This stray debug println should be removed (or use the debug!
logging macro).
miri/fn_call.rs
Outdated
println!("{}", item_path); | ||
} | ||
match &*item_path { | ||
"std::sys::unix::thread::guard::init" | "std::sys::unix::thread::guard::current" => { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you add a more thorough explanation of what problem this solves?
miri/fn_call.rs
Outdated
match ret_ty.sty { | ||
ty::TyAdt(ref adt_def, _) => { | ||
assert!(adt_def.is_enum(), "Unexpected return type for {}", item_path); | ||
let none_variant_index = adt_def.variants.iter().enumerate().find(|&(_i, ref def)| { |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can use Iterator::position
to simplify this, as in:
let none_variant_index = adt_def.variants.iter().position(|def| def.name.as_str() == "None").expect("No None variant");
miri/intrinsic.rs
Outdated
Place::Ptr { .. } => { | ||
bug!("init intrinsic tried to write to fat or unaligned ptr target") | ||
} | ||
_ => bug!("TODO"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can you elaborate this TODO? What needs to be done here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't know, it comes from #361
The current failure looks to me like we are not initializing |
@oli-obk second last commit should fix it. I incorrectly returned true from mark_static_initialised |
…uninitialized static error
The problem is that the const evaluation from rustc ends up creating an immutable allocation. You need to evaluate the static without calling |
Don't check interpret_interner when accessing a static to fix miri mutable statics Mutable statics don't work in my PR to fix the standalone [miri](https://github.com/solson/miri), as init_static didn't get called when the interpret_interner already contained a entry for the static, which is always immutable. cc rust-lang/miri#364
Don't check interpret_interner when accessing a static to fix miri mutable statics Mutable statics don't work in my PR to fix the standalone [miri](https://github.com/solson/miri), as init_static didn't get called when the interpret_interner already contained a entry for the static, which is always immutable. cc rust-lang/miri#364
miri/lib.rs
Outdated
.interpret_interner | ||
.get_cached(cid.instance.def_id()) | ||
.expect("uncached static"); | ||
ecx.memory.copy(ptr, layout.align, to_ptr.into(), layout.align, layout.size.bytes(), true)?; |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
This won't work, because the memory might contain references into other statics. We also can't implement it as deep copies, because then we'd duplicate some other static's memory instead of referring to it.
Instead of calling ecx.const_eval
do the code you had above, just without the interpret_interner
accesses (instead to the machine caching as you do it here)
miri/lib.rs
Outdated
@@ -287,6 +287,7 @@ impl<'mir, 'tcx: 'mir> Machine<'mir, 'tcx> for Evaluator<'tcx> { | |||
.interpret_interner | |||
.get_cached(cid.instance.def_id()) | |||
.expect("uncached static"); | |||
// TODO: do a recursive copy |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
No, no recursive copy. if you have
static FOO: &'static Foo = &BAR;
static BAR: Foo = Foo { field: &FOO };
Then BAR would end up referring to a copy of FOO instead of referring to it directly.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
We can't evaluate the static in the same EvalContext as far as I know, so we have to copy it anyway. A cache could be used just like ecx.memory.data.mut_static
to prevent duplication.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
You can do it. Just push a stackframe for it as you had before 907e67f
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But the while ecx.step()? {}
will step out of the frame present at the beginning of the init_static
function.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
ah, you can solve that by recording the stack size before pushing the frame (self.frames().len()
) in a variable and looping until you're back at the same frame.
Do you know where miri is failing atm? The travis logs are unhelpful due to an error reporting bug I need to fix in rustc |
With rust-lang/rust#49540 applied run-pass gives:
Compile-fail tests all fail, because of changed error format. Edit: updated for last commit (only two tests failing, one invalid discriminant for enum with niche filling ( |
miri/lib.rs
Outdated
// ------------------------------------------------------------ | ||
// ||||||||||||| TODO: remove this horrible hack |||||||||||||| | ||
// ------------------------------------------------------------ | ||
unsafe { &mut *(frame as *const Frame as *mut Frame) }.storage_dead(local); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
TODO: remove this!
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Eh, this is an &
to &mut
cast, right? This is UB. You might as well deref a dangling pointer here.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I know, that is why I want to remove it.
miri/lib.rs
Outdated
let call_stackframe = ecx.stack().len(); | ||
while ecx.step()? && ecx.stack().len() >= call_stackframe { | ||
if ecx.stack().len() == call_stackframe { | ||
let frame = ecx.stack().last().unwrap(); |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
is frame_mut
still public? if so, you can use that, otherwise, make it public in rustc
miri/validation.rs
Outdated
@@ -558,6 +560,7 @@ impl<'a, 'tcx> EvalContextExt<'tcx> for EvalContext<'a, 'tcx, super::Evaluator<' | |||
TyAdt(adt, _) if adt.is_box() => true, | |||
TySlice(_) | TyAdt(_, _) | TyTuple(..) | TyClosure(..) | TyArray(..) | | |||
TyDynamic(..) | TyGenerator(..) | TyForeign(_) => false, | |||
TyGeneratorWitness(..) => bug!("I'm not sure what to return here"), |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
What should I return here?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I don't think this is reachable, so just throw an unreachable!()
invocation there for now
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Sounds reasonable.
A list of upstream TODO's:
|
@bjorn3 thank's for your awesome work! I have rebased and sqashed some commits into the |
Based on #361