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

Rollup of 7 pull requests #129750

Merged
merged 30 commits into from
Aug 29, 2024
Merged

Rollup of 7 pull requests #129750

merged 30 commits into from
Aug 29, 2024

Conversation

GuillaumeGomez
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

kornelski and others added 30 commits August 28, 2024 23:32
Allows disabling `fmt::Debug` derive and debug formatting.
* Choose test inputs more thoroughly and systematically.
* Check that `isqrt` and `checked_isqrt` have equivalent results for
  signed types, either equivalent numerically or equivalent as a panic
  and a `None`.
* Check that `isqrt` has numerically-equivalent results for unsigned
  types and their `NonZero` counterparts.
* Reuse `ilog10` benchmarks, plus benchmarks that use a uniform
  distribution.
* Use a lookup table for 8-bit integers and the Karatsuba square root
  algorithm for larger integers.
* Include optimization hints that give the compiler the exact numeric
  range of results.
I am surprised the diff is so small for this enormous crate.
…Urgau

debug-fmt-detail option

I'd like to propose a new option that makes `#[derive(Debug)]` generate no-op implementations that don't print anything, and makes `{:?}` in format strings a no-op.

There are a couple of motivations for this:

1. A more thorough stripping of debug symbols. Binaries stripped of debug symbols still retain some of them through `Debug` implementations. It's hard to avoid that without compiler's help, because debug formatting can be used in many places, including dependencies, and their loggers, asserts, panics, etc.
   * In my testing it gives about 2% binary size reduction on top of all other binary-minimizing best practices (including `panic_immediate_abort`). There are targets like Web WASM or embedded where users pay attention to binary sizes.
   * Users distributing closed-source binaries may not want to "leak" any symbol names as a matter of principle.
2. Adds ability to test whether code depends on specifics of the `Debug` format implementation in unwise ways (e.g. trying to get data unavailable via public interface, or using it as a serialization format). Because current Rust's debug implementation doesn't change, there's a risk of it becoming a fragile de-facto API that [won't be possible to change in the future](https://www.hyrumslaw.com/). An option that "breaks" it can act as a [grease](https://www.rfc-editor.org/rfc/rfc8701.html).

This implementation is a `-Z fmt-debug=opt` flag that takes:

* `full` — the default, current state.
* `none` — makes derived `Debug` and `{:?}` no-ops. Explicit `impl Debug for T` implementations are left unharmed, but `{:?}` format won't use them, so they may get dead-code eliminated if they aren't invoked directly.
* `shallow` — makes derived `Debug` print only the type's name, without recursing into fields. Fieldless enums print their variant names. `{:?}` works.

The `shallow` option is a compromise between minimizing the `Debug` code, and compatibility. There are popular proc-macro crates that use `Debug::fmt` as a way to convert enum values into their Rust source code.

There's a corresponding `cfg` flag: `#[cfg(fmt_debug = "none")]` that can be used in user code to react to this setting to minimize custom `Debug` implementations or remove unnecessary formatting helper functions.
Improved `checked_isqrt` and `isqrt` methods

### Improved tests of `isqrt` and `checked_isqrt` implementations

* Inputs chosen more thoroughly and systematically.
* Checks that `isqrt` and `checked_isqrt` have equivalent results for signed types, either equivalent numerically or equivalent as a panic and a `None`.
* Checks that `isqrt` has numerically-equivalent results for unsigned types and their `NonZero` counterparts.

### Added benchmarks for `isqrt` implementations

### Greatly sped up `checked_isqrt` and `isqrt` methods

* Uses a lookup table for 8-bit integers and then the Karatsuba square root algorithm for larger integers.
* Includes optimization hints that give the compiler the exact numeric range of results.

### Feature tracking issue

`isqrt` is an unstable feature tracked at rust-lang#116226.

<details><summary>Benchmarked improvements</summary>

### Command used to benchmark

    ./x bench library/core -- int_sqrt

### Before

    benchmarks:
        num::int_sqrt::i128::isqrt           439591.65/iter  +/- 6652.70
        num::int_sqrt::i16::isqrt              5302.97/iter   +/- 160.93
        num::int_sqrt::i32::isqrt             62999.11/iter  +/- 2022.05
        num::int_sqrt::i64::isqrt            125248.81/iter  +/- 1674.43
        num::int_sqrt::i8::isqrt                123.56/iter     +/- 1.87
        num::int_sqrt::isize::isqrt          125356.56/iter  +/- 1017.03
        num::int_sqrt::non_zero_u128::isqrt  437443.75/iter  +/- 3535.43
        num::int_sqrt::non_zero_u16::isqrt     8604.58/iter    +/- 94.76
        num::int_sqrt::non_zero_u32::isqrt    62933.33/iter   +/- 517.30
        num::int_sqrt::non_zero_u64::isqrt   125076.38/iter +/- 11340.61
        num::int_sqrt::non_zero_u8::isqrt       221.51/iter     +/- 1.58
        num::int_sqrt::non_zero_usize::isqrt 136005.21/iter  +/- 2020.35
        num::int_sqrt::u128::isqrt           439014.55/iter  +/- 3920.45
        num::int_sqrt::u16::isqrt              8575.08/iter   +/- 148.06
        num::int_sqrt::u32::isqrt             63008.89/iter   +/- 803.67
        num::int_sqrt::u64::isqrt            125088.09/iter   +/- 879.29
        num::int_sqrt::u8::isqrt                230.18/iter     +/- 2.04
        num::int_sqrt::usize::isqrt          125237.51/iter  +/- 4747.83
### After

    benchmarks:
        num::int_sqrt::i128::isqrt           105184.89/iter +/- 1171.38
        num::int_sqrt::i16::isqrt              1910.26/iter   +/- 78.50
        num::int_sqrt::i32::isqrt             34260.34/iter  +/- 960.84
        num::int_sqrt::i64::isqrt             45939.19/iter +/- 2525.65
        num::int_sqrt::i8::isqrt                 22.87/iter    +/- 0.45
        num::int_sqrt::isize::isqrt           45884.17/iter  +/- 595.49
        num::int_sqrt::non_zero_u128::isqrt  106344.27/iter  +/- 780.99
        num::int_sqrt::non_zero_u16::isqrt     2790.19/iter   +/- 53.43
        num::int_sqrt::non_zero_u32::isqrt    33613.99/iter  +/- 362.96
        num::int_sqrt::non_zero_u64::isqrt    46235.42/iter  +/- 429.69
        num::int_sqrt::non_zero_u8::isqrt        31.78/iter    +/- 0.75
        num::int_sqrt::non_zero_usize::isqrt  46208.75/iter  +/- 375.27
        num::int_sqrt::u128::isqrt           106385.94/iter +/- 1649.95
        num::int_sqrt::u16::isqrt              2747.69/iter   +/- 28.72
        num::int_sqrt::u32::isqrt             33627.09/iter  +/- 475.68
        num::int_sqrt::u64::isqrt             46182.29/iter  +/- 311.16
        num::int_sqrt::u8::isqrt                 33.10/iter    +/- 0.30
        num::int_sqrt::usize::isqrt           46165.00/iter  +/- 388.41

</details>

Tracking Issue for {u8,i8,...}::isqrt rust-lang#116226

try-job: test-various
Add `-Zlint-llvm-ir`

This flag is similar to `-Zverify-llvm-ir` and allows us to lint the generated IR.

r? compiler
… r=nnethercote

riscv64imac: allow shadow call stack sanitizer

cc `@Darksonn` for shadow call stack sanitizer support on RV64IMAC and RV64GC
…ouxu

Add `needs-unwind` compiletest directive to `libtest-thread-limit` and replace some `Path` with `path` in `run-make`

Part of rust-lang#121876 and the associated [Google Summer of Code project](https://blog.rust-lang.org/2024/05/01/gsoc-2024-selected-projects.html).

This PR does two things:

1. Add this to `libtest-thread-limit` ([Why?](rust-lang#128507 (comment)))
```
//@ needs-unwind
// Reason: this should be ignored in cg_clif (Cranelift) CI and anywhere
// else that uses panic=abort.
```

2. Use `path` instead of `Path` to simplify multiple run-make tests.
…rgau

Add `unreachable_pub`, round 3

A follow-up to rust-lang#129648.

r? `@Urgau`
… r=notriddle

Fix rustdoc clippy lints

Ran clippy on rustdoc and fixed the errors.

r? `@notriddle`
@rustbot rustbot added the rollup A PR which is a rollup label Aug 29, 2024
@GuillaumeGomez
Copy link
Member Author

@bors r+ p=7 rollup=never

@bors
Copy link
Contributor

bors commented Aug 29, 2024

📌 Commit 9c7ae1d has been approved by GuillaumeGomez

It is now in the queue for this repository.

@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 29, 2024
@bors
Copy link
Contributor

bors commented Aug 29, 2024

⌛ Testing commit 9c7ae1d with merge cff318c...

bors added a commit to rust-lang-ci/rust that referenced this pull request Aug 29, 2024
…llaumeGomez

Rollup of 7 pull requests

Successful merges:

 - rust-lang#123940 (debug-fmt-detail option)
 - rust-lang#128166 (Improved `checked_isqrt` and `isqrt` methods)
 - rust-lang#128970 (Add `-Zlint-llvm-ir`)
 - rust-lang#129316 (riscv64imac: allow shadow call stack sanitizer)
 - rust-lang#129690 (Add `needs-unwind` compiletest directive to `libtest-thread-limit` and replace some `Path` with `path` in `run-make`)
 - rust-lang#129732 (Add `unreachable_pub`, round 3)
 - rust-lang#129743 (Fix rustdoc clippy lints)

r? `@ghost`
`@rustbot` modify labels: rollup
@rust-log-analyzer
Copy link
Collaborator

The job dist-aarch64-apple failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
  DOC_ARTIFACT_NAME: doc-auto-cff318cf
  AWS_ACCESS_KEY_ID: AKIA46X5W6CZN24CBO55
  AWS_SECRET_ACCESS_KEY: ***
##[endgroup]
cp: /var/folders/m4/5dz5h26x329cqq4fx333f8gm0000gn/T/tmp.RZgFbR8uPF/rust-nightly-aarch64-apple-darwin.pkg: fcopyfile failed: No space left on device
cp: /var/folders/m4/5dz5h26x329cqq4fx333f8gm0000gn/T/tmp.RZgFbR8uPF/rust-nightly-aarch64-apple-darwin.tar.xz: fcopyfile failed: No space left on device
cp: /var/folders/m4/5dz5h26x329cqq4fx333f8gm0000gn/T/tmp.RZgFbR8uPF/rust-src-nightly.tar.xz: fcopyfile failed: No space left on device
cp: /var/folders/m4/5dz5h26x329cqq4fx333f8gm0000gn/T/tmp.RZgFbR8uPF/rust-std-nightly-aarch64-apple-darwin.tar.xz: fcopyfile failed: No space left on device
cp: /var/folders/m4/5dz5h26x329cqq4fx333f8gm0000gn/T/tmp.RZgFbR8uPF/rustc-codegen-cranelift-nightly-aarch64-apple-darwin.tar.xz: fcopyfile failed: No space left on device
cp: /var/folders/m4/5dz5h26x329cqq4fx333f8gm0000gn/T/tmp.RZgFbR8uPF/rustc-dev-nightly-aarch64-apple-darwin.tar.xz: fcopyfile failed: No space left on device
cp: /var/folders/m4/5dz5h26x329cqq4fx333f8gm0000gn/T/tmp.RZgFbR8uPF/rustc-nightly-aarch64-apple-darwin.tar.xz: fcopyfile failed: No space left on device
cp: /var/folders/m4/5dz5h26x329cqq4fx333f8gm0000gn/T/tmp.RZgFbR8uPF/rustfmt-nightly-aarch64-apple-darwin.tar.xz: fcopyfile failed: No space left on device
##[error]Process completed with exit code 1.
[command]/opt/homebrew/bin/git version
git version 2.46.0
Copying '/Users/runner/.gitconfig' to '/Users/runner/work/_temp/c08796ad-443f-419f-bfb0-d23701f16b59/.gitconfig'
Temporarily overriding HOME='/Users/runner/work/_temp/c08796ad-443f-419f-bfb0-d23701f16b59' before making global git config changes

@bors
Copy link
Contributor

bors commented Aug 29, 2024

💔 Test failed - checks-actions

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

@bors retry

@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 29, 2024
@bors
Copy link
Contributor

bors commented Aug 29, 2024

⌛ Testing commit 9c7ae1d with merge 0d63418...

@bors
Copy link
Contributor

bors commented Aug 29, 2024

☀️ Test successful - checks-actions
Approved by: GuillaumeGomez
Pushing 0d63418 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Aug 29, 2024
@bors bors merged commit 0d63418 into rust-lang:master Aug 29, 2024
7 checks passed
@rustbot rustbot added this to the 1.82.0 milestone Aug 29, 2024
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#123940 debug-fmt-detail option 56eadd58c333204d64388440a66ee65811ecb85b (link)
#128166 Improved checked_isqrt and isqrt methods a54ec3dd7ce82bb3eae1bd018c758717e315e8bf (link)
#128970 Add -Zlint-llvm-ir 4f9cb91ad5f75334de32059aef5e37df9558e4e1 (link)
#129316 riscv64imac: allow shadow call stack sanitizer dd2fdd54c68c1b1371f2bf003574ea9caa687f58 (link)
#129690 Add needs-unwind compiletest directive to `libtest-thread… 5b1bf9b8e8897ce80f515c64ed143e2755101c04 (link)
#129732 Add unreachable_pub, round 3 6c40c7f40045446b70290f8d3fb33ec80685057d (link)
#129743 Fix rustdoc clippy lints 1f9307aedd693e66ceccde7ccb9f9c870906ec64 (link)

previous master: 784d444733

In the case of a perf regression, run the following command for each PR you suspect might be the cause: @rust-timer build $SHA

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (0d63418): comparison URL.

Overall result: ❌ regressions - no action needed

@rustbot label: -perf-regression

Instruction count

This is a highly reliable metric that was used to determine the overall result at the top of this comment.

mean range count
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
6.0% [6.0%, 6.0%] 1
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) - - 0

Max RSS (memory usage)

This benchmark run did not return any relevant results for this metric.

Cycles

This benchmark run did not return any relevant results for this metric.

Binary size

This benchmark run did not return any relevant results for this metric.

Bootstrap: missing data
Artifact size: 338.73 MiB -> 338.74 MiB (0.00%)

@GuillaumeGomez GuillaumeGomez deleted the rollup-gphsb7y branch August 30, 2024 09:20
@Kobzol
Copy link
Contributor

Kobzol commented Sep 3, 2024

The single coercions regression was a bimodality, it reverted itself:
image

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-query-system Area: The rustc query system (https://rustc-dev-guide.rust-lang.org/query.html) A-run-make Area: port run-make Makefiles to rmake.rs A-translation Area: Translation infrastructure, and migrating existing diagnostics to SessionDiagnostic merged-by-bors This PR was explicitly merged by bors. PG-exploit-mitigations Project group: Exploit mitigations rollup A PR which is a rollup S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. T-libs Relevant to the library team, which will review and decide on the PR/issue. T-rustdoc Relevant to the rustdoc team, which will review and decide on the PR/issue. WG-trait-system-refactor The Rustc Trait System Refactor Initiative (-Znext-solver)
Projects
None yet
Development

Successfully merging this pull request may close these issues.