Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

do not call black_box on Miri #75282

Merged
merged 1 commit into from
Aug 8, 2020
Merged

Conversation

RalfJung
Copy link
Member

@RalfJung RalfJung commented Aug 8, 2020

Helps with #75274 (but #74932 introduced unrelated breakage that will need a separate fix)
Cc @eggyal r? @Mark-Simulacrum

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 8, 2020
@eggyal
Copy link
Contributor

eggyal commented Aug 8, 2020

Since black_box is only a best effort, wouldn't it be better to omit the llvm_asm within it when compiling miri rather than omitting the call altogether? Else other attempts to call black_box (if there ever are any) would also have to be explicitly omitted?

@RalfJung RalfJung mentioned this pull request Aug 8, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Aug 8, 2020

I was considering that, but current black_box is in a weird state until #64102 has been implemented.

But I noticed the comment already says this is best-effort, so I think adding this cfg in there is indeed allowed. I'll do that, thanks.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 8, 2020

I agree, we should just #[cfg(miri)]

llvm_asm!("" : : "r"(&dummy));
and allow the use of black_box everywhere.

@oli-obk
Copy link
Contributor

oli-obk commented Aug 8, 2020

r? @oli-obk

r=me with CI passing or local tests giving high confidence in them passing

@RalfJung
Copy link
Member Author

RalfJung commented Aug 8, 2020

@bors r=oli-obk p=1
(higher priority to unblock a Miri fix)

@bors
Copy link
Contributor

bors commented Aug 8, 2020

📌 Commit 8385146 has been approved by oli-obk

@bors bors added S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. and removed S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. labels Aug 8, 2020
@RalfJung
Copy link
Member Author

RalfJung commented Aug 8, 2020

@bors rollup

@eggyal
Copy link
Contributor

eggyal commented Aug 8, 2020

This should close #75274, no?

@RalfJung
Copy link
Member Author

RalfJung commented Aug 8, 2020

No, see the OP of this PR.

rust-lang/miri#1500 will have to land in Miri (and we need a submodule update in rustc) to make Miri build again.

@bors
Copy link
Contributor

bors commented Aug 8, 2020

⌛ Testing commit 8385146 with merge c92fc8d...

@bors
Copy link
Contributor

bors commented Aug 8, 2020

☀️ Test successful - checks-actions, checks-azure
Approved by: oli-obk
Pushing c92fc8d to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 8, 2020
@bors bors merged commit c92fc8d into rust-lang:master Aug 8, 2020
bors added a commit to rust-lang/miri that referenced this pull request Aug 8, 2020
bors added a commit to rust-lang/miri that referenced this pull request Aug 8, 2020
@RalfJung RalfJung deleted the miri-black-box branch August 11, 2020 07:06
@cuviper cuviper added this to the 1.47.0 milestone May 2, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
merged-by-bors This PR was explicitly merged by bors. S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

7 participants