Skip to content

do not call black_box on Miri#75282

Merged
bors merged 1 commit into
rust-lang:masterfrom
RalfJung:miri-black-box
Aug 8, 2020
Merged

do not call black_box on Miri#75282
bors merged 1 commit into
rust-lang:masterfrom
RalfJung:miri-black-box

Conversation

@RalfJung

@RalfJung RalfJung commented Aug 8, 2020

Copy link
Copy Markdown
Member

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

eggyal commented Aug 8, 2020

Copy link
Copy Markdown
Contributor

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

RalfJung commented Aug 8, 2020

Copy link
Copy Markdown
Member Author

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

oli-obk commented Aug 8, 2020

Copy link
Copy Markdown
Contributor

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

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

Comment thread library/core/src/hint.rs Outdated
@oli-obk

oli-obk commented Aug 8, 2020

Copy link
Copy Markdown
Contributor

r? @oli-obk

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

@RalfJung

RalfJung commented Aug 8, 2020

Copy link
Copy Markdown
Member Author

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

@bors

bors commented Aug 8, 2020

Copy link
Copy Markdown
Collaborator

📌 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

RalfJung commented Aug 8, 2020

Copy link
Copy Markdown
Member Author

@bors rollup

@eggyal

eggyal commented Aug 8, 2020

Copy link
Copy Markdown
Contributor

This should close #75274, no?

@RalfJung

RalfJung commented Aug 8, 2020

Copy link
Copy Markdown
Member Author

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

bors commented Aug 8, 2020

Copy link
Copy Markdown
Collaborator

⌛ Testing commit 8385146 with merge c92fc8d...

@bors

bors commented Aug 8, 2020

Copy link
Copy Markdown
Collaborator

☀️ 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