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 elem_align = elem_layout.align.abi(); | ||
| let elem_align = elem_layout.align; | ||
| 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.
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: |
solson
left a comment
There was a problem hiding this comment.
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.
This stray debug println should be removed (or use the debug! logging macro).
| 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.
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.
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.
Can you elaborate this TODO? What needs to be done here?
|
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.
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
| .interpret_interner | ||
| .get_cached(cid.instance.def_id()) | ||
| .expect("uncached static"); | ||
| // TODO: do a recursive copy |
There was a problem hiding this comment.
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.
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.
You can do it. Just push a stackframe for it as you had before 907e67f
There was a problem hiding this comment.
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.
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: DetailsCompile-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.
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.
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.
is frame_mut still public? if so, you can use that, otherwise, make it public in rustc
miri/validation.rs
Outdated
| 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.
What should I return here?
There was a problem hiding this comment.
I don't think this is reachable, so just throw an unreachable!() invocation there for now
|
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