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

Refactor panic_unwind/seh.rs pointer use #123490

Merged
merged 1 commit into from
Apr 12, 2024

Conversation

niluxv
Copy link
Contributor

@niluxv niluxv commented Apr 5, 2024

  • x86 now conforms to strict-provenance
  • x86_64 now uses the expose API (instead of as casts)
  • changed ptr_t from a type alias to a repr(transparent) struct for some extra type-safety
  • replaced the ptr! macro by methods on ptr_t, as there is now no reason (as far as I can see) anymore to use a macro

On x86_64 pointers in SEH are represented by 32-bit offsets from __ImageBase, so we can't use a pointer type. It might be possible to leak the provenance into the FFI by using a MaybeUninit<u32> instead of a u32, but that is a bit more involved than using expose, and I'm not sure that would be worth it.

@rustbot
Copy link
Collaborator

rustbot commented Apr 5, 2024

r? @Amanieu

rustbot has assigned @Amanieu.
They will have a look at your PR within the next two weeks and either review your PR or reassign to another reviewer.

Use r? to explicitly pick a reviewer

@rustbot rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-libs Relevant to the library team, which will review and decide on the PR/issue. labels Apr 5, 2024
@niluxv
Copy link
Contributor Author

niluxv commented Apr 5, 2024

@rustbot label +A-strict-provenance

related issue: #95494

@rustbot rustbot added the A-strict-provenance Area: Strict provenance for raw pointers label Apr 5, 2024
@niluxv
Copy link
Contributor Author

niluxv commented Apr 5, 2024

The CI failure gives no information except This job failed. Spurious? EDIT: force-pushed and now CI is green

@rustbot label +O-windows +A-panic

@rustbot rustbot added A-panic Area: Panicking machinery O-windows Operating system: Windows labels Apr 5, 2024
@Amanieu
Copy link
Member

Amanieu commented Apr 10, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 10, 2024

📌 Commit 3d0ed4a has been approved by Amanieu

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 Apr 10, 2024
matthiaskrgr added a commit to matthiaskrgr/rust that referenced this pull request Apr 11, 2024
…manieu

Refactor `panic_unwind/seh.rs` pointer use

* `x86` now conforms to strict-provenance
* `x86_64` now uses the expose API (instead of `as` casts)
* changed `ptr_t` from a type alias to a `repr(transparent)` struct for some extra type-safety
* replaced the `ptr!` macro by methods on `ptr_t`, as there is now no reason (as far as I can see) anymore to use a macro

On `x86_64` pointers in SEH are represented by 32-bit offsets from `__ImageBase`, so we can't use a pointer type. It might be possible to leak the provenance into the FFI by using a `MaybeUninit<u32>` instead of a `u32`, but that is a bit more involved than using expose, and I'm not sure that would be worth it.
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2024
…iaskrgr

Rollup of 3 pull requests

Successful merges:

 - rust-lang#123490 (Refactor `panic_unwind/seh.rs` pointer use)
 - rust-lang#123704 (Tweak value suggestions in `borrowck` and `hir_analysis`)
 - rust-lang#123753 (compiletest: error when finding a trailing directive)

r? `@ghost`
`@rustbot` modify labels: rollup
bors added a commit to rust-lang-ci/rust that referenced this pull request Apr 11, 2024
…nieu

Refactor `panic_unwind/seh.rs` pointer use

* `x86` now conforms to strict-provenance
* `x86_64` now uses the expose API (instead of `as` casts)
* changed `ptr_t` from a type alias to a `repr(transparent)` struct for some extra type-safety
* replaced the `ptr!` macro by methods on `ptr_t`, as there is now no reason (as far as I can see) anymore to use a macro

On `x86_64` pointers in SEH are represented by 32-bit offsets from `__ImageBase`, so we can't use a pointer type. It might be possible to leak the provenance into the FFI by using a `MaybeUninit<u32>` instead of a `u32`, but that is a bit more involved than using expose, and I'm not sure that would be worth it.
@bors
Copy link
Contributor

bors commented Apr 11, 2024

⌛ Testing commit 3d0ed4a with merge be361ad...

@bors
Copy link
Contributor

bors commented Apr 11, 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 Apr 11, 2024
@rust-log-analyzer

This comment has been minimized.

@Amanieu
Copy link
Member

Amanieu commented Apr 11, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2024

📌 Commit 52b5868 has been approved by Amanieu

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 Apr 11, 2024
@niluxv
Copy link
Contributor Author

niluxv commented Apr 11, 2024

Ai, only x.py checked it, not build... Now fixed.

I can rebase/squash the commits into one before merge?

@Amanieu
Copy link
Member

Amanieu commented Apr 11, 2024

Sure, just rebase and I will approve it again.

@bors r-

@bors bors removed the S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. label Apr 11, 2024
@bors bors added the S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. label Apr 11, 2024
@niluxv
Copy link
Contributor Author

niluxv commented Apr 11, 2024

push hook took a while, but here it is.

@Amanieu
Copy link
Member

Amanieu commented Apr 11, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Apr 11, 2024

📌 Commit f63d5d1 has been approved by Amanieu

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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Apr 11, 2024
@bors
Copy link
Contributor

bors commented Apr 12, 2024

⌛ Testing commit f63d5d1 with merge 6bc9dcd...

@bors
Copy link
Contributor

bors commented Apr 12, 2024

☀️ Test successful - checks-actions
Approved by: Amanieu
Pushing 6bc9dcd to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Apr 12, 2024
@bors bors merged commit 6bc9dcd into rust-lang:master Apr 12, 2024
12 checks passed
@rustbot rustbot added this to the 1.79.0 milestone Apr 12, 2024
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (6bc9dcd): comparison URL.

Overall result: no relevant changes - no action needed

@rustbot label: -perf-regression

Instruction count

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

Max RSS (memory usage)

Results

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)
- - 0
Improvements ✅
(primary)
-4.7% [-4.7%, -4.7%] 1
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) -4.7% [-4.7%, -4.7%] 1

Cycles

Results

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)
3.6% [2.7%, 5.0%] 13
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
- - 0
All ❌✅ (primary) 3.6% [2.7%, 5.0%] 13

Binary size

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

Bootstrap: 676.134s -> 676.794s (0.10%)
Artifact size: 316.06 MiB -> 316.09 MiB (0.01%)

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
A-panic Area: Panicking machinery A-strict-provenance Area: Strict provenance for raw pointers merged-by-bors This PR was explicitly merged by bors. O-windows Operating system: Windows S-waiting-on-bors Status: Waiting on bors to run and complete tests. Bors will change the label on completion. 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.

6 participants