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

make Condvar, Mutex, RwLock const constructors work with the unsupported impl #98457

Merged
merged 1 commit into from
Sep 25, 2022

Conversation

japaric
Copy link
Member

@japaric japaric commented Jun 24, 2022

applying this patch locally to the rust-src component fixes #98378

however, the solution seems wrong to me because PR #97791 didn't add any rustc_const_stable attribute to underlying implementations like std::sys::unix::futex, so I must be missing something about how const-stability is checked ... maybe the restricted_std feature (gate?) has an effect?

fixes #98378
fixes #98293 (probably)

@rustbot rustbot added the T-libs Relevant to the library team, which will review and decide on the PR/issue. label Jun 24, 2022
@rust-highfive
Copy link
Collaborator

Hey! It looks like you've submitted a new PR for the library teams!

If this PR contains changes to any rust-lang/rust public library APIs then please comment with @rustbot label +T-libs-api -T-libs to tag it appropriately. If this PR contains changes to any unstable APIs please edit the PR description to add a link to the relevant API Change Proposal or create one if you haven't already. If you're unsure where your change falls no worries, just leave it as is and the reviewer will take a look and make a decision to forward on if necessary.

Examples of T-libs-api changes:

  • Stabilizing library features
  • Introducing insta-stable changes such as new implementations of existing stable traits on existing stable types
  • Introducing new or changing existing unstable library APIs (excluding permanently unstable features / features without a tracking issue)
  • Changing public documentation in ways that create new stability guarantees
  • Changing observable runtime behavior of library APIs

@rust-highfive
Copy link
Collaborator

r? @joshtriplett

(rust-highfive has picked a reviewer for you, use r? to override)

@rust-highfive rust-highfive added the S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. label Jun 24, 2022
@Ayush1325
Copy link
Contributor

I had to add #[rustc_const_stable(feature = "const_locks", since = "1.63.0")] to the MovableMutex and MovableRwLock as well, but that fixes #98293 for UEFI as well.

@japaric
Copy link
Member Author

japaric commented Jun 27, 2022

I had to add #[rustc_const_stable(feature = "const_locks", since = "1.63.0")] to the MovableMutex and MovableRwLock as well, but that fixes #98293 for UEFI as well.

my bad. I meant to apply rustc_const_stable to the movable mutex/rwlock rather than the static ones. fixed that and checked that both thumbv7m and uefi work

@kjetilkjeka
Copy link
Contributor

This is also a problem on nvptx64-nvidia-cuda. I have confirmed that your patch fixes it there as well.

@tmandry
Copy link
Member

tmandry commented Jul 21, 2022

r? @m-ou-se

@kjetilkjeka
Copy link
Contributor

Now with llvm-15 being merged the bug this PR fixes is a blocker for using bugfixes from LLVM for the nvptx64-nvidia-cuda target as this target does not come with prebuilt corelib.

@kjetilkjeka
Copy link
Contributor

I realize this bug is one that must necessarily be prioritized lower than a lot of the other great work @m-ou-se is doing. I also realize that it is not at all my place to request attention to things just because I personally care about them. But I do think it is unfortunate that -Zbuild-std has been in many cases unusable for many months. So I will still do this small nudge in case this PR has simply been forgotten about.

Maybe there are other qualified persons that will have the bandwidth to look at this in the near future? What do you think @tmandry

@m-ou-se
Copy link
Member

m-ou-se commented Sep 23, 2022

Apologies for the delay, and thanks for the reminder. (My past few weeks (months?) have been very chaotic. I'll have more time for reviewing again soon.)

however, the solution seems wrong to me because PR #97791 didn't add any rustc_const_stable attribute to underlying implementations like std::sys::unix::futex, so I must be missing something about how const-stability is checked ... maybe the restricted_std feature (gate?) has an effect?

Yeah that seems odd. Not sure why this is the case, but figuring that out doesn't have to block this change.

@bors r+

@bors
Copy link
Contributor

bors commented Sep 23, 2022

📌 Commit 513eda0 has been approved by m-ou-se

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 Sep 23, 2022
@bors
Copy link
Contributor

bors commented Sep 25, 2022

⌛ Testing commit 513eda0 with merge e20fabb...

@bors
Copy link
Contributor

bors commented Sep 25, 2022

☀️ Test successful - checks-actions
Approved by: m-ou-se
Pushing e20fabb to master...

@bors bors added the merged-by-bors This PR was explicitly merged by bors. label Sep 25, 2022
@bors bors merged commit e20fabb into rust-lang:master Sep 25, 2022
@rustbot rustbot added this to the 1.66.0 milestone Sep 25, 2022
@rust-timer
Copy link
Collaborator

Finished benchmarking commit (e20fabb): comparison URL.

Overall result: ✅ improvements - 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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-1.3% [-2.0%, -0.5%] 2
All ❌✅ (primary) - - 0

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
- - 0
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-3.2% [-3.9%, -2.6%] 2
All ❌✅ (primary) - - 0

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.

mean1 range count2
Regressions ❌
(primary)
- - 0
Regressions ❌
(secondary)
4.8% [4.2%, 5.4%] 2
Improvements ✅
(primary)
- - 0
Improvements ✅
(secondary)
-4.9% [-4.9%, -4.9%] 1
All ❌✅ (primary) - - 0

Footnotes

  1. the arithmetic mean of the percent change 2 3

  2. number of relevant changes 2 3

@Manishearth Manishearth added beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. and removed beta-nominated Nominated for backporting to the compiler in the beta channel. stable-nominated Nominated for backporting to the compiler in the stable channel. labels Oct 4, 2022
@Manishearth
Copy link
Member

(Wanted to nominate it for backporting before I realized it would have no effect, see thread for more details)

Manishearth added a commit to Manishearth/rust that referenced this pull request Nov 22, 2022
mark sys_common::once::generic::Once::new const-stable

Attempt to address rust-lang#103191 by marking the impl const-stable.
Picked the declaration from the callsite:
https://github.com/rust-lang/rust/blob/21b246587c2687935bd6004ffa5dcc4f4dd6600d/library/std/src/sync/once.rs#L67

This is similar to rust-lang#98457.

With this in, `python3 x.py build library/std --target x86_64-unknown-none` succeeds.
thomcc pushed a commit to tcdi/postgrestd that referenced this pull request Feb 10, 2023
mark sys_common::once::generic::Once::new const-stable

Attempt to address rust-lang/rust#103191 by marking the impl const-stable.
Picked the declaration from the callsite:
https://github.com/rust-lang/rust/blob/21b246587c2687935bd6004ffa5dcc4f4dd6600d/library/std/src/sync/once.rs#L67

This is similar to rust-lang/rust#98457.

With this in, `python3 x.py build library/std --target x86_64-unknown-none` succeeds.
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. 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