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 4 pull requests #131111

Merged
merged 10 commits into from
Oct 1, 2024
Merged

Rollup of 4 pull requests #131111

merged 10 commits into from
Oct 1, 2024

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

RalfJung and others added 10 commits September 11, 2024 16:38
… r=Urgau

Replace -Z default-hidden-visibility with -Z default-visibility

Issue rust-lang#105518
…ottmcm

ptr::add/sub: do not claim equivalence with `offset(c as isize)`

In rust-lang#110837, the `offset` intrinsic got changed to also allow a `usize` offset parameter. The intention is that this will do an unsigned multiplication with the size, and we have UB if that overflows -- and we also have UB if the result is larger than `usize::MAX`, i.e., if a subsequent cast to `isize` would wrap. ~~The LLVM backend sets some attributes accordingly.~~

This updates the docs for `add`/`sub` to match that intent, in preparation for adjusting codegen to exploit this UB. We use this opportunity to clarify what the exact requirements are: we compute the offset using mathematical multiplication (so it's no problem to have an `isize * usize` multiplication, we just multiply integers), and the result must fit in an `isize`.
Cc `@rust-lang/opsem` `@nikic`

rust-lang#130239 updates Miri to detect this UB.

`sub` still has some cases of UB not reflected in the underlying intrinsic semantics (and Miri does not catch): when we subtract `usize::MAX`, then after casting to `isize` that's just `-1` so we end up adding one unit without noticing any UB, but actually the offset we gave does not fit in an `isize`. Miri will currently still not complain for such cases:
```rust
fn main() {
    let x = &[0i32; 2];
    let x = x.as_ptr();
    // This should be UB, we are subtracting way too much.
    unsafe { x.sub(usize::MAX).read() };
}
```
However, the LLVM IR we generate here also is UB-free. This is "just" library UB but not language UB.
Cc `@saethlin;` might be worth adding precondition checks against overflow on `offset`/`add`/`sub`?

Fixes rust-lang#130211
Update Unicode escapes in `/library/core/src/char/methods.rs`

`char::MAX` is inconsistent on how Unicode escapes should be formatted. This PR resolves that.
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. 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. rollup A PR which is a rollup labels Oct 1, 2024
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=4

@bors
Copy link
Contributor

bors commented Oct 1, 2024

📌 Commit bd5ee83 has been approved by matthiaskrgr

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

bors commented Oct 1, 2024

⌛ Testing commit bd5ee83 with merge 06bb836...

@bors
Copy link
Contributor

bors commented Oct 1, 2024

☀️ Test successful - checks-actions
Approved by: matthiaskrgr
Pushing 06bb836 to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Oct 1, 2024
@bors bors merged commit 06bb836 into rust-lang:master Oct 1, 2024
7 checks passed
@rustbot rustbot added this to the 1.83.0 milestone Oct 1, 2024
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#130005 Replace -Z default-hidden-visibility with -Z default-visibi… b6a6fdcb9b739b8ec00d7d37f46b79c27b5cd636 (link)
#130229 ptr::add/sub: do not claim equivalence with `offset(c as is… c1cd965f24b4e7a9ef6e13428ddd9cd05aa426ce (link)
#130773 Update Unicode escapes in `/library/core/src/char/methods.r… c050f8588d5d134f88bdcf79066cb7348bac4e04 (link)
#130933 rustdoc: lists items that contain multiple paragraphs are m… 9b2daa699cf8b6458868bcf41392282107254226 (link)

previous master: c817d5dc20

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 (06bb836): comparison URL.

Overall result: ❌ regressions - ACTION NEEDED

Next Steps: If you can justify the regressions found in this perf run, please indicate this with @rustbot label: +perf-regression-triaged along with sufficient written justification. If you cannot justify the regressions please open an issue or create a new PR that fixes the regressions, add a comment linking to the newly created issue or PR, and then add the perf-regression-triaged label to this PR.

@rustbot label: +perf-regression
cc @rust-lang/wg-compiler-performance

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.2% [0.2%, 0.2%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [0.2%, 0.2%] 4

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: 771.522s -> 770.255s (-0.16%)
Artifact size: 341.52 MiB -> 341.52 MiB (0.00%)

@rustbot rustbot added the perf-regression Performance regression. label Oct 1, 2024
@rylev
Copy link
Member

rylev commented Oct 9, 2024

It seems like #130005 would be the only PR that could possibly have introduced any perf variation here though it's not clear to me why.

@rust-timer build b6a6fdc

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (b6a6fdc): comparison URL.

Overall result: ❌ regressions - please read the text below

Instruction count

This is the most reliable metric that we have; it was used to determine the overall result at the top of this comment. However, even this metric can sometimes exhibit noise.

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

Max RSS (memory usage)

Results (primary 4.5%)

This is a less reliable metric that may be of interest but was not used to determine the overall result at the top of this comment.

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

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: 771.522s -> 769.405s (-0.27%)
Artifact size: 341.52 MiB -> 341.53 MiB (0.00%)

@lqd
Copy link
Member

lqd commented Oct 11, 2024

though it's not clear to me why

@rylev I believe it's because the PR introduced new unconditional query calls to compute the symbol visibility (even though it will still return the same default as before, until the default is switched to "protected" in the future). The big reachable_non_generics query in particular.

That is, we could gain the perf back, and delay this computation until that change is needed, most likely when lld is used on stable by default, so not right around the corner.

@rylev
Copy link
Member

rylev commented Oct 14, 2024

cc @davidlattimore @Urgau (author and reviewer of #130005) for visibility ^^

@Urgau
Copy link
Member

Urgau commented Oct 14, 2024

Yeah, we may want to re-add the fast path for !did.is_local():

// Things with export level C don't get instantiated in
// downstream crates.
if !id.is_local() {
return Visibility::Hidden;
}

@lqd
Copy link
Member

lqd commented Oct 14, 2024

The query will be more costly than the is_local() check.

@Urgau
Copy link
Member

Urgau commented Oct 14, 2024

Oh, yeah you're right, when the default visibility wasn't changed we just returned Default and did nothing else.

fn default_visibility(tcx: TyCtxt<'_>, id: DefId, is_generic: bool) -> Visibility {
if !tcx.sess.default_hidden_visibility() {
return Visibility::Default;
}

I completely overlook that.

bors added a commit to rust-lang-ci/rust that referenced this pull request Oct 14, 2024
Add fast-path when computing the default visibility

This PR adds (or more correctly re-adds the) fast-path when computing the default visibility, by taking advantage of the fact that the "interposable" requested visibility always return the "default" codegen visibility.

Should address the small regression observed in rust-lang#131111 (comment).

r? `@lqd`
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. perf-regression Performance regression. 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.