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

Bump the toolchain to latest nightly. #1703

Merged
merged 3 commits into from
Apr 4, 2024

Conversation

cbiffle
Copy link
Collaborator

@cbiffle cbiffle commented Apr 1, 2024

Also fixed some new warnings reported by the toolchain, and many places where rustfmt has apparently changed its mind about some things.

Removed size constraints for tasks that increased in size rather than playing guess-the-number.

This leaves a number of warnings, which I'm hoping to divide up and not have to fix myself.

@cbiffle cbiffle force-pushed the cbiffle/toolchain-update-2024-04 branch 3 times, most recently from 3e64525 to cef2175 Compare April 2, 2024 19:48
@cbiffle cbiffle changed the title Bump the toolchain to the nightly equivalent of 1.75.0 Bump the toolchain to latest nightly. Apr 2, 2024
@cbiffle
Copy link
Collaborator Author

cbiffle commented Apr 2, 2024

After some discussion with upstream, I've gone ahead and bumped this to latest nightly to pick up some size fixes by @saethlin and others. This has caused a cavalcade of new warnings, most of which are valid, some of which are Clippy bugs.

Currently this PR has Humility and Clippy tests disabled:

  • Humility, because a bug in its DWARF parser prevents it from parsing output of rust 1.75 and later, and
  • Clippy, because it is complaining about valid things in code I didn't write and would prefer not to have to fix.

@cbiffle cbiffle force-pushed the cbiffle/toolchain-update-2024-04 branch 2 times, most recently from 776cd26 to 36d7af1 Compare April 2, 2024 23:06
Copy link
Member

@hawkw hawkw left a comment

Choose a reason for hiding this comment

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

overall, this seems correct to me once the requisite Humility changes and clippy fixes are done!

Cargo.toml Show resolved Hide resolved
app/gimlet/base.toml Outdated Show resolved Hide resolved
Comment on lines 44 to 49
// We'd love to use an AtomicBool here but we gotta support ARMv6M.
let previous_fail =
core::mem::replace(unsafe { &mut KERNEL_HAS_FAILED }, true);
// This could probably become SyncUnsafeCell in a future where it exists.
let previous_fail = core::mem::replace(
unsafe { &mut *core::ptr::addr_of_mut!(KERNEL_HAS_FAILED) },
true,
);
Copy link
Member

Choose a reason for hiding this comment

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

Alternatively, as per the above comment re: AtomicBool, could we use armv6m-atomic-hack here instead? Or is there a reason not to?

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

That doesn't work in the kernel. It could probably be made to work in the kernel, but I haven't really looked into it yet.

Copy link

Choose a reason for hiding this comment

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

Are you sure you don't want to use ptr::replace here? Reborrowing into &mut after you've used addr_of_mut! is odd.

Copy link

Choose a reason for hiding this comment

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

Oh I see, the point of this function is to return &'static mut. So I suppose some friction with the lint is unavoidable.

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

Hi @saethlin! I think you're right actually, because the two blocks are operating on two different statics. I have replaced the replace with core::ptr::replace, which it appears I didn't know existed when I wrote this.

Copy link
Member

Choose a reason for hiding this comment

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

That doesn't work in the kernel. It could probably be made to work in the kernel, but I haven't really looked into it yet.

In that case, I might look into it --- do you happen to remember why it doesn't work in the kernel? It does seem like AtomicBool would be the ideologically correct solution to this...

Copy link
Collaborator Author

Choose a reason for hiding this comment

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

The atomic hack crate assumes that it's in a Hubris task where there is no concurrency or nonlocal control flow, so it basically just negates the critical section.

In the kernel on v6M we'd probably want to disable-reenable interrupts to be strictly-strictly correct in the face of user-written interrupt service routines. This might be a case where portable-atomics is the right choice.

@cbiffle cbiffle force-pushed the cbiffle/toolchain-update-2024-04 branch 6 times, most recently from cb8f3d0 to f484001 Compare April 4, 2024 19:40
@cbiffle cbiffle marked this pull request as ready for review April 4, 2024 19:41
@cbiffle cbiffle requested review from flihp and labbott as code owners April 4, 2024 19:41
cbiffle added 2 commits April 4, 2024 12:41
Also fixed _some_ new warnings reported by the toolchain, and many
places where rustfmt has apparently changed its mind about some things.

Removed size constraints for tasks that increased in size rather than
playing guess-the-number.

This leaves a number of warnings, which I'm hoping to divide up and not
have to fix myself.
*famous last words
@cbiffle cbiffle force-pushed the cbiffle/toolchain-update-2024-04 branch from f484001 to 1f10682 Compare April 4, 2024 19:41
@cbiffle
Copy link
Collaborator Author

cbiffle commented Apr 4, 2024

Humility is back on after the fix there made it into the nightly build. Clippy is still off, will file bugs to track once we get this in (since that will mean the toolchain version stops changing).

@cbiffle cbiffle enabled auto-merge (rebase) April 4, 2024 21:02
@cbiffle cbiffle disabled auto-merge April 4, 2024 21:02
@cbiffle cbiffle force-pushed the cbiffle/toolchain-update-2024-04 branch from 531c21c to 4a823a5 Compare April 4, 2024 21:12
This was adding 20 kiB (!) to the task because (1) it wasn't getting
inlined and (2) its non-inlined implementation contains a panic that
pulls in f32 formatting code, which has recently gotten really
expensive.
@cbiffle cbiffle force-pushed the cbiffle/toolchain-update-2024-04 branch from 4a823a5 to d6cf1e3 Compare April 4, 2024 21:20
Copy link
Contributor

@flihp flihp left a comment

Choose a reason for hiding this comment

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

LGTM

@cbiffle cbiffle merged commit 8272a5c into master Apr 4, 2024
103 checks passed
@cbiffle cbiffle deleted the cbiffle/toolchain-update-2024-04 branch April 4, 2024 21:40
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

4 participants