BPF: Disable atomic CAS #106796

vadorovsky · 2023-01-13T06:18:15Z

Enabling CAS for BPF targets (#105708) breaks the build of core library.
The failure occurs both when building rustc for BPF targets and when
building crates for BPF targets with the current nightly.

The LLVM BPF backend does not correctly lower all atomicrmw operations
and crashes for unsupported ones.

Before we can enable CAS for BPF in Rust, we need to fix the LLVM BPF
backend first.

Fixes #106795

Signed-off-by: Michal Rostecki vadorovsky@gmail.com

rustbot · 2023-01-13T06:18:23Z

Thanks for the pull request, and welcome! The Rust team is excited to review your changes, and you should hear from @cjgillot (or someone else) soon.

Please see the contribution instructions for more information.

rustbot · 2023-01-13T06:18:26Z

These commits modify compiler targets.
(See the Target Tier Policy.)

vadorovsky · 2023-01-14T01:10:56Z

/cc @nagisa @tomerze @alessandrod

alessandrod · 2023-01-14T09:20:13Z

There are at least two separate issues here I think:

The atomic_int macro is defined only for [cfg(target_has_atomic_load_store = "8")]. That should probably be changed to check for all the _load_store sizes.
The LLVM BPF backend does not correctly lower all atomicrmw ops, and will crash for unsupported ones, so atomic_cas should be reverted back to false. The min_atomic_width definition should probably stay tho.

The bpfel-* targets are completely broken for non trivial programs (pretty much all aya programs and CI are broken with recent nightly), so I'd like to get this merged asap.

cjgillot · 2023-01-14T09:35:19Z

r? @nagisa

tomerze · 2023-01-14T09:39:30Z

There are at least two separate issues here I think:
* The `atomic_int` macro is defined only for `[cfg(target_has_atomic_load_store = "8")]`. That should probably be changed to check for all the `_load_store` sizes.

* The LLVM BPF backend does not correctly lower all `atomicrmw` ops, and will crash for unsupported ones, so `atomic_cas` should be reverted back to false. The `min_atomic_width` definition should probably stay tho.
The bpfel-* targets are completely broken for non trivial programs (pretty much all aya programs and CI are broken with recent nightly), so I'd like to get this merged asap.

We should probably fix the first issue regardless, for people who might be creating a custom target json.

alessandrod · 2023-01-14T09:41:32Z

We should probably fix the first issue regardless, for people who might be creating a custom target json.

Yes, I think we should just revert atomic_cas back to false here, then fix the atomic_int macro definition in a separate PR.

vadorovsky · 2023-01-14T10:01:31Z

There are at least two separate issues here I think:
* The `atomic_int` macro is defined only for `[cfg(target_has_atomic_load_store = "8")]`. That should probably be changed to check for all the `_load_store` sizes.

* The LLVM BPF backend does not correctly lower all `atomicrmw` ops, and will crash for unsupported ones, so `atomic_cas` should be reverted back to false. The `min_atomic_width` definition should probably stay tho.
The bpfel-* targets are completely broken for non trivial programs (pretty much all aya programs and CI are broken with recent nightly), so I'd like to get this merged asap.
We should probably fix the first issue regardless, for people who might be creating a custom target json.

I already prepared a patch for the first issue, I will submit it soon.

Enabling CAS for BPF targets (rust-lang#105708) breaks the build of core library. The failure occurs both when building rustc for BPF targets and when building crates for BPF targets with the current nightly. The LLVM BPF backend does not correctly lower all `atomicrmw` operations and crashes for unsupported ones. Before we can enable CAS for BPF in Rust, we need to fix the LLVM BPF backend first. Fixes rust-lang#106795 Signed-off-by: Michal Rostecki <vadorovsky@gmail.com>

vadorovsky · 2023-01-14T14:18:03Z

@alessandrod According to your suggestion, I'm changing here only atomic_cas to false. Therefore, it's not a simple revert anymore, so I added a more elaborate commit message.

Please note that we need also #106856 to fully fix BPF. Since we keep min_atomic_width untouched (64) in this PR, the problem with #[cfg(target_has_atomic = "8")] still remains. So we need to merge both of them.

This until rust-lang/rust#106796 gets fixed

vadorovsky · 2023-01-20T09:10:05Z

@nagisa ping

vadorovsky · 2023-01-20T09:11:40Z

/cc @bjorn3

bjorn3 · 2023-01-20T10:14:19Z

This change makes sense to me. It was already disabled up until very recently and is clearly broken.

@bors r+

bors · 2023-01-20T10:14:21Z

📌 Commit 651e873 has been approved by bjorn3

It is now in the queue for this repository.

We need to wait for the following fixes in Rust nightly to be able to use the newest one: * rust-lang/rust#106796 * rust-lang/rust#106856 Signed-off-by: Michal Rostecki <vadorovsky@gmail.com>

bjorn3 · 2023-01-23T08:57:53Z

#106856 should be enough to fix that error, right?

taiki-e · 2023-01-23T09:03:30Z

Oh, sorry, I misunderstood that this was a PR to fix #106845.

bjorn3 · 2023-01-23T10:41:44Z

I believe that PR was split out of this one. I did notice that this PR still has "Fixes #106795" in the description, so the issue will need to be re-opened once this PR lands.

vadorovsky · 2023-01-23T11:46:02Z

Sorry for the confusion with issues! Yes, the other PR got split from this one, since we created a separate issue for fixing the annotations - #106845

When in fact, this PR is a partial fix for #106795 and the other PR fixes both issues ultimately.

Rollup of 7 pull requests Successful merges: - rust-lang#106796 (BPF: Disable atomic CAS) - rust-lang#106886 (Make stage2 rustdoc and proc-macro-srv disableable in x.py install) - rust-lang#107101 (Filter param-env predicates for errors before calling `to_opt_poly_trait_pred`) - rust-lang#107109 (ThinBox: Add intra-doc-links for Metadata) - rust-lang#107148 (remove error code from `E0789`, add UI test/docs) - rust-lang#107151 (Instantiate dominators algorithm only once) - rust-lang#107153 (Consistently use dominates instead of is_dominated_by) Failed merges: r? `@ghost` `@rustbot` modify labels: rollup

rustbot assigned cjgillot Jan 13, 2023

rustbot added S-waiting-on-review Status: Awaiting review from the assignee but also interested parties. T-compiler Relevant to the compiler team, which will review and decide on the PR/issue. labels Jan 13, 2023

vadorovsky force-pushed the revert-105708-enable-atomic-cas-bpf branch from c632abf to be5516b Compare January 13, 2023 14:18

rustbot assigned nagisa and unassigned cjgillot Jan 14, 2023

tomerze mentioned this pull request Jan 14, 2023

Some functions in core::sync are annotated to only support targets with 8-bit atomic support #106845

Closed

vadorovsky force-pushed the revert-105708-enable-atomic-cas-bpf branch from be5516b to 11c77c5 Compare January 14, 2023 13:42

vadorovsky force-pushed the revert-105708-enable-atomic-cas-bpf branch from 11c77c5 to 651e873 Compare January 14, 2023 14:14

vadorovsky changed the title ~~Revert "Enable atomic cas for bpf targets"~~ BPF: Disable atomic CAS Jan 14, 2023

alessandrod added a commit to alessandrod/aya that referenced this pull request Jan 19, 2023

Pin nightly to nightly-2023-01-10

df73936

This until rust-lang/rust#106796 gets fixed

alessandrod mentioned this pull request Jan 19, 2023

Pin nightly to nightly-2023-01-10 aya-rs/aya#494

Merged

alessandrod added a commit to aya-rs/aya that referenced this pull request Jan 19, 2023

Pin nightly to nightly-2023-01-10

d33ed21

This until rust-lang/rust#106796 gets fixed

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 Jan 20, 2023

vadorovsky mentioned this pull request Jan 22, 2023

examples: Pin Rust nightly to 2023-01-10 aya-rs/book#87

Merged

This comment was marked as resolved.

Sign in to view

JohnTitor mentioned this pull request Jan 23, 2023

Rollup of 7 pull requests #107220

Merged

bors merged commit 9e79642 into rust-lang:master Jan 23, 2023

rustbot added this to the 1.69.0 milestone Jan 23, 2023

vadorovsky deleted the revert-105708-enable-atomic-cas-bpf branch January 24, 2023 00:21

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

BPF: Disable atomic CAS #106796

BPF: Disable atomic CAS #106796

vadorovsky commented Jan 13, 2023 •

edited

Loading

rustbot commented Jan 13, 2023

rustbot commented Jan 13, 2023

vadorovsky commented Jan 14, 2023

alessandrod commented Jan 14, 2023 •

edited

Loading

cjgillot commented Jan 14, 2023

tomerze commented Jan 14, 2023 •

edited

Loading

alessandrod commented Jan 14, 2023

vadorovsky commented Jan 14, 2023

vadorovsky commented Jan 14, 2023 •

edited

Loading

vadorovsky commented Jan 20, 2023

vadorovsky commented Jan 20, 2023

bjorn3 commented Jan 20, 2023

bors commented Jan 20, 2023

This comment was marked as resolved.

bjorn3 commented Jan 23, 2023

taiki-e commented Jan 23, 2023 •

edited

Loading

bjorn3 commented Jan 23, 2023

vadorovsky commented Jan 23, 2023

BPF: Disable atomic CAS #106796

BPF: Disable atomic CAS #106796

Conversation

vadorovsky commented Jan 13, 2023 • edited Loading

rustbot commented Jan 13, 2023

rustbot commented Jan 13, 2023

vadorovsky commented Jan 14, 2023

alessandrod commented Jan 14, 2023 • edited Loading

cjgillot commented Jan 14, 2023

tomerze commented Jan 14, 2023 • edited Loading

alessandrod commented Jan 14, 2023

vadorovsky commented Jan 14, 2023

vadorovsky commented Jan 14, 2023 • edited Loading

vadorovsky commented Jan 20, 2023

vadorovsky commented Jan 20, 2023

bjorn3 commented Jan 20, 2023

bors commented Jan 20, 2023

This comment was marked as resolved.

bjorn3 commented Jan 23, 2023

taiki-e commented Jan 23, 2023 • edited Loading

bjorn3 commented Jan 23, 2023

vadorovsky commented Jan 23, 2023

vadorovsky commented Jan 13, 2023 •

edited

Loading

alessandrod commented Jan 14, 2023 •

edited

Loading

tomerze commented Jan 14, 2023 •

edited

Loading

vadorovsky commented Jan 14, 2023 •

edited

Loading

taiki-e commented Jan 23, 2023 •

edited

Loading