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

Implement black_box using intrinsic #87916

Merged
merged 1 commit into from
Aug 12, 2021
Merged

Conversation

nbdd0121
Copy link
Contributor

Introduce black_box intrinsic, as suggested in #87590 (comment).

This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity.

cc @Amanieu as this is related to inline assembly
cc @bjorn3 for rustc_codegen_cranelift changes
cc @RalfJung as this affects MIRI

r? @nagisa I suppose

@rust-highfive
Copy link
Collaborator

Some changes occured to rustc_codegen_cranelift

cc @bjorn3

Some changes occured to the CTFE / Miri engine

cc @rust-lang/miri

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Aug 10, 2021
@nagisa
Copy link
Member

nagisa commented Aug 10, 2021

Looks pretty nice. And in the future we have freedom to implement this in a better or otherwise backend specific manner better.

@nagisa
Copy link
Member

nagisa commented Aug 10, 2021

@bors delegate=nbdd0121

(feel free to r=nagisa once the unwrap is bug!ified)

@bors
Copy link
Contributor

bors commented Aug 10, 2021

✌️ @nbdd0121 can now approve this pull request

@nbdd0121
Copy link
Contributor Author

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Aug 10, 2021

📌 Commit 3cf2a69 has been approved by nagisa

@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 10, 2021
dummy
#[cfg(not(bootstrap))]
{
crate::intrinsics::black_box(dummy)
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Longer-term we might even want to directly reexport the intrinsic instead of using a wrapper function... but that's blocked on the fn ptr reification PR.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I guess that's also blocked by path component stability

Copy link
Member

@RalfJung RalfJung Aug 10, 2021

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, we already reexport transmute and probably will reexport copy/copy_nonoverlapping soon (we did but it was reverted due to the fn ptr reification problem).

So, while the lack of path component stability is a problem, it's not necessarily a blocker.

Anyway, even the exported function is currently still unstable; this is a discussion to be had at stabilization time.

@nbdd0121
Copy link
Contributor Author

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Aug 10, 2021

📌 Commit f98d540 has been approved by nagisa

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Aug 11, 2021
Implement `black_box` using intrinsic

Introduce `black_box` intrinsic, as suggested in rust-lang#87590 (comment).

This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity.

cc `@Amanieu` as this is related to inline assembly
cc `@bjorn3` for rustc_codegen_cranelift changes
cc `@RalfJung` as this affects MIRI

r? `@nagisa` I suppose
@JohnTitor
Copy link
Member

Possibly failed in rollup: #87939 (comment)
@bors r-

@bors bors added S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. and removed S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. labels Aug 11, 2021
@nbdd0121
Copy link
Contributor Author

failures:

---- [ui] ui/sanitize/memory.rs stdout ----

error: error pattern ' in the stack frame of function 'random'' not found!
command: "/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/memory/a"
stdout:
------------------------------------------


------------------------------------------
stderr:
------------------------------------------
==10866==WARNING: MemorySanitizer: use-of-uninitialized-value
    #0 0x5565d1c6fb44  (/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/memory/a+0x67b44)
    #1 0x7f93163a10b2  (/lib/x86_64-linux-gnu/libc.so.6+0x270b2)
    #2 0x5565d1c111cd  (/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/memory/a+0x91cd)

  Uninitialized value was created by an allocation of 'r.i' in the stack frame of function 'main'
    #0 0x5565d1c6fa40  (/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/memory/a+0x67a40)

SUMMARY: MemorySanitizer: use-of-uninitialized-value (/checkout/obj/build/x86_64-unknown-linux-gnu/test/ui/sanitize/memory/a+0x67b44) 

------------------------------------------

It seems to me that the codegen is better with the intrinsic approach and a memcpy is eliminated from random, so the place where uninit values end up getting used becomes main.

Previously it's like (expressed in MIR):

fn black_box(_1: T) -> T {
    let mut _0: T; // return place
    let mut _2: &mut T;

	bb0: {
	      _2 = &mut _1;
		  llvm_asm!(""::"r"(_2):"memory":"volatile");
	      _0 = move _1; // LLVM can't optimize out
		  return;
	}
}

The intrinsic approach it's codegenned more like:

fn black_box(_1: T) -> T {
    let mut _0: T; // return place
    let mut _2: &mut T;

	bb0: {
		  _0 = move _1; // LLVM can optimize out
	      _2 = &mut _0;
		  llvm_asm!(""::"r"(_2):"memory":"volatile");
		  return;
	}
}

I think it'd be sufficient just to change the pattern so that "main" is also matched?

@Amanieu Amanieu mentioned this pull request Aug 11, 2021
@nbdd0121
Copy link
Contributor Author

@nagisa A sanitize test needs to be fixed (last commit). PTAL

@nagisa
Copy link
Member

nagisa commented Aug 12, 2021

Please squash the commits. r=me once its done.

The new implementation allows some `memcpy`s to be optimized away,
so the uninit value in ui/sanitize/memory.rs is constructed directly
onto the return place. Therefore the sanitizer now says that the
value is allocated by `main` rather than `random`.
@nbdd0121
Copy link
Contributor Author

@bors r=nagisa

@bors
Copy link
Contributor

bors commented Aug 12, 2021

📌 Commit 1fb1643 has been approved by nagisa

@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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Aug 12, 2021
Manishearth added a commit to Manishearth/rust that referenced this pull request Aug 12, 2021
Implement `black_box` using intrinsic

Introduce `black_box` intrinsic, as suggested in rust-lang#87590 (comment).

This is still codegenned as empty inline assembly for LLVM. For MIR interpretation and cranelift it's treated as identity.

cc `@Amanieu` as this is related to inline assembly
cc `@bjorn3` for rustc_codegen_cranelift changes
cc `@RalfJung` as this affects MIRI

r? `@nagisa` I suppose
@bors
Copy link
Contributor

bors commented Aug 12, 2021

⌛ Testing commit 1fb1643 with merge 0fa3190...

@bors
Copy link
Contributor

bors commented Aug 12, 2021

☀️ Test successful - checks-actions
Approved by: nagisa
Pushing 0fa3190 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 12, 2021
bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 12, 2021
…arth

Rollup of 4 pull requests

Successful merges:

 - rust-lang#87916 (Implement `black_box` using intrinsic)
 - rust-lang#87922 (Add c_enum_min_bits target spec field, use for arm-none and thumb-none targets)
 - rust-lang#87953 (Improve formatting of closure capture migration suggestion for multi-line closures.)
 - rust-lang#87965 (Silence non_fmt_panic from external macros.)

Failed merges:

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit 0fa3190 into rust-lang:master Aug 12, 2021
@rustbot rustbot added this to the 1.56.0 milestone Aug 12, 2021
@nbdd0121 nbdd0121 deleted the black_box branch August 12, 2021 23:49
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.

8 participants