Reduce special treatment for zsts

[oli-obk]

addresses #67467 (comment)

cc @RalfJung

[rust-highfive]

r? @eddyb

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

[oli-obk]

r? @RalfJung

[bors]

☔ The latest upstream changes (presumably #67540) made this pull request unmergeable. Please resolve the merge conflicts.

[bors]

☔ The latest upstream changes (presumably #67596) made this pull request unmergeable. Please resolve the merge conflicts.

[RalfJung]

This is quite clever! I am conflicted. I don't like the weird condition in try_as_mplace very much, and I don't like how this spreads values obtained by MemPlace::dangling, which I don't feel should even existing in the spec so I wonder why we have a hard time avoiding it here. On the other hand, I do like the simplifications this brings.

The previous uses of dangling, I think, were never actually observable by the program. Basically, what we should never do is call dangling and then later call to_ref. Could you add that in a doc-comment, and at least superficially audit the users of try_as_mplace accordingly?

Do you think there is any chance to also kill the other use of MemPlace::dangling, and then remove the dangling function entirely and inline it into try_as_mplace?

[oli-obk]

There are also two uses of dangling in miri for uses of call_function where the invoked function is statically known to have unit return value.

[oli-obk]

Even if we removed dangling, you could achieve the same effect via

OpTy::from(ImmTy::from(Scalar::zst(), layout)).try_as_mplace()

So I'm not sure what we'd have gained.

[RalfJung]

There are also two uses of dangling in miri for uses of call_function where the invoked function is statically known to have unit return value.

Two? I remember one, for calling drop. But yeah, there's probably no good replacement for that.

This point still stands, though:

The previous uses of dangling, I think, were never actually observable by the program. Basically, what we should never do is call dangling and then later call to_ref. Could you add that in a doc-comment, and at least superficially audit the users of try_as_mplace accordingly?

[oli-obk]

Two? I remember one, for calling drop. But yeah, there's probably no good replacement for that.

Two in miri, and one in the miri engine (the one for drop)

[oli-obk]

Basically, what we should never do is call dangling and then later call to_ref.

That feels impossible to uphold and I don't see what we'd gain. Do you want to prevent (&() as *const () as usize) / 2 to fail because the value is a pointer but (&*&() as *const () as usize) / 2 to succeed? Because this is I think what would happen with the current PR with the appropriate feature gates.

[RalfJung]

We'd gain not baking these fake dangling pointers into the implemented semantics of the Abstract Machine in an observable way. Or rather, that's a property that we currently have, and I do not want to lose it.

[RalfJung]

(Dang, looks like GH stopped sending email notifications for force-pushes. So please let me know when this is ready for review again.)

[oli-obk]

We'd gain not baking these fake dangling points into the implemented semantics of the Abstract Machine in an observable way.

I can poison the MemPlace when it is created via dangling and make as_ref panic if the MemPlace is poisoned.

[rust-highfive]

The job x86_64-gnu-llvm-7 of your PR failed (pretty log, raw log). Through arcane magic we have determined that the following fragments from the build log may contain information about the problem.

Click to expand the log.

2019-12-26T22:55:28.0677629Z ##[command]git remote add origin https://github.com/rust-lang/rust
2019-12-26T22:55:28.0877646Z ##[command]git config gc.auto 0
2019-12-26T22:55:28.0958304Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2019-12-26T22:55:28.1003323Z ##[command]git config --get-all http.proxy
2019-12-26T22:55:28.1157041Z ##[command]git -c http.extraheader="AUTHORIZATION: basic ***" fetch --force --tags --prune --progress --no-recurse-submodules --depth=2 origin +refs/heads/*:refs/remotes/origin/* +refs/pull/67501/merge:refs/remotes/pull/67501/merge

I'm a bot! I can only do what humans tell me to, so if this was not helpful or you have suggestions for improvements, please ping or otherwise contact @TimNN. (Feature Requests)

[oli-obk]

I addressed all the review comments.

at least superficially audit the users of try_as_mplace accordingly?

I did the audit and found no problematic cases. All cases are either Operand uses, so we'll definitely read later or write to the memory, but want to have the address back for anything other than getting the backing memory.

[RalfJung]

Just a couple nits. r=me with those fixed.

[oli-obk]

@bors r=RalfJung

[bors]

📌 Commit c5c4fa8 has been approved by RalfJung

[RalfJung]

@oli-obk Miri broke in a rollup containing this PR, and this seems like the likely culprit. Could you prepare a fixup PR?

[oli-obk]

on it