Skip to content

Conversation

matthiaskrgr
Copy link
Member

Successful merges:

Failed merges:

r? @ghost
@rustbot modify labels: rollup

Create a similar rollup

WaffleLapkin and others added 30 commits July 4, 2024 17:56
The rules for casting `*mut X<dyn A>` -> `*mut Y<dyn B>` are as follows:
- If `B` has a principal
  - `A` must have exactly the same principal (including generics)
  - Auto traits of `B` must be a subset of autotraits in `A`

Note that `X<_>` and `Y<_>` can be identity, or arbitrary structs with last field being the dyn type.
The lifetime of the trait object itself (`dyn ... + 'a`) is not checked.

This prevents a few soundness issues with `#![feature(arbitrary_self_types)]` and trait upcasting.
Namely, these checks make sure that vtable is always valid for the pointee.
I think it's fine, but let's ask T-lang separately.
...because it's very sketchy and causes FCWs.
In this case it *is* actually sound, but still.

I should write a better fork of anymap...
…, r=compiler-errors,oli-obk,lnicola

Make casts of pointers to trait objects stricter

This is an attempt to `fix` rust-lang#120222 and rust-lang#120217.

This is done by adding restrictions on casting pointers to trait objects.

Before this PR the rules were as follows:

> When casting `*const X<dyn A>` -> `*const Y<dyn B>`, principal traits in `A` and `B` must refer to the same trait definition (or no trait).

With this PR the rules are changed to

> When casting `*const X<dyn Src>` -> `*const Y<dyn Dst>`
> - if `Dst` has a principal trait `DstP`,
>   - `Src` must have a principal trait `SrcP`
>   - `dyn SrcP` and `dyn DstP` must be the same type (modulo the trait object lifetime, `dyn T+'a` -> `dyn T+'b` is allowed)
>   - Auto traits in `Dst` must be a subset of auto traits in `Src`
>     - Not adhering to this is currently a FCW (warn-by-default + `FutureReleaseErrorReportInDeps`), instead of an error
> - if `Src` has a principal trait `Dst` must as well
>   - this restriction will be removed in a follow up PR

This ensures that
1. Principal trait's generic arguments match (no `*const dyn Tr<A>` -> `*const dyn Tr<B>` casts, which are a problem for [rust-lang#120222](rust-lang#120222))
2. Principal trait's lifetime arguments match (no `*const dyn Tr<'a>` -> `*const dyn Tr<'b>` casts, which are a problem for [rust-lang#120217](rust-lang#120217))
3. No auto traits can be _added_ (this is a problem for arbitrary self types, see [this comment](rust-lang#120248 (comment)))

Some notes:
 - We only care about the metadata/last field, so you can still cast `*const dyn T` to `*const WithHeader<dyn T>`, etc
- The lifetime of the trait object itself (`dyn A + 'lt`) is not checked, so you can still cast `*mut FnOnce() + '_` to `*mut FnOnce() + 'static`, etc
  - This feels fishy, but I couldn't come up with a reason it must be checked

The diagnostics are currently not great, to say the least, but as far as I can tell this correctly fixes the issues.

cc `@oli-obk` `@compiler-errors` `@lcnr`
Mark format! with must_use hint

Uses unstable feature rust-lang#94745

Part of rust-lang#126475

First contribution to rust, please let me know if the blessing of tests is correct
Thanks `@bjorn3` for the help
Verify that allocations output by GVN are sufficiently aligned.

Fixes rust-lang#127396

r? `@oli-obk`
…pratt

clarify `sys::unix::fd::FileDesc::drop` comment

closes rust-lang#66876

simply clarifies some resource-relevant things regarding the `close` syscall to reduce the amount of search needed in other parts of the web.
…r=clubby789

bootstrap: once_cell::sync::Lazy -> std::sync::LazyLock

Since rust-lang#121377 has landed on beta
@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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. rollup A PR which is a rollup labels Jul 8, 2024
@matthiaskrgr
Copy link
Member Author

@bors r+ rollup=never p=6

@bors
Copy link
Collaborator

bors commented Jul 8, 2024

📌 Commit a659f7a 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 Jul 8, 2024
@bors
Copy link
Collaborator

bors commented Jul 8, 2024

⌛ Testing commit a659f7a with merge a06e9c8...

@bors
Copy link
Collaborator

bors commented Jul 8, 2024

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

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Jul 8, 2024
@bors bors merged commit a06e9c8 into rust-lang:master Jul 8, 2024
@rustbot rustbot added this to the 1.81.0 milestone Jul 8, 2024
@rust-timer
Copy link
Collaborator

📌 Perf builds for each rolled up PR:

PR# Message Perf Build Sha
#120248 Make casts of pointers to trait objects stricter 68e716ee1724c56f434d8948d6f3c0c594532cec (link)
#127355 Mark format! with must_use hint 05413a8a1c8704df349768ab3948dc5a9294ca15 (link)
#127399 Verify that allocations output by GVN are sufficiently alig… 5b9176f9cd315b66640bb79633cca3fec0218c8c (link)
#127460 clarify sys::unix::fd::FileDesc::drop comment 60c09aa7557ece95dad86d03ca8f3c8a186a575a (link)
#127467 bootstrap: once_cell::sync::Lazy -> std::sync::LazyLock fdd928748e990ccd0d67b7fad4f548612248b91e (link)

previous master: cfd7cf5a0e

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

Overall result: ❌✅ regressions and improvements - 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.4% [0.2%, 0.4%] 7
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
-0.8% [-0.8%, -0.8%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-0.8%, 0.4%] 8

Max RSS (memory usage)

Results (primary 0.7%)

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)
5.1% [2.5%, 10.5%] 5
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-4.8% [-7.4%, -0.7%] 4
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.7% [-7.4%, 10.5%] 9

Cycles

Results (primary -1.0%, secondary 2.3%)

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)
- - 0
Regressions ❌
(secondary)
2.3% [1.9%, 3.1%] 6
Improvements ✅
(primary)
-1.0% [-1.0%, -1.0%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -1.0% [-1.0%, -1.0%] 1

Binary size

Results (primary -0.0%, secondary 0.1%)

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)
0.0% [0.0%, 0.1%] 27
Regressions ❌
(secondary)
0.1% [0.1%, 0.1%] 4
Improvements ✅
(primary)
-0.3% [-0.4%, -0.0%] 7
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.0% [-0.4%, 0.1%] 34

Bootstrap: 701.941s -> 700.067s (-0.27%)
Artifact size: 328.61 MiB -> 328.73 MiB (0.04%)

@rustbot rustbot added the perf-regression Performance regression. label Jul 8, 2024
@Kobzol
Copy link
Member

Kobzol commented Jul 10, 2024

@rust-timer build 05413a8

@rust-timer

This comment has been minimized.

@rust-timer
Copy link
Collaborator

Finished benchmarking commit (05413a8): comparison URL.

Overall result: ❌✅ regressions and improvements - ACTION NEEDED

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.3% [0.2%, 0.5%] 8
Regressions ❌
(secondary)
0.2% [0.2%, 0.2%] 1
Improvements ✅
(primary)
-0.7% [-0.7%, -0.7%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 0.2% [-0.7%, 0.5%] 9

Max RSS (memory usage)

Results (primary 1.1%)

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)
6.3% [2.9%, 11.5%] 4
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
-5.9% [-6.8%, -4.4%] 3
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 1.1% [-6.8%, 11.5%] 7

Cycles

Results (primary -0.9%, secondary 2.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)
- - 0
Regressions ❌
(secondary)
2.5% [2.2%, 2.8%] 3
Improvements ✅
(primary)
-0.9% [-0.9%, -0.9%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.9% [-0.9%, -0.9%] 1

Binary size

Results (primary -0.0%, secondary 0.1%)

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)
0.0% [0.0%, 0.1%] 27
Regressions ❌
(secondary)
0.1% [0.1%, 0.1%] 4
Improvements ✅
(primary)
-0.3% [-0.4%, -0.1%] 6
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -0.0% [-0.4%, 0.1%] 33

Bootstrap: 701.941s -> 700.738s (-0.17%)
Artifact size: 328.61 MiB -> 328.73 MiB (0.04%)

@Kobzol
Copy link
Member

Kobzol commented Jul 10, 2024

Interesting, I did not expect #127355 to have any perf. effect. Maybe the macro change?

@matthiaskrgr matthiaskrgr deleted the rollup-lvv018b branch September 1, 2024 17:35
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-bootstrap Relevant to the bootstrap subteam: Rust's build system (x.py and src/bootstrap) 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.
Projects
None yet
Development

Successfully merging this pull request may close these issues.

9 participants