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

Wrap write_bytes in a function. Move docs #57997

Open
wants to merge 1 commit into
base: master
from

Conversation

Projects
None yet
@nitnelave
Copy link

nitnelave commented Jan 30, 2019

This will allow us to add debug assertions.
See issue #53871.

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Jan 30, 2019

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @KodrAus (or someone else) soon.

If any changes to this PR are deemed necessary, please add them as extra commits. This ensures that the reviewer can see what has changed since they last reviewed the code. Due to the way GitHub handles out-of-date commits, this should also make it reasonably obvious what issues have or haven't been addressed. Large or tricky changes may require several passes of review and changes.

Please see the contribution instructions for more information.

@nitnelave

This comment has been minimized.

Copy link
Author

nitnelave commented Jan 30, 2019

@rust-highfive rust-highfive assigned RalfJung and unassigned KodrAus Jan 30, 2019

///
/// // Now the box is fine
/// assert_eq!(*v, 42);
/// ```
#[stable(feature = "rust1", since = "1.0.0")]

This comment has been minimized.

@RalfJung

RalfJung Jan 30, 2019

Member

This should then also be removed, I think.

EDIT: Uh, never mind. This is public.

This comment has been minimized.

@RalfJung

RalfJung Jan 30, 2019

Member

Oh never mind. I did not know the functions in this module are pub. What the heck, why do we have public intrinsics?!?

This comment has been minimized.

@nitnelave

nitnelave Jan 30, 2019

Author

That's what I was surprised about when writing the wrapper :D

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 30, 2019

Cc @rust-lang/libs Please advise: we'd like to replace ptr::write_bytes (currently a reexport) by a function so that we can add some debug assertions in there (see #53871). However, I just realized that the following compiles on stable:

fn main() {
    let mut x = 2;
    unsafe {
        core::intrinsics::write_bytes(&mut x, 0, 1);
    }
}

That's quite bad, obviously, because it allows direct access to the intrinsic. I am seriously surprised that anything in intrinsics got stabilized.

So we could either move this one intrinsic to a submodule, meaning core::intrinsics::write_bytes would not actually be an intrinsic, or we could replace the reexport by a wrapper function, meaning core::intrinsics::write_bytes and core::ptr::write_bytes would not be the same operation. Which do you prefer?

@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 30, 2019

That core::intrinsics::write_bytes works on Stable is a bug: #15702. I think we have precedent declaring this kind of bug not covered by the stability promise.

In fact I’d be in favor of actively unstabilizing everything in core::intrinsics, and replacing reexports by stable #[inline] wrapper functions.

@nitnelave

This comment has been minimized.

Copy link
Author

nitnelave commented Jan 30, 2019

I added the #[inline] annotation, but I'm leaving the destabilization of core::intrinsics for another issue.

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 30, 2019

Wrapping raw intrinsics in new functions has historically caused a noticeable impact to compile times. I'd recommend holding off on landing a PR that wraps everything until we have a perf run at least to evaluate the impact.

@nitnelave

This comment has been minimized.

Copy link
Author

nitnelave commented Jan 30, 2019

@alexcrichton That's okay with me. How do I trigger a perf run? Or is it just going to come eventually from the CI?

@alexcrichton

This comment has been minimized.

Copy link
Member

alexcrichton commented Jan 30, 2019

Oh triggering a perf run currently is still pretty manual. @nitnelave do you want to wrap a few more functions or just this one? Afterwards it's just @bors: try, wait for that, then @rustc-perf ...

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 30, 2019

I added the #[inline] annotation, but I'm leaving the destabilization of core::intrinsics for another issue.

What do we gain from separating this?

Anyway this PR currently removes the doc comment from a public stable function, I don't think that's a good idea (don't we have a lint against undocumented functions?).

That core::intrinsics::write_bytes works on Stable is a bug: #15702.

Hm, I don't see the connection to that issue. write_bytes is actively marked as stable in the intrinsics module, so it is not surprising that it is stable, is it?

EDIT: Oh, I @alexcrichton wrote "The intrinsics module is unstable but the function is stable because it's reexported in std::ptr". I thought we could attach a stability attribute to the reexport, but that doesn't seem to work then.

In fact I’d be in favor of actively unstabilizing everything in core::intrinsics, and replacing reexports by stable #[inline] wrapper functions.

Ah, that's great! I think it is impossible for transmute, but I suggest we do that here at least for the pointer methods.

@nitnelave

This comment has been minimized.

Copy link
Author

nitnelave commented Jan 30, 2019

I guess we'll eventually also wrap copy and copy_nonoverlapping, so I might as well do it all in one go.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 30, 2019

Yes please do. That'll also make the perf impact bigger (if it exists) and hence easier to measure.

I'd even use #[inline(always)], and that has been necessary before, but it is worth trying without always first (I think @nnethercote prefers it that way).

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Jan 30, 2019

Thanks! Let's see what the timing says.

@bors try

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Jan 30, 2019

⌛️ Trying commit 1c8b67b with merge 4f8466b...

bors added a commit that referenced this pull request Jan 30, 2019

Auto merge of #57997 - nitnelave:master, r=<try>
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue #53871.
@SimonSapin

This comment has been minimized.

Copy link
Contributor

SimonSapin commented Jan 30, 2019

write_bytes is actively marked as stable in the intrinsics module, so it is not surprising that it is stable, is it?

Yes, but going "through" an unstable module in a path such as core::intrinsics::write_bytes should not be allowed on stable. In other words, the only difference on the stable channel between an unstable module and a non-existing module should be the contents of error messages.

@nitnelave

This comment has been minimized.

Copy link
Author

nitnelave commented Feb 4, 2019

@rustc-perf

@nitnelave

This comment has been minimized.

Copy link
Author

nitnelave commented Feb 4, 2019

I'm trying to run a benchmark, how do I trigger that? @RalfJung

@lzutao

This comment has been minimized.

Copy link
Contributor

lzutao commented Feb 4, 2019

@nitnelave Try @rust-timer build 4f8466b.
Edit:

  • But why do you need to do a benchmark? I don't think this PR has much performance impact.
  • You must have enough permission to run rust-timer.
@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Feb 4, 2019

Insufficient permissions to issue commands to rust-timer.

1 similar comment
@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Feb 4, 2019

Insufficient permissions to issue commands to rust-timer.

@nitnelave

This comment has been minimized.

Copy link
Author

nitnelave commented Feb 4, 2019

  • But why do you need to do a benchmark? I don't think this PR has much performance impact.

It shouldn't, but we're trying to assess that, see #57997 (comment)

  • You must have enough permission to run rust-timer.

Who does? Who can I ask?

@lzutao

This comment has been minimized.

Copy link
Contributor

lzutao commented Feb 4, 2019

It shouldn't, but we're trying to assess that, see #57997 (comment)

Ah, yeah!

Who does? Who can I ask?

If you're not in the whitelist and try to run the timer, the timer will complain.

I will try to cc @RalfJung .

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Feb 4, 2019

@rust-timer

This comment has been minimized.

Copy link

rust-timer commented Feb 4, 2019

Success: Queued 4f8466b with parent 43b4c4a, comparison URL.

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Feb 14, 2019

I am going to r+ this based on the previous statements in this PR, which all (together with CI) seem to indicate this is all right.

@bors r+

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 14, 2019

📌 Commit 26bd433 has been approved by RalfJung

yeastplume added a commit to mimblewimble/rust-secp256k1-zkp that referenced this pull request Feb 15, 2019

stop depending on intrinsics (#43)
The interface, though marked stable, is actually not.
See rust-lang/rust#57997.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2019

⌛️ Testing commit 26bd433 with merge de2bd43...

bors added a commit that referenced this pull request Feb 15, 2019

Auto merge of #57997 - nitnelave:master, r=RalfJung
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue #53871.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 15, 2019

Your PR failed on Travis (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.

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)

@RalfJung

This comment has been minimized.

Copy link
Member

RalfJung commented Feb 15, 2019

Looks like some spurious macOS thing?

$ git clone --depth=2 --branch=auto https://github.com/rust-lang/rust.git rust-lang/rust
Cloning into 'rust-lang/rust'...
fatal: unknown value for config 'protocol.version': 2
The command "eval git clone --depth=2 --branch=auto https://github.com/rust-lang/rust.git rust-lang/rust " failed. Retrying, 2 of 3.

Cc @rust-lang/infra

@bors retry

@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2019

⌛️ Testing commit 26bd433 with merge 72a4f13...

bors added a commit that referenced this pull request Feb 15, 2019

Auto merge of #57997 - nitnelave:master, r=RalfJung
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue #53871.
@bors

This comment has been minimized.

Copy link
Contributor

bors commented Feb 15, 2019

💔 Test failed - checks-travis

@rust-highfive

This comment has been minimized.

Copy link
Collaborator

rust-highfive commented Feb 15, 2019

Your PR failed on Travis (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.

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)

@pietroalbini

This comment has been minimized.

Copy link
Member

pietroalbini commented Feb 15, 2019

@bors retry -- apparently we can't clone the repo anymore on macOS

@pietroalbini

This comment was marked as resolved.

Copy link
Member

pietroalbini commented Feb 15, 2019

@bors retry -- apparently we can't clone the repo anymore on macOS

GuillaumeGomez added a commit to GuillaumeGomez/rust that referenced this pull request Feb 17, 2019

Rollup merge of rust-lang#57997 - nitnelave:master, r=RalfJung
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue rust-lang#53871.

nitnelave added a commit to nitnelave/rust-secp256k1 that referenced this pull request Feb 18, 2019

Stop depending on intrinsics
Intrinsics are not supposed to be stable.

See rust-lang/rust#57997

Centril added a commit to Centril/rust that referenced this pull request Feb 20, 2019

Rollup merge of rust-lang#57997 - nitnelave:master, r=RalfJung
Wrap write_bytes in a function. Move docs

This will allow us to add debug assertions.
See issue rust-lang#53871.

bors added a commit that referenced this pull request Feb 20, 2019

Auto merge of #58594 - Centril:rollup, r=Centril
Rollup of 12 pull requests

Successful merges:

 - #55632 (Deny the `overflowing_literals` lint for all editions)
 - #57656 (Deprecate the unstable Vec::resize_default)
 - #57997 (Wrap write_bytes in a function. Move docs)
 - #58059 (deprecate before_exec in favor of unsafe pre_exec)
 - #58122 (RangeInclusive internal iteration performance improvement.)
 - #58198 (Suggest removing parentheses surrounding lifetimes)
 - #58353 (Check the Self-type of inherent associated constants)
 - #58476 (Remove `LazyTokenStream`.)
 - #58511 (Const to op simplification)
 - #58555 (Add a note about 2018e if someone uses `try {` in 2015e)
 - #58588 (remove a bit of dead code)
 - #58589 (cleanup macro after 2018 transition)

Failed merges:

r? @ghost
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment