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

Create Atomic<T> type alias #130543

Open
wants to merge 4 commits into
base: master
Choose a base branch
from
Open

Conversation

CAD97
Copy link
Contributor

@CAD97 CAD97 commented Sep 19, 2024

and use it in core/alloc/std where possible, ignoring test files for now.

This is step one, creating the alias from Atomic<T> to AtomicT. The next step of flipping this and aliasing AtomicT to Atomic<T> will have a bigger impact, since AtomicT imports can be dropped once Atomic::new is a usable name.

First commit is the true change. Second commit is mostly mechanical replacement of AtomicT type names with Atomic<T>.

See how this was done for NonZero for the rough blueprint I'm following.

@rustbot
Copy link
Collaborator

rustbot commented Sep 19, 2024

r? @Noratrieb

rustbot has assigned @Noratrieb.
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-hermit Operating System: Hermit O-itron Operating System: ITRON O-SGX Target: SGX O-unix Operating system: Unix-like O-wasm Target: WASM (WebAssembly), http://webassembly.org/ O-windows Operating system: Windows 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 Sep 19, 2024
@CAD97

This comment was marked as outdated.

macro impl_atomic_primitive(
$Atom:ident $(<$T:ident>)? ($Primitive:ty),
size($size:literal),
align($align:literal) $(,)?
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

FYI the align is given to impl_atomic_primitive! because while it isn't actually used in this form, it's needed for the form where Atomic<T> is the struct definition. So it'll make the next diff a little cleaner (and here is kinda just residual from paring this back to just do an alias initially).

@CAD97
Copy link
Contributor Author

CAD97 commented Sep 19, 2024

Alright that's the merge conflicts fixed. If I end up needing to continue playing catch-up on some of the touched files, I'll probably just drop files' touch-up to use Atomic<T> from this PR instead of continuously rebasing, as they create conflicts.

@rust-log-analyzer

This comment has been minimized.

@Noratrieb
Copy link
Member

r=me after fixing the import issues

@CAD97 CAD97 force-pushed the generic-atomic branch 2 times, most recently from 67a79e8 to fbf092e Compare September 19, 2024 05:37
@CAD97
Copy link
Contributor Author

CAD97 commented Sep 19, 2024

Also found a similar conflict with the windows futex impl plumbing via local check, so that's also resolved by the amend (hopefully; my local checkout suddenly is complaining about duplicate rmeta for crate object for some reason).

IIRC I don't have perms to do this but might as well try:

@bors r=Noratrieb rollup=iffy

@bors
Copy link
Contributor

bors commented Sep 19, 2024

@CAD97: 🔑 Insufficient privileges: Not in reviewers

@bors
Copy link
Contributor

bors commented Sep 19, 2024

@CAD97: 🔑 Insufficient privileges: not in try users

@workingjubilee
Copy link
Member

@bors r=Noratrieb rollup=iffy

@bors
Copy link
Contributor

bors commented Sep 19, 2024

📌 Commit fbf092e has been approved by Noratrieb

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 19, 2024
workingjubilee added a commit to workingjubilee/rustc that referenced this pull request Sep 19, 2024
Create `Atomic<T>` type alias

and use it in core/alloc/std where possible, ignoring test files for now.

This is step one, creating the alias from `Atomic<T>` to `AtomicT`. The next step of flipping this and aliasing `AtomicT` to `Atomic<T>` will have a bigger impact, since `AtomicT` imports can be dropped once `Atomic::new` is a usable name.

First commit is the true change. Second commit is mostly mechanical replacement of `AtomicT` type names with `Atomic<T>`.

See [how this was done for `NonZero`](rust-lang#120257) for the rough blueprint I'm following.

- ACP: rust-lang/libs-team#443 (comment)
- Tracking issue: rust-lang#130539
@rust-log-analyzer

This comment has been minimized.

@workingjubilee
Copy link
Member

whups
@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 Sep 19, 2024
@CAD97
Copy link
Contributor Author

CAD97 commented Sep 19, 2024

I

  • don't really see how this could cause UI changes, especially to the test that was reported; and
  • locally have weird link errors happening (the most recent ones I think in LLVM) even for a clean; build on git master;

so it might take me a while to diagnose and fix whatever's going on. I did say rollup was iffy 😅

@workingjubilee
Copy link
Member

@CAD97 I usually give PRs a shot in a rollup even if they're iffy if they haven't ever been tried.

in core/alloc/std only for now, and ignoring test files
@rust-log-analyzer

This comment has been minimized.

clippy's ref to unfreeze const lint can't see through type
projections, so just avoid using a type projection inside
of the type of ONCE_INIT.
@CAD97
Copy link
Contributor Author

CAD97 commented Sep 21, 2024

Oh, I think I might see what the issue could be. The atomic within Once changed from AtomicU32 to Atomic<u32>, and clippy is failing to perform sufficient normalization to see that the type of ONCE_INIT contains interior mutability. I spent some time trying to resolve type projections from within the late lint pass, but didn't manage to do anything (I somehow made all of the clippy UI tests fail with "found staticlib std instead of rlib or dylib"). So I just took the cop-out solution and switched Once back to using AtomicU32; the next step of making Atomic<u32> the definition instead of an alias will make this limitation moot anyway.

@rust-log-analyzer

This comment has been minimized.

library/core/src/sync/atomic.rs Outdated Show resolved Hide resolved
library/core/src/sync/atomic.rs Outdated Show resolved Hide resolved
library/core/src/sync/atomic.rs Outdated Show resolved Hide resolved
@rust-log-analyzer
Copy link
Collaborator

The job x86_64-gnu-llvm-18 failed! Check out the build log: (web) (plain)

Click to see the possible cause of the failure (guessed by this bot)
------
 > importing cache manifest from ghcr.io/rust-lang/rust-ci-cache:30ca74372d8b771363f68f939a58b017a592fae4f69398600dc51145997160f03e9652051f957840c41898984a88855e9757fa23464703a5a4ba21316ddebb04:
------
##[endgroup]
Setting extra environment values for docker:  --env ENABLE_GCC_CODEGEN=1 --env GCC_EXEC_PREFIX=/usr/lib/gcc/
[CI_JOB_NAME=x86_64-gnu-llvm-18]
---
sccache: Starting the server...
##[group]Configure the build
configure: processing command line
configure: 
configure: build.configure-args := ['--build=x86_64-unknown-linux-gnu', '--llvm-root=/usr/lib/llvm-18', '--enable-llvm-link-shared', '--set', 'rust.randomize-layout=true', '--set', 'rust.thin-lto-import-instr-limit=10', '--set', 'change-id=99999999', '--enable-verbose-configure', '--enable-sccache', '--disable-manage-submodules', '--enable-locked-deps', '--enable-cargo-native-static', '--set', 'rust.codegen-units-std=1', '--set', 'dist.compression-profile=balanced', '--dist-compression-formats=xz', '--set', 'rust.lld=false', '--disable-dist-src', '--release-channel=nightly', '--enable-debug-assertions', '--enable-overflow-checks', '--enable-llvm-assertions', '--set', 'rust.verify-llvm-ir', '--set', 'rust.codegen-backends=llvm,cranelift,gcc', '--set', 'llvm.static-libstdcpp', '--enable-new-symbol-mangling']
configure: target.x86_64-unknown-linux-gnu.llvm-config := /usr/lib/llvm-18/bin/llvm-config
configure: llvm.link-shared     := True
configure: rust.randomize-layout := True
configure: rust.thin-lto-import-instr-limit := 10
---
  Downloaded boml v0.3.1
   Compiling boml v0.3.1
   Compiling y v0.1.0 (/checkout/compiler/rustc_codegen_gcc/build_system)
    Finished `release` profile [optimized] target(s) in 3.55s
     Running `/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-codegen/x86_64-unknown-linux-gnu/release/y test --use-system-gcc --use-backend gcc --out-dir /checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc --release --mini-tests --std-tests`
Using system GCC
[BUILD] example
[AOT] mini_core_hello_world
/checkout/obj/build/x86_64-unknown-linux-gnu/stage1-tools/cg_gcc/mini_core_hello_world
abc
---
##[group]Building stage0 tool linkchecker (x86_64-unknown-linux-gnu)
    Finished `release` profile [optimized] target(s) in 0.13s
##[endgroup]
##[group]Testing stage0 Linkcheck (x86_64-unknown-linux-gnu)
core/sync/atomic/type.Atomic.html:1: broken link fragment `#associatedtype.AtomicInner` pointing to `core/sync/atomic/trait.AtomicPrimitive.html`
std/sync/atomic/type.Atomic.html:1: broken link fragment `#associatedtype.AtomicInner` pointing to `std/sync/atomic/trait.AtomicPrimitive.html`
number of HTML files scanned: 43683
number of HTML redirects found: 13730
number of links checked: 3254320
number of links ignored due to external: 78974
number of links ignored due to external: 78974
number of links ignored due to exceptions: 17
number of intra doc links ignored: 8
errors found: 2
found some broken links
Command has failed. Rerun with -v to see more details.
  local time: Mon Sep 23 00:28:25 UTC 2024
  network time: Mon, 23 Sep 2024 00:28:25 GMT
##[error]Process completed with exit code 1.
Post job cleanup.

@bors
Copy link
Contributor

bors commented Sep 23, 2024

☔ The latest upstream changes (presumably #130724) made this pull request unmergeable. Please resolve the merge conflicts.

@alex-semenyuk alex-semenyuk 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-author Status: This is awaiting some action (such as code changes or more information) from the author. labels Nov 12, 2024
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
O-hermit Operating System: Hermit O-itron Operating System: ITRON O-SGX Target: SGX O-unix Operating system: Unix-like O-wasm Target: WASM (WebAssembly), http://webassembly.org/ O-windows Operating system: Windows 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.

7 participants