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

Make more arithmetic functions unstably const #68809

Merged
merged 12 commits into from
Feb 6, 2020

Conversation

ecstatic-morse
Copy link
Contributor

@ecstatic-morse ecstatic-morse commented Feb 3, 2020

This is a smaller version of #66884 (thanks @9999years) that constifies many of the arithmetic functions on integer primitives from #53718 that were blocked on #49146.

This makes the following things unstably const.

  • feature = const_int_unchecked_arith

    • intrinsics::unchecked_add
    • intrinsics::unchecked_sub
    • intrinsics::unchecked_mul
    • intrinsics::unchecked_div
    • intrinsics::unchecked_rem
  • feature = const_checked_int_methods

    • checked_add
    • checked_sub
    • checked_mul
    • checked_div (Uses intrinsics::unchecked_div internally)
    • checked_rem (Uses intrinsics::unchecked_rem internally)
    • checked_neg
    • checked_shl
    • checked_shr
    • checked_abs
  • feature = const_saturating_int_methods

    • saturating_mul
    • saturating_neg (Uses intrinsics::unchecked_sub internally)
    • saturating_abs (Uses intrinsics::unchecked_sub internally)
  • feature = const_wrapping_int_methods

    • wrapping_div
    • wrapping_rem
  • feature = const_overflowing_int_methods

    • overflowing_div
    • overflowing_rem
  • feature = const_euclidean_int_methods

    • checked_div_euclid
    • checked_rem_euclid
    • wrapping_div_euclid
    • wrapping_rem_euclid
    • overflowing_div_euclid
    • overflowing_rem_euclid

Exponentiation and operations on the NonZero types are left to a later PR.

r? @oli-obk
cc @rust-lang/wg-const-eval @rust-lang/libs

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Feb 3, 2020
@ecstatic-morse ecstatic-morse changed the title Make more arithmetic function unstably const Make more arithmetic functions unstably const Feb 3, 2020
@RalfJung
Copy link
Member

RalfJung commented Feb 3, 2020

#67466 changed the way the compiler marked intrinsics as const, so this requires a few #![cfg(bootstrap)]s. It may be prudent to wait until the beta version is bumped to land this, or we could just land the first five commits for now, which don't require any conditional compilation.

A release just happened, so it can't be long until the bootstrap compiler gets bumped. Work is in progress at #68708.

@oli-obk
Copy link
Contributor

oli-obk commented Feb 4, 2020

The bootstrap compiler has been bumped

ecstatic-morse and others added 9 commits February 4, 2020 11:04
@rust-highfive

This comment has been minimized.

@rust-highfive

This comment has been minimized.

@ecstatic-morse
Copy link
Contributor Author

ecstatic-morse commented Feb 5, 2020

@oli-obk. This should be ready now, and with no conditional compilation. I updated the feature names to jibe with the preexisting saturating_int_methods gate and avoid conflict with the already stabilized const_int_overflow gate. I don't think there's a policy around this, but it seemed prudent.

This PR also implements five unchecked arithmetic intrinsics in MIRI and marks them as rustc_const_unstable. The error message suggest that this requires an RFC but was written in before rustc_const_unstable worked on intrinsics. Since these should be uncontroversial, perhaps an FCP on this PR would suffice.

@Centril
Copy link
Contributor

Centril commented Feb 5, 2020

Since these should be uncontroversial, perhaps an FCP on this PR would suffice.

Not sure what you want FCP for... landing the gates unstably? That doesn't need FCP, Oliver can just r+.

@rust-highfive
Copy link
Collaborator

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.
2020-02-05T04:37:21.0663012Z ========================== Starting Command Output ===========================
2020-02-05T04:37:21.0665773Z [command]/bin/bash --noprofile --norc /home/vsts/work/_temp/7e462e2e-b1bc-42d0-91bb-672149a06716.sh
2020-02-05T04:37:21.0665810Z 
2020-02-05T04:37:21.0670863Z ##[section]Finishing: Disable git automatic line ending conversion
2020-02-05T04:37:21.0676641Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/68809/merge to s
2020-02-05T04:37:21.0678514Z Task         : Get sources
2020-02-05T04:37:21.0678582Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-05T04:37:21.0678732Z Version      : 1.0.0
2020-02-05T04:37:21.0678771Z Author       : Microsoft
---
2020-02-05T04:37:21.8963367Z ##[command]git remote add origin https://github.com/rust-lang/rust
2020-02-05T04:37:21.9071221Z ##[command]git config gc.auto 0
2020-02-05T04:37:21.9106863Z ##[command]git config --get-all http.https://github.com/rust-lang/rust.extraheader
2020-02-05T04:37:21.9152781Z ##[command]git config --get-all http.proxy
2020-02-05T04:37:21.9283810Z ##[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/68809/merge:refs/remotes/pull/68809/merge
---
2020-02-05T04:40:38.0165042Z E: Unable to fetch some archives, maybe run apt-get update or try with --fix-missing?
2020-02-05T04:40:38.0208152Z 
2020-02-05T04:40:38.0322225Z ##[error]Bash exited with code '100'.
2020-02-05T04:40:38.0335205Z ##[section]Finishing: Install awscli
2020-02-05T04:40:38.0356356Z ##[section]Starting: Checkout rust-lang/rust@refs/pull/68809/merge to s
2020-02-05T04:40:38.0358076Z Task         : Get sources
2020-02-05T04:40:38.0358127Z Description  : Get sources from a repository. Supports Git, TfsVC, and SVN repositories.
2020-02-05T04:40:38.0358196Z Version      : 1.0.0
2020-02-05T04:40:38.0358241Z Author       : Microsoft
2020-02-05T04:40:38.0358241Z Author       : Microsoft
2020-02-05T04:40:38.0358291Z Help         : [More Information](https://go.microsoft.com/fwlink/?LinkId=798199)
2020-02-05T04:40:38.0358362Z ==============================================================================
2020-02-05T04:40:38.4381860Z Cleaning any cached credential from repository: rust-lang/rust (GitHub)
2020-02-05T04:40:38.4425004Z ##[section]Finishing: Checkout rust-lang/rust@refs/pull/68809/merge to s
2020-02-05T04:40:38.4555636Z Cleaning up task key
2020-02-05T04:40:38.4556585Z Start cleaning up orphan processes.
2020-02-05T04:40:38.4690695Z Terminate orphan process: pid (5090) (python)
2020-02-05T04:40:38.4882654Z ##[section]Finishing: Finalize Job

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
Copy link
Contributor

oli-obk commented Feb 5, 2020

@bors r+

@bors
Copy link
Contributor

bors commented Feb 5, 2020

📌 Commit 78f8ad3 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 Feb 5, 2020
Dylan-DPC-zz pushed a commit to Dylan-DPC-zz/rust that referenced this pull request Feb 5, 2020
… r=oli-obk

Make more arithmetic functions unstably const

This is a smaller version of rust-lang#66884 (thanks @9999years) that constifies many of the arithmetic functions on integer primitives from rust-lang#53718 that were blocked on rust-lang#49146.

This makes the following things unstably const.

- `feature = const_int_unchecked_arith`
  - `intrinsics::unchecked_add`
  - `intrinsics::unchecked_sub`
  - `intrinsics::unchecked_mul`
  - `intrinsics::unchecked_div`
  - `intrinsics::unchecked_rem`

- `feature = const_checked_int_methods`
  - `checked_add`
  - `checked_sub`
  - `checked_mul`
  - `checked_div` (Uses `intrinsics::unchecked_div` internally)
  - `checked_rem` (Uses `intrinsics::unchecked_rem` internally)
  - `checked_neg`
  - `checked_shl`
  - `checked_shr`
  - `checked_abs`

- `feature = const_saturating_int_methods`
  - `saturating_mul`
  - `saturating_neg`  (Uses `intrinsics::unchecked_sub` internally)
  - `saturating_abs` (Uses `intrinsics::unchecked_sub` internally)

- `feature = const_wrapping_int_methods`
  - `wrapping_div`
  - `wrapping_rem`

- `feature = const_overflowing_int_methods`
  - `overflowing_div`
  - `overflowing_rem`

- `feature = const_euclidean_int_methods`
  - `checked_div_euclid`
  - `checked_rem_euclid`
  - `wrapping_div_euclid`
  - `wrapping_rem_euclid`
  - `overflowing_div_euclid`
  - `overflowing_rem_euclid`

Exponentiation and operations on the `NonZero` types are left to a later PR.

r? @oli-obk
cc @rust-lang/wg-const-eval @rust-lang/libs
bors added a commit that referenced this pull request Feb 5, 2020
Rollup of 8 pull requests

Successful merges:

 - #68762 (Strip unnecessary subexpression)
 - #68790 (Improve `merge_from_succ`)
 - #68809 (Make more arithmetic functions unstably const)
 - #68832 (Clean up E0264, E0267 and E0268 explanations)
 - #68840 (On suggesting `#![recursion_limit = "X"]`, note current crate name)
 - #68846 (doc fix on doc attribute)
 - #68851 (Fix issue number of `capacity` method)
 - #68858 (Merge item id stable hashing functions)

Failed merges:

r? @ghost
bors added a commit that referenced this pull request Feb 6, 2020
Rollup of 8 pull requests

Successful merges:

 - #68762 (Strip unnecessary subexpression)
 - #68790 (Improve `merge_from_succ`)
 - #68809 (Make more arithmetic functions unstably const)
 - #68832 (Clean up E0264, E0267 and E0268 explanations)
 - #68840 (On suggesting `#![recursion_limit = "X"]`, note current crate name)
 - #68846 (doc fix on doc attribute)
 - #68851 (Fix issue number of `capacity` method)
 - #68858 (Merge item id stable hashing functions)

Failed merges:

r? @ghost
@bors bors merged commit 78f8ad3 into rust-lang:master Feb 6, 2020
if let sym::unchecked_shl | sym::unchecked_shr = intrinsic_name {
throw_ub_format!("Overflowing shift by {} in `{}`", r_val, intrinsic_name);
} else {
throw_ub_format!("Overflow executing `{}`", intrinsic_name);
Copy link
Member

Choose a reason for hiding this comment

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

Are there any tests that check that these errors work? Miri has them, but so far I think nothing tests this on the CTFE side.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

Added in #68937.

JohnTitor added a commit to JohnTitor/rust that referenced this pull request Feb 12, 2020
…-test, r=RalfJung

Test failure of unchecked arithmetic intrinsics in const eval

Test that the unchecked arithmetic intrinsics that were made unstably const in rust-lang#68809 emit an error during const-eval if given invalid input.

Addresses [this comment](rust-lang#68809 (comment)).

r? @RalfJung
@ecstatic-morse ecstatic-morse deleted the const-int-functions branch October 6, 2020 01:42
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
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.

6 participants