-
Notifications
You must be signed in to change notification settings - Fork 13.6k
Labels
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.Relevant to the compiler team, which will review and decide on the PR/issue.
Description
Hello, this is your friendly neighborhood mergebot.
After merging PR #66547, I observed that the tool miri has failing tests.
A follow-up PR to the repository https://github.com/rust-lang/miri is needed to fix the fallout.
cc @leo60228, do you think you would have time to do the follow-up work?
If so, that would be great!
cc @dtolnay, the PR reviewer, and nominating for compiler team prioritization.
Metadata
Metadata
Labels
T-compilerRelevant to the compiler team, which will review and decide on the PR/issue.Relevant to the compiler team, which will review and decide on the PR/issue.
Type
Projects
Milestone
Relationships
Development
Select code repository
Activity
dtolnay commentedon Nov 29, 2019
This test is failing: https://github.com/rust-lang/miri/blob/3bf51f591f7ffdd45f51cd1d42a4002482af2bd5/tests/run-pass/args.rs
dtolnay commentedon Nov 29, 2019
rust-lang/miri#624 might give a hint what needs to be updated. I haven't gotten a chance to look closely yet.
RalfJung commentedon Nov 29, 2019
Yeah, we'll need to emulate that new static thing added by #66547. Unfortunately I don't entirely understand the static magic that goes on there... there's just a fn ptr stored in a particular section and somehow that is enough?!?
eddyb commentedon Nov 29, 2019
Yes, all platforms have sections that they treat as an array of
fn
pointers, filling the entire section, which makes it easy to append more entries, as linkers will happily concatenate sections with the same name.This is how C++ global constructors are implemented everywhere.
So I guess there's two levels to this:
targer_env
is"gnu"
, the ctors (or just the init flavor?) can also take 1, 2 or 3 arguments, not only 0 arguments (I believe this is also how C main technically works)RalfJung commentedon Nov 29, 2019
Okay. This is basically rust-lang/miri#450. I hoped we'd never have to do this...
For dtors, we already run TLS dtors, but those are entirely separate (we hook the pthread calls where dtors are set, and then run those functions later). There's no way those can be unified to one mechanism, is there?
Also, this begs the question in which order stuff is called before/after main.
eddyb commentedon Nov 29, 2019
C++ asks itself the same question and I personally haven't heard of there being an answer.
The safe thing to do is to make sure any order is safe (in Rust you have to initialize global state before any runtime initialization, so it's safer than C++ where before the constructor runs, the fields are uninitialized).
The implementations just go through the sections in order, so the unknown quantity is what order the linker concatenates all of these sections.
In miri you don't have the linker to think about, but you do need a mechanisms to access all statics from all crates.
eddyb commentedon Nov 29, 2019
Wait, why can't libstd initialize the args from the start lang item as well? Surely there's an inexpensive way to do so?
leo60228 commentedon Nov 29, 2019
@eddyb That's what I did originally, but @joshtriplett said not to.
leo60228 commentedon Nov 29, 2019
Reverting d448ab0 should fix this.
RalfJung commentedon Nov 29, 2019
We could either adjust the
cfg
's to no use the new method whencfg(miri)
is set, or we could implement those initializers. Seems like we'll want them eventually anyway...What would be the best way to do that?
11 remaining items
eddyb commentedon Nov 29, 2019
I don't understand, this would only be a few bytes with
#[inline(never)]
?To unbreak miri it's probably fine but I'm increasingly worried about
cfg(miri)
, as it allows divergence to exist in ways which are not controlled within miri itself.eddyb commentedon Nov 29, 2019
@RalfJung I was talking about a user of these facilities, not miri implementation.
To expand a bit (although it's kind of offtopic):
Rust forces you to mutate a
static
that started out with a constant value (e.g.Mutex::new(None)
), which means nothing else would observe an uninitialized value, just the initial constant value, if it happened to ran before the runtime initialization (which would e.g. replace theNone
with aSome
).This makes Rust +
.init_array
safer in general than C++ with global constructors.dtolnay commentedon Nov 30, 2019
Ack.
RalfJung commentedon Nov 30, 2019
Aand now we also need to wait for #66896 to land. That's the third independent Miri-breaking PR in 24h, impressive. ;)