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

Fix mut static task queue in SGX target #125800

Merged

Conversation

raoulstrackx
Copy link
Contributor

PR 125046 prevents mutable references to statics with #[linkage]. Such a construct was used with the tests for the x86_64-fortanix-unknown-sgx target. This PR fixes this and cleans up code a bit in 5 steps. Each step passes CI:

  • The mut static is removed, and Task explicitly implements Send
  • Renaming of the task_queue::lock function
  • Pass function for Thread as Send to Thread::imp and update when Packet<'scope, T> implements Sync
  • Storing Task::p as a type that implements Send
  • Letting the compiler auto implement Send for Task

cc: @jethrogb

@rustbot
Copy link
Collaborator

rustbot commented May 31, 2024

r? @cuviper

rustbot has assigned @cuviper.
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 O-SGX Target: SGX 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 May 31, 2024
@rust-log-analyzer

This comment has been minimized.

@raoulstrackx raoulstrackx force-pushed the raoul/rte-99-fix_mut_static_task_queue branch from 1ae82d7 to 70f8310 Compare May 31, 2024 08:09
@bjorn3
Copy link
Member

bjorn3 commented Jun 2, 2024

I think in #125046 I should have only denied #[linkage] static mut for extern statics. Basically moving the code I added in compiler/rustc_codegen_ssa/src/codegen_attrs.rs into the if tcx.is_foreign_item(did) arm.

@raoulstrackx
Copy link
Contributor Author

I think in #125046 I should have only denied #[linkage] static mut for extern statics. Basically moving the code I added in compiler/rustc_codegen_ssa/src/codegen_attrs.rs into the if tcx.is_foreign_item(did) arm.

Right, but even then I don't see why Task::p wouldn't need to implement Send as it's still passed from one thread (added to the list in Thread::new) to another (removed from the list in Thread::entry).

Anyway, I believe this PR still cleans up the code.

@workingjubilee
Copy link
Member

Yeah, seems preferable this way.
@bors r+

@bors
Copy link
Contributor

bors commented Jun 3, 2024

📌 Commit 70f8310 has been approved by workingjubilee

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 Jun 3, 2024
@raoulstrackx raoulstrackx force-pushed the raoul/rte-99-fix_mut_static_task_queue branch from 70f8310 to 8db363c Compare June 4, 2024 06:46
@workingjubilee
Copy link
Member

@bors r-

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

Waiting for @jethrogb's approval.

@workingjubilee
Copy link
Member

hm in fact
@bors delegate=jethrogb

@bors
Copy link
Contributor

bors commented Jun 4, 2024

✌️ @jethrogb, you can now approve this pull request!

If @workingjubilee told you to "r=me" after making some further change, please make that change, then do @bors r=@workingjubilee

@jethrogb
Copy link
Contributor

jethrogb commented Jun 4, 2024

@bors r+

@bors
Copy link
Contributor

bors commented Jun 4, 2024

📌 Commit 8db363c has been approved by jethrogb

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 Jun 4, 2024
fmease added a commit to fmease/rust that referenced this pull request Jun 4, 2024
…ic_task_queue, r=jethrogb

Fix `mut` static task queue in SGX target

[PR 125046](rust-lang#125046) prevents mutable references to statics with `#[linkage]`. Such a construct was used with the tests for the `x86_64-fortanix-unknown-sgx` target. This PR fixes this and cleans up code a bit in 5 steps. Each step passes CI:

- The `mut` static is removed, and `Task` explicitly implements `Send`
- Renaming of the `task_queue::lock` function
- Pass function for `Thread` as `Send` to `Thread::imp` and update when `Packet<'scope, T>` implements `Sync`
- Storing `Task::p` as a type that implements `Send`
- Letting the compiler auto implement `Send` for `Task`

cc: `@jethrogb`
fmease added a commit to fmease/rust that referenced this pull request Jun 4, 2024
…f, r=Urgau

Allow static mut definitions with #[linkage]

Unlike static declarations with #[linkage], for definitions rustc doesn't rewrite it to add an extra indirection.

This was accidentally disallowed in rust-lang#125046.

cc rust-lang#125800 (comment)
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Rollup of 7 pull requests

Successful merges:

 - rust-lang#125273 (bootstrap: implement new feature `bootstrap-self-test`)
 - rust-lang#125800 (Fix `mut` static task queue in SGX target)
 - rust-lang#125903 (rustc_span: Inline some hot functions)
 - rust-lang#125920 (Allow static mut definitions with #[linkage])
 - rust-lang#125921 (coverage: Carve out hole spans in a separate early pass)
 - rust-lang#125995 (Use inline const blocks to create arrays of `MaybeUninit`.)
 - rust-lang#125996 (Closures are recursively reachable)

r? `@ghost`
`@rustbot` modify labels: rollup
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Jun 5, 2024
…f, r=Urgau

Allow static mut definitions with #[linkage]

Unlike static declarations with #[linkage], for definitions rustc doesn't rewrite it to add an extra indirection.

This was accidentally disallowed in rust-lang#125046.

cc rust-lang#125800 (comment)
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Rollup merge of rust-lang#125920 - bjorn3:allow_static_mut_linkage_def, r=Urgau

Allow static mut definitions with #[linkage]

Unlike static declarations with #[linkage], for definitions rustc doesn't rewrite it to add an extra indirection.

This was accidentally disallowed in rust-lang#125046.

cc rust-lang#125800 (comment)
bors added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
…iaskrgr

Rollup of 9 pull requests

Successful merges:

 - rust-lang#124840 (resolve: mark it undetermined if single import is not has any bindings)
 - rust-lang#125622 (Winnow private method candidates instead of assuming any candidate of the right name will apply)
 - rust-lang#125648 (Remove unused(?) `~/rustsrc` folder from docker script)
 - rust-lang#125672 (Add more ABI test cases to miri (RFC 3391))
 - rust-lang#125800 (Fix `mut` static task queue in SGX target)
 - rust-lang#125871 (Orphanck[old solver]: Consider opaque types to never cover type parameters)
 - rust-lang#125893 (Handle all GVN binops in a single place.)
 - rust-lang#126008 (Port `tests/run-make-fulldeps/issue-19371` to ui-fulldeps)
 - rust-lang#126032 (Update description of the `IsTerminal` example)

r? `@ghost`
`@rustbot` modify labels: rollup
@bors bors merged commit fcc0b64 into rust-lang:master Jun 5, 2024
6 checks passed
@rustbot rustbot added this to the 1.80.0 milestone Jun 5, 2024
rust-timer added a commit to rust-lang-ci/rust that referenced this pull request Jun 5, 2024
Rollup merge of rust-lang#125800 - fortanix:raoul/rte-99-fix_mut_static_task_queue, r=jethrogb

Fix `mut` static task queue in SGX target

[PR 125046](rust-lang#125046) prevents mutable references to statics with `#[linkage]`. Such a construct was used with the tests for the `x86_64-fortanix-unknown-sgx` target. This PR fixes this and cleans up code a bit in 5 steps. Each step passes CI:

- The `mut` static is removed, and `Task` explicitly implements `Send`
- Renaming of the `task_queue::lock` function
- Pass function for `Thread` as `Send` to `Thread::imp` and update when `Packet<'scope, T>` implements `Sync`
- Storing `Task::p` as a type that implements `Send`
- Letting the compiler auto implement `Send` for `Task`

cc: ``@jethrogb``
@fmease
Copy link
Member

fmease commented Jun 6, 2024

bors sleepy @bors r-

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

jethrogb commented Jun 6, 2024

@fmease I'm confused about what you're trying to do. This PR is merged into master.

@fmease
Copy link
Member

fmease commented Jun 6, 2024

Just maintenance. Sometimes @bors's queue gets out of sync with GitHub (e.g., containing already merged PRs). While I could press the "synchronize" button, it's a bit destructive and buggy (try-builds get upgraded to builds, failing PRs suddenly show up in the queue, etc.). Therefore it's typical for someone to swoop in and manually "sync" the queue by r-'ing already merged PRs which still show up in the queue.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-SGX Target: SGX S-waiting-on-author Status: This is awaiting some action (such as code changes or more information) from the author. 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