Remove unused `#[used]` from things that used #2167

hawkw · 2025-07-22T19:22:03Z

To paraphrase Donald Rumsfeld: as we know, there are used useds;
there are things we use. We also know there are used unuseds, that is to
say there are some things we use but the compiler doesn't know we use,
so #[used] so that the thing we use appears used. . But there are also
unused useds; the ones we use but put #[used] on so the compiler
thinks we use them, but the #[used] is unused because we already use
it.

Presently, the ringbuf!, counters!, and counted_ringbuf! macros
all place a #[used] attribute on the statics that they generate. This
is intended to prevent the compiler from eliminating writes to these
statics when they are read only by humility. However, in the case of
both ringbuf and counters, the #[used] attribute is not actually
necessary: when writing to a ringbuf, we read the last value written, to
check if they are equal, and writes to a counter use fetch_add, which
is a load as well as a store. So, we don't need the #[used] attribute
in either case.

In recent nightly toolchains, the behavior of #[used] on targets
which produce ELF binaries has changed. Now, it also tells the
linker that the value is used, as well as the compiler's own dead-code
elimination. This is unfortunate, as it means that ringbufs and counters
are never optimized out at link time, even when they are not actually
written to in a particular task. For example, consider the
drv_i2c_devices crate. This crate contains modules with drivers for a
whole bunch of different devices. Lots of tasks depend on
drv_i2c_devices, and use the drivers for one or two of those devices.
Most of the drivers have their own debugging ringbufs. When updating to
a nightly toolchain after the #[used] change, the ringbufs for all
the drivers in the crate get linked into every task that depends on
drv_i2c_devices, regardless of which drivers the task actually, well,
uses.

This commit fixes that by getting rid of the #[used] attributes. We
don't actually need them. I've flashed my Gimletlet with this change,
and all the ringbufs and counters are still there even without #[used]
on anything.

To [paraphrase Donald Rumsfeld][1]: as we know, there are used useds; there are things we use. We also know there are used unuseds, that is to say there are some things we use but the compiler doesn't know we use, so `#[used]` so that the thing we use appears used. . But there are also unused useds; the ones we use but put `#[used]` on so the compiler thinks we use them, but the `#[used]` is unused because we already use it. Presently, the `ringbuf!`, `counters!`, and `counted_ringbuf!` macros all place a `#[used]` attribute on the statics that they generate. This is intended to prevent the compiler from eliminating writes to these statics when they are read only by humility. However, in the case of both `ringbuf` and `counters`, the `#[used]` attribute is not actually necessary: when writing to a ringbuf, we read the last value written, to check if they are equal, and writes to a counter use `fetch_add`, which is a load as well as a store. So, we don't need the `#[used]` attribute in either case. In recent nightly toolchains, the [behavior of `#[used]` on targets which produce ELF binaries has changed][2]. Now, it also tells the _linker_ that the value is used, as well as the compiler's own dead-code elimination. This is unfortunate, as it means that ringbufs and counters are never optimized out at link time, even when they are not actually *written* to in a particular task. For example, consider the `drv_i2c_devices` crate. This crate contains modules with drivers for a whole bunch of different devices. Lots of tasks depend on `drv_i2c_devices`, and use the drivers for one or two of those devices. Most of the drivers have their own debugging ringbufs. When updating to a nightly toolchain after the `#[used]` change, the ringbufs for *all* the drivers in the crate get linked into *every* task that depends on `drv_i2c_devices`, regardless of which drivers the task actually, well, uses. This commit fixes that by getting rid of the `#[used]` attributes. We don't actually need them. I've flashed my Gimletlet with this change, and all the ringbufs and counters are still there even without `#[used]` on anything. [1]: https://en.wikipedia.org/wiki/There_are_unknown_unknowns [2]: #2165 (comment)

Turns out the [compiler is actually right][1] that this enum is not used when `dump-agent` is compiled with the `no-rot` feature flag, as we don't write to the ringbuf at all when that feature is set. The `#[used]` attribute was suppressing the unused type warning. Hahaha. Thus, I just made the whole thing `#[cfg(not(feature = "no-rot"))]`. Great.

To [paraphrase Donald Rumsfeld][1]: as we know, there are used useds; there are things we use. We also know there are used unuseds, that is to say there are some things we use but the compiler doesn't know we use, so `#[used]` so that the thing we use appears used. . But there are also unused useds; the ones we use but put `#[used]` on so the compiler thinks we use them, but the `#[used]` is unused because we already use it. Presently, the `ringbuf!`, `counters!`, and `counted_ringbuf!` macros all place a `#[used]` attribute on the statics that they generate. This is intended to prevent the compiler from eliminating writes to these statics when they are read only by humility. However, in the case of both `ringbuf` and `counters`, the `#[used]` attribute is not actually necessary: when writing to a ringbuf, we read the last value written, to check if they are equal, and writes to a counter use `fetch_add`, which is a load as well as a store. So, we don't need the `#[used]` attribute in either case. In recent nightly toolchains, the [behavior of `#[used]` on targets which produce ELF binaries has changed][2]. Now, it also tells the _linker_ that the value is used, as well as the compiler's own dead-code elimination. This is unfortunate, as it means that ringbufs and counters are never optimized out at link time, even when they are not actually *written* to in a particular task. For example, consider the `drv_i2c_devices` crate. This crate contains modules with drivers for a whole bunch of different devices. Lots of tasks depend on `drv_i2c_devices`, and use the drivers for one or two of those devices. Most of the drivers have their own debugging ringbufs. When updating to a nightly toolchain after the `#[used]` change, the ringbufs for *all* the drivers in the crate get linked into *every* task that depends on `drv_i2c_devices`, regardless of which drivers the task actually, well, uses. This commit fixes that by getting rid of the `#[used]` attributes. We don't actually need them. I've flashed my Gimletlet with this change, and all the ringbufs and counters are still there even without `#[used]` on anything. [1]: https://en.wikipedia.org/wiki/There_are_unknown_unknowns [2]: #2165 (comment)

See oxidecomputer/hubris#2167

There's no reason not to just use the `counters::counters!` macro for IPC counters,rather than generating the same code in idol codegen. This way, we're always in sync with what other uses of the `counters` crate get. If we had been doing this all along, we would have just gotten the `#[used]` change from oxidecomputer/hubris#2167 without having to make changes in idol.

This commit removes one more instance of the `#[used]` attribute in `ringbuf` that we don't actually need --- in the macro that generates `counted_ringbuf!`s when counters are enabled but the actual ringbuf isn't. I had somehow forgotten this in #2167, my bad!

This branch updates `idol` to a recent toolchain, `nightly-2025-07-20`. In order to generate code that compiles without warnings on this toolchain, it was necessary to change a few things. `idol` used to put the `#[automatically_derived]` attribute on all generated `impl` blocks. At some point between the previous ancient toolchain and the one from a couple days ago, this attribute was changed to only work on _trait impls_ and not _inherent impls_, and placing it on inherent impls emits a warning. The server codegen only generates trait impls, but the client code also generates an inherent impl for the client struct (which is where most of the action happens), and it was thus necessary to remove the attribute from this. One of the things `#[automatically_derived]` does is to suppress dead code and unused variable warnings. Now that these warnings are no longer suppressed in the client impl, some dead code and unused variables had to be removed to make these warnings go away. In particular, it turns out that the `<Name_of_interface>_<name_of_ipc>_REPLY` structs we used to generate with zerocopy IPC clients were unused except by Humility, so I've added `#[allow(dead_code)]` to them. Additionally, I've changed the codegen for IPC counters to just use the `counters!` macro to generate the static, instead of reimplementing it in `idol` codegen. This way, we get whatever the `counters!` macro generates, instead of having to keep it in sync. In particular, this lets us benefit from the change to remove the `#[used]` attribute from counters that was made in oxidecomputer/hubris#2167. Otherwise, it would have been necessary to also remove that attribute from `idol`'s reimplementation of the counters static generation.

This commit removes one more instance of the `#[used]` attribute in `ringbuf` that we don't actually need --- in the macro that generates `counted_ringbuf!`s when counters are enabled but the actual ringbuf isn't. I had somehow forgotten this in #2167, my bad!

Currently, Hubris' Rust toolchain is pinned to `nightly-2024-09-17`, which is from 10 months ago. This is kind of unfortunate, especially because that nightly is too old to support the Rust 2024 edition, which means we cannot update our dependencies on any crates where the latest version is built with Rust 2024. Beyond just updating the toolchain, there were some noteworthy changes: - Naked functions are now stable (yay!), but the attribute changed to `#[unsafe(naked)]`. Inline assembly in naked functions must now use `core::arch::naked_asm!` rather than normal `asm!`. As far as I can tell, the only difference between this and regular `asm!` is that it does not support `options(noreturn)`, as I believe the `naked_asm!` block kind of implies at least some of the codegen differences for `noreturn`. - New warnings on creation of temporary shared references to mutable statics showed up in `stm32h7-update-server`, where we were using `[T]::as_ptr()` on zero-sized slices in mutable statics that were used to get linker-generated addresses. `[T]::as_ptr()` takes an `&self` receiver, so this was generating a temporary shared reference to the mutable static. I changed this to use `&raw const`, which takes the address of the static without creating a shared reference. - There was a substantial regression in flash and RAM usage on the new toolchain due to [a change in the behavior of the `#[used]` attribute][1] which revealed [an underlying issue where ringbufs were not zero initialized][2]. These issues were resolved separately in #2167, #2168, #2170, and oxidecomputer/idolatry#65. In addition, there were a variety of unremarkable linting changes, including slightly better dead code detection (which detected some new dead code), and some annoying clippy nonsense. Note that this branch requires oxidecomputer/idolatry#65, which updates `idol` to generate code that doesn't emit warnings with the new toolchain, and fixes some of the flash/RAM size regression in code generated by `idol`. Fixes #2165 [1]: #2165 (comment) [2]: #2165 (comment)

To [paraphrase Donald Rumsfeld][1]: as we know, there are used useds; there are things we use. We also know there are used unuseds, that is to say there are some things we use but the compiler doesn't know we use, so `#[used]` so that the thing we use appears used. . But there are also unused useds; the ones we use but put `#[used]` on so the compiler thinks we use them, but the `#[used]` is unused because we already use it. Presently, the `ringbuf!`, `counters!`, and `counted_ringbuf!` macros all place a `#[used]` attribute on the statics that they generate. This is intended to prevent the compiler from eliminating writes to these statics when they are read only by humility. However, in the case of both `ringbuf` and `counters`, the `#[used]` attribute is not actually necessary: when writing to a ringbuf, we read the last value written, to check if they are equal, and writes to a counter use `fetch_add`, which is a load as well as a store. So, we don't need the `#[used]` attribute in either case. In recent nightly toolchains, the [behavior of `#[used]` on targets which produce ELF binaries has changed][2]. Now, it also tells the _linker_ that the value is used, as well as the compiler's own dead-code elimination. This is unfortunate, as it means that ringbufs and counters are never optimized out at link time, even when they are not actually *written* to in a particular task. For example, consider the `drv_i2c_devices` crate. This crate contains modules with drivers for a whole bunch of different devices. Lots of tasks depend on `drv_i2c_devices`, and use the drivers for one or two of those devices. Most of the drivers have their own debugging ringbufs. When updating to a nightly toolchain after the `#[used]` change, the ringbufs for *all* the drivers in the crate get linked into *every* task that depends on `drv_i2c_devices`, regardless of which drivers the task actually, well, uses. This commit fixes that by getting rid of the `#[used]` attributes. We don't actually need them. I've flashed my Gimletlet with this change, and all the ringbufs and counters are still there even without `#[used]` on anything. [1]: https://en.wikipedia.org/wiki/There_are_unknown_unknowns [2]: oxidecomputer#2165 (comment)

Turns out the [compiler is actually right][1] that this enum is not used when `dump-agent` is compiled with the `no-rot` feature flag, as we don't write to the ringbuf at all when that feature is set. The `#[used]` attribute was suppressing the unused type warning. Hahaha. Thus, I just made the whole thing `#[cfg(not(feature = "no-rot"))]`. Great.

This commit removes one more instance of the `#[used]` attribute in `ringbuf` that we don't actually need --- in the macro that generates `counted_ringbuf!`s when counters are enabled but the actual ringbuf isn't. I had somehow forgotten this in oxidecomputer#2167, my bad!

Currently, Hubris' Rust toolchain is pinned to `nightly-2024-09-17`, which is from 10 months ago. This is kind of unfortunate, especially because that nightly is too old to support the Rust 2024 edition, which means we cannot update our dependencies on any crates where the latest version is built with Rust 2024. Beyond just updating the toolchain, there were some noteworthy changes: - Naked functions are now stable (yay!), but the attribute changed to `#[unsafe(naked)]`. Inline assembly in naked functions must now use `core::arch::naked_asm!` rather than normal `asm!`. As far as I can tell, the only difference between this and regular `asm!` is that it does not support `options(noreturn)`, as I believe the `naked_asm!` block kind of implies at least some of the codegen differences for `noreturn`. - New warnings on creation of temporary shared references to mutable statics showed up in `stm32h7-update-server`, where we were using `[T]::as_ptr()` on zero-sized slices in mutable statics that were used to get linker-generated addresses. `[T]::as_ptr()` takes an `&self` receiver, so this was generating a temporary shared reference to the mutable static. I changed this to use `&raw const`, which takes the address of the static without creating a shared reference. - There was a substantial regression in flash and RAM usage on the new toolchain due to [a change in the behavior of the `#[used]` attribute][1] which revealed [an underlying issue where ringbufs were not zero initialized][2]. These issues were resolved separately in oxidecomputer#2167, oxidecomputer#2168, oxidecomputer#2170, and oxidecomputer/idolatry#65. In addition, there were a variety of unremarkable linting changes, including slightly better dead code detection (which detected some new dead code), and some annoying clippy nonsense. Note that this branch requires oxidecomputer/idolatry#65, which updates `idol` to generate code that doesn't emit warnings with the new toolchain, and fixes some of the flash/RAM size regression in code generated by `idol`. Fixes oxidecomputer#2165 [1]: oxidecomputer#2165 (comment) [2]: oxidecomputer#2165 (comment)

hawkw requested a review from cbiffle July 22, 2025 19:22

hawkw enabled auto-merge (squash) July 22, 2025 19:23

hawkw disabled auto-merge July 22, 2025 19:25

hawkw enabled auto-merge (squash) July 22, 2025 19:25

hawkw added 2 commits July 22, 2025 12:35

hawkw force-pushed the eliza/unused-used branch from db20fbb to 79c723b Compare July 22, 2025 19:36

hawkw disabled auto-merge July 22, 2025 19:36

hawkw enabled auto-merge (rebase) July 22, 2025 19:36

cbiffle approved these changes Jul 22, 2025

View reviewed changes

hawkw merged commit f908b60 into master Jul 22, 2025
135 checks passed

hawkw deleted the eliza/unused-used branch July 22, 2025 19:45

hawkw mentioned this pull request Jul 22, 2025

we should probably update to a nightly toolchain that's not from 2024 #2165

Closed

hawkw added a commit to oxidecomputer/idolatry that referenced this pull request Jul 23, 2025

Don't use unused #[used] for counters statics

7819a80

See oxidecomputer/hubris#2167

hawkw mentioned this pull request Jul 23, 2025

Update toolchain to nightly-2025-07-20 oxidecomputer/idolatry#65

Merged

This was referenced Jul 23, 2025

ringbuf: Remove unused #[used] we weren't using #2170

Merged

Update to nightly-2025-07-20 #2169

Merged

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Uh oh!

Remove unused `#[used]` from things that used #2167

Remove unused `#[used]` from things that used #2167

Uh oh!

hawkw commented Jul 22, 2025 •

edited

Loading

Uh oh!

Uh oh!

Uh oh!

Remove unused #[used] from things that used #2167

Remove unused #[used] from things that used #2167

Uh oh!

Conversation

hawkw commented Jul 22, 2025 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

Uh oh!

Uh oh!

Remove unused `#[used]` from things that used #2167

Remove unused `#[used]` from things that used #2167

hawkw commented Jul 22, 2025 •

edited

Loading