fix building libstd with cfg(miri)

[RalfJung]

Follow-up to #60156 to fix the failure in rust-lang/miri#721.

I think that build scripts get the same feature flags, but someone please correct me if I am wrong.

Also I wonder why this didn't cause #60156 to fail...

[rust-highfive]

r? @joshtriplett

(rust_highfive has picked a reviewer for you, use r? to override)

[joshtriplett]

Can you explain a bit more (in the commit message, not just as a comment here) about why this would only be needed for miri, and not for non-miri?

[RalfJung]

With #60156, we use SecRandomCopyBytes on macOS with Miri. But we also still need to be able to build the libstd so Miri can use it, and that builds a dylib, and I think that's what's failing here.

I added a comment to the code saying so.

[RalfJung]

This now blocks #60533

[RalfJung]

Cc @rust-lang/libs does this seem reasonable?

[SimonSapin]

Should SecRandomCopyBytes also be used in non-miri std?

[RalfJung]

No: #59879

[alexcrichton]

This seems indicative of a deeper bug that would be better fixed than linking yet another framework in the build script. Why, when running with miri, is any artifact being linked? Presumably miri just needs full metadata, right?

[RalfJung]

@alexcrichton that would be #56443

[alexcrichton]

I'm not sure that really answers my question though because that issue is about producing just an rlib, but that still feels wrong because miri doesn't need anything beyond metadata?

[RalfJung]

How can we build a metadata-only libstd? We can't even figure out how to build an rlib-only libstd.

[alexcrichton]

I don't really even know how you're building libstd to begin with. If you're using cargo it seems like you could use cargo check instead of cargo build. If you're doing something else it seems like you could presumably switch some flags...

I'm basically just asking that some elbow grease is applied somewhere by someone so we don't have to keep applying these hacks. I do not have the time to apply the elbow grease myself, and I can try to scrounge together enough time to at least review things. I'm sorry though but I'm not available for a back-and-forth design discussion over how to design builds of libstd for miri, that's just so far off my radar.

[joshtriplett]

This is decidedly outside my area of expertise; handing off to someone more familiar with miri.

r? @Centril

[Centril]

Also outside of my expertise; r? @oli-obk

[RalfJung]

I don't really even know how you're building libstd to begin with. If you're using cargo it seems like you could use cargo check instead of cargo build. If you're doing something else it seems like you could presumably switch some flags...

Interesting idea, I'll put that on the list. However, at least the way things are right now, when Miri is built as a rustc tool it shares the libstd with the rest of the build, so we'd still have to also build the full thing.

If this PR here is considered too hacky, I can also just revert #60156. This means HashMap (with the default hasher) will not work on macOS but it will work everywhere else.

[oli-obk]

My opinion is that this is ok, since it is a temporary hack anyway. It will go away once we figure out how to (fake-)open /dev/random in miri.

Until then I think it would be better to support HashMap on mac os. If you fear the hack will persisist, I think it would be reasonable to set a "kill the hack or revert and break HashMap on mac os" deadline so we miri folks have some pressure. Something like 6 months seems ok to me.

[alexcrichton]

Having a deadline for the removal of this seems reasonable to me, so I would be ok with that. Has this change been tested with miri? It seems like the previous one wasn't maybe and if you're using Cargo with RUSTFLAGS this change won't work (since build scripts don't get RUSTFLAGS), so I just wanted to confirm.

[RalfJung]

I don' have a Mac, so I am afraid I cannot test this.

[alexcrichton]

Ok but surely we can at least agree that before merging hacks into libstd we should at least have anyone testing to show that they do indeed satisfy the intended use case?

[RalfJung]

I agree with the sentiment, but don't have any idea how to do so.

Miri is currently broken, so that can't get worse. ;) But I don't know if this could have an effect on the other tools, which I think are built by the same builder.

[oli-obk]

You could bump the miri submodule in this PR. That way the PR only merges if your change actually fixes miri (assuming we test miri on mac in CI)On 11 May 2019 00:01, Ralf Jung <[email protected]> wrote:I agree with the sentiment, but don't have any idea how to do so. Miri is currently broken, so that can't get worse. ;) But I don't know if this could have an effect on the other tools, which I think are built by the same builder. —You are receiving this because you were assigned.Reply to this email directly, view it on GitHub, or mute the thread.

[RalfJung]

Hm, that would help. However we don't build libstd the same way here on CI ans in standalone Miri.

[RalfJung]

Now this includes a Miri update.

But given that #60156 was able to land, I don't think this actually tests if linking works -- though I don't know why.

Maybe it is easier to just revert #60156...

[RalfJung]

I just realized I had entirely forgotten about rust-lang/cargo#4423. So we won't see the cfg(miri) in the build script anyway -- the entire approach does not work.