Add MemoryExtra to find_foreign_static

[pvdrz]

r? @RalfJung @oli-obk

Related miri PR: rust-lang/miri#928

[RalfJung]

Sounds reasonable!

@bors r+ rollup=never

(I don't want this to land before the next nightly because it'll break Miri, and we desparately need a new working Miri shipped by rustup.)

[bors]

📌 Commit 0bd6df9 has been approved by RalfJung

[bors]

💡 This pull request was already approved, no need to approve it again.

• There's another pull request that is currently being tested, blocking this pull request: update miri #63922

[bors]

📌 Commit 0bd6df9 has been approved by RalfJung

[pvdrz]

(I don't want this to land before the next nightly because it'll break Miri, and we desparately need a new working Miri shipped by rustup.)

No problem :)

[RalfJung]

All right, now it can't land before midnight anyway.

@bors rollup-

[pvdrz]

I discovered that I also need to change Machine::find_foreign_static return type to Alloc<M::Tag, M::AllocExtra> should I do it on a separate PR?

[oli-obk]

@bors r-

No, do it here

[pvdrz]

Jmm wait a second... quoting the docs: "This allocation will then be fed to tag_allocation to initialize the extra state"
Edit: I might have to change other stuff as well, if its not a huge change I'll do it here

[RalfJung]

I discovered that I also need to change Machine::find_foreign_static return type to Alloc<M::Tag, M::AllocExtra> should I do it on a separate PR?

Why that? Also what is Alloc, do you mean Allocation?

[pvdrz]

yes, it is Allocation instead of Alloc. I had to completely untag the Allocation for environ so it can be returned by find_foreign_static. However, stacked_borrows will catch that later when miri tries to read from the allocation.

I thought that if I were able to keep the allocation's Tag it would be possible to read from it without triggering the stacked borrows errors

[RalfJung]

Hm... not sure that that makes sense. find_foreign_static is only called the first time a foreign static is accessed, then it gets put into the alloc_map.

But indeed so far we didn't have foreign statics that can change, so it is possible that something will have to change here. I am not sure what the best design looks like.

[pvdrz]

I think I'm using MiriMemoryKind::Env for this and I should be using MiriMemoryKind::Static instead: https://github.com/rust-lang/miri/blob/master/src/stacked_borrows.rs#L474

Edit: Anyway I'll do any other changes in other PR because they aren't as simple as I thought. Feel free to merge this whenever you think is appropiate

[RalfJung]

No, that would just make your life even harder.

[RalfJung]

But also you are getting initially tagged with M::STATIC_KIND.map(MemoryKind::Machine) as kind anyway.

[RalfJung]

Initial plan: also pass the AllocId to find_foreign_static.

On the Miri side, have state in EnvVars that stores an environ: Option<AllocId> (or maybe Option<Pointer<Tag>>, obtained via tag_static_base_pointer). Whenever an env var changes, if that variable is set, you need to build a new env list and store the address in the allocation identified there. In find_foreign_static, (a) assert it is still None, (b) set it to point to the AllocId you got, and (c) return as initial value the initial env list.

I think that should work. It is also quite clumsy though. I'll need to sleep over it.

[RalfJung]

@oli-obk is it possible, given the link name of a foreign static, to compute its DefId?

What happens with something like

extern {
    static foo: *const libc::c_void;
}
mod bar {
    extern {
        static foo: *const libc::c_void;
    }
}

Will that be two separate DefId for the same static? My guess is "yes", which means our pointer equality would be wrong... but also we'll have to be careful about that kind of duplication in Miri.

[oli-obk]

Oh :/ yea that seems like we'd screw it up. I guess we can use the string name as a unique identifier.

[RalfJung]

The problem is that every DefId gives rise to an AllocId. For normal statics that already makes things subtle as there are two AllocId (the DefId one and the "evaluated" one); for extern statics that's basically nonsense.

OTOH, I think it is fine for now to ICE if the program uses two extern static with the same name.

[JohnCSimon]

Ping from triage -
@christianpoveda @RalfJung @oli-obk
This conversation seems to have stalled out for a week. Is this ready for review? Thanks

[pvdrz]

I think we can close this PR and come back to it when I have a clearer idea on how to fix it