Fix various cfg-related crimes #1774

cbiffle · 2024-05-06T21:18:03Z

The nightly toolchain started doing checking for cfgs on Sunday, so I bumped our toolchain revision locally to see what it'd find.

In short, it got big mad.

This PR fixes All The Things. The commits provide detailed descriptions, but here's a summary:

Boards (as in target_board) now have to be defined somewhere. I've added a boards directory containing (currently empty) config files per board. Any reference to a target_board without a corresponding file will now be treated as a typo.
Renamed some boards that only mentioned their processor type and not the board itself.
Removed vestiges of the psc-a and sidecar-a boards, which saw firmware support ended a while ago, but were never fully removed from the code.
Removed the final bits of STM32H7B3 devel board support, something we haven't used in ~4 years at this point.
Removed dead code implementing a traptrace feature in Sidecar, which isn't actually in the Cargo.toml and thus can't be built.
Fixed stm32xx-i2c, which was attempting to key off of target_board without actually exposing the cfg in its build script. Forgetting to do this is now a compiler warning.
Fixed jefe, which was attempting to key off of armv8m without actually exposing the cfg in its build script. Also now a warning.
Fixed build/i2c to use its own features instead of generating code that tried to match on features in its clients, which was working by accident.

In order to formally move us to a toolchain that prevents this from regressing, I had to do some small unrelated fixes, namely:

Stop using the core::uXX::MAX constants, which are deprecated now.
Bump a couple deps because their derive macros produced code the new compiler dislikes.
Add a missing syn feature in derive-idol-err, which had apparently been picking it up from some dependency.

mkeeter · 2024-05-07T14:20:10Z

build/util/src/lib.rs

+
+    let values = boards.join(",");
+
+    println!("cargo:rustc-check-cfg=cfg(target_board, values({values}))");


it took me a while to figure out what was going on here, but it's amazing

mkeeter · 2024-05-07T14:30:54Z

drv/stm32xx-sys-api/src/h7.rs

-    #[cfg(feature = "h7b3")]
-    OctoSpi1 = periph(Group::Ahb3, 14), // B3 only
-    #[cfg(any(feature = "h743", feature = "h747", feature = "h753"))]
+    #[cfg(any(feature = "h743", feature = "h753"))]


Is any(feature = "h743", feature = "h753") guaranteed to be true? Now that we've removed h7b3 support, h743 and h753 are the only remaining h7* features, so I think we can remove this condition (and others below).

mkeeter · 2024-05-07T14:33:54Z

drv/stm32xx-sys-api/src/h7.rs

@@ -158,12 +151,9 @@ pub enum Peripheral {
    Swp = periph(Group::Apb1H, 2),
    Crsen = periph(Group::Apb1H, 1),

-    #[cfg(feature = "h7b3")]
-    Dfsdm1 = periph(Group::Apb2, 30), // B3 differs from 43/47
-
    Hrtim = periph(Group::Apb2, 29), // 43/47 only


I'm confused by the comments in this file – there are many lines which was 43/47 only but aren't gated on h743.

(we could also probably remove the references to the B3 and 47 more generally, since they're both unsupported)

The lines aren't consistently conditional because I felt the danger of having someone accidentally name a peripheral that isn't implemented on their chip was small. I would like to at least keep the comments; we could comment out the lines that are only on chips we don't support, if you'd prefer.

mkeeter · 2024-05-07T14:35:26Z

README.mkdn

- `cargo xtask dist app/demo-stm32g0-nucleo/app-g0b1.toml` - stm32g0b1
+- `cargo xtask dist app/demo-stm32g0-nucleo/app-g031.toml` - stm32g031-nucleo
+- `cargo xtask dist app/demo-stm32g0-nucleo/app-g070.toml` - stm32g070-nucleo
+- `cargo xtask dist app/demo-stm32g0-nucleo/app-g0b1.toml` - stm32g0b1-nucleo


Naming nit: should nucleo be at the start or end? We have both stm32g0b1-nucleo and nucleo-ih743zi2 board names.

It appears that the vendor consistently puts nucleo at the front of the name, so, that's an argument for consistently doing that too.

mkeeter · 2024-05-07T14:37:00Z

task/hiffy/src/main.rs

@@ -71,7 +71,6 @@ cfg_if::cfg_if! {
        target_board = "gimlet-d",
        target_board = "gimlet-e",
        target_board = "gimlet-f",
-        target_board = "sidecar-a",


mkeeter · 2024-05-07T14:37:34Z

task/power/src/main.rs

-        target_board = "psc-b",
-        target_board = "psc-c"
-    ),
+    any(target_board = "psc-b", target_board = "psc-c"),
    path = "bsp/psc_abc.rs"


Nit: rename to psc_bc.rs now that rev A is gone?

mkeeter · 2024-05-07T14:38:50Z

Thanks for doing all of this cleanup!

With the toolchain bump, do you see any changes to stackmargin for our in-product images?

hawkw · 2024-05-07T15:19:10Z

boards/README.mkdn

hawkw · 2024-05-07T15:21:18Z

build/util/src/lib.rs

+            let Ok(dirent) = dirent else {
+                continue;
+            };


nit: should we maybe print a warning on weird/bad dir entries?

aapoalas

Few comments passing by

aapoalas · 2024-05-07T18:44:46Z

build/util/src/lib.rs

+        );
+    }
+    boards.sort();
+    boards.dedup();


nitpick: Duplicates would probably be a mistake of some sort, maybe worth a warning print as well?

Wait, is it even possible to get duplicates since we take only toml files' stems and don't do eg. lowercasing? Shouldn't all the file stems be necessarily unique?

aapoalas · 2024-05-07T18:51:06Z

drv/stm32xx-sys-api/src/h7.rs

    Hrtim = periph(Group::Apb2, 29), // 43/47 only

-    #[cfg(any(feature = "h743", feature = "h747", feature = "h757"))]
+    #[cfg(feature = "h743")]
    Dfsdm1 = periph(Group::Apb2, 28), // 43/47 differ from B3

    Sai3 = periph(Group::Apb2, 24), // 43/47 only


question: There's a bunch of "B3 only" commented lines below: Are those now entirely unused?

EDIT: Specifically these are Usart10, Uart9, Dfsdm2, Dts, and Dac2.

cbiffle · 2024-05-07T19:35:55Z

With the toolchain bump, do you see any changes to stackmargin for our in-product images?

I have not yet been able to test this. I'd be willing to remove the toolchain bump from this PR, but the changes may regress and I'd like to opt out of refixing them.

cbiffle · 2024-09-18T23:11:21Z

Okay, I have resuscitated this PR after getting distracted for several months.

I've fixed some of the things people suggested (thanks folks!) and left a few for future work -- basically in cases where this seems like a big improvement over what we had before, and the further improvement would take thought.

I'd appreciate a re-review; I've kept changes from code review at the top of the branch.

aapoalas

Went in, looking for a new outlook to life and programming: Found only questions.

Joking aside, I have no right to say this but the changes look good to me.

aapoalas · 2024-09-19T04:35:18Z

build/util/src/lib.rs

+        );
+    }
+    boards.sort();
+    boards.dedup();


Wait, is it even possible to get duplicates since we take only toml files' stems and don't do eg. lowercasing? Shouldn't all the file stems be necessarily unique?

aapoalas · 2024-09-19T04:39:29Z

drv/stm32xx-i2c/src/lib.rs

-            // (3417); on g031, this is 195.88 (196).
+            // t_i2cclk this yields 1219.7 (1220); on g031, this is 195.88
+            // (196). Note that these numbers make assumptions about the
+            // system's clocking and clock tree configuration; TODO.


question: What is the TODO here about exactly? Is it about finishing this comment or something code related?

The TODO is intended to be about the earlier part of the sentence: when visiting this file I noticed that it contains hardcoded assumptions about the system's clocking and clock tree configuration, which are under the control of the application main file. This should eventually be fixed.

Thank you, that explains it really well.

mkeeter · 2024-09-19T12:39:37Z

The code changes look good to me, but I'd still like to see before / after stackmargin values for Gimlet / Sidecar / PSC, to make sure the compiler change hasn't made any Exciting New Choices About Inlining 😅

cbiffle · 2024-09-19T16:54:49Z

The code changes look good to me, but I'd still like to see before / after stackmargin values for Gimlet / Sidecar / PSC, to make sure the compiler change hasn't made any Exciting New Choices About Inlining 😅

Agreed, will work on that today.

I may also bump the compiler to a more recent version, since it's been months since I picked this one. If we're testing stackmargin anyway...

labbott · 2024-09-19T18:32:31Z

boards/README.mkdn

@@ -0,0 +1,8 @@
+# Board definitions


Do we have a full list of "adding a new board" steps written down somewhere?

I don't believe we do. Eventually I'd like "add a file here" to be the whole list, but as you may have noticed, xtask flash and friends still have hardcoded match statements after this change. (I would love to fix that.)

cbiffle · 2024-09-20T21:19:18Z

Alright! I have pushed commits addressing most of the code review feedback. I have also bumped the toolchain rev to 2024-09-17, arbitrarily chosen as "the nightly that seemed to work and was closest to the date that I made the change." This corresponds to a post-1.82 version.

I've done stackmargin testing on all production boards, though only one rev of each, due to hardware availability (and, frankly, time). Some stacks are up, many are down -- in general the code appears to have gotten a little tighter with the updated toolchain. The only problems I found were

Gimlet's hf task was suspiciously low on stack, but I found 2 kiB of unnecessary data on its stack and fixed that in hf-api: knock 2 kiB off stack usage #1874, so it's good now.
PSC's eeprom task reported a stack margin of zero, but that turned out to be a kernel bug (Stack scribbling does not work for tasks with pow2 stack and no other RAM. #1876) that I will fix later. The task itself appears to be mostly okay, but should probably be removed from production images, as it appears to be a debug/test fixture.

So given that, how are people feeling about the change?

cbiffle · 2024-09-20T21:52:47Z

Ugh Clippy has developed some new opinions about things.

flihp · 2024-09-26T22:02:06Z

README.mkdn

- `cargo xtask dist app/demo-stm32g0-nucleo/app-g0b1.toml` - stm32g0b1
+- `cargo xtask dist app/demo-stm32g0-nucleo/app-g031.toml` - stm32g031-nucleo
+- `cargo xtask dist app/demo-stm32g0-nucleo/app-g070.toml` - stm32g070-nucleo
+- `cargo xtask dist app/demo-stm32g0-nucleo/app-g0b1.toml` - stm32g0b1-nucleo


Nit: app/demo-stm32g0-nucleo/app-g0b1.toml was renamed to app/demo-stm32g0-nucleo/app-g0b1.toml.noworky back in c12d361

These were named after the generic processor model, which isn't right.

This code can't be built, because we have no board support for PSC A anymore.

All of this code was statically unreachable and hasn't been built in... years, at this point. If we want it we can find it in version control.

There's no way to turn this on, so it definitely hasn't been tested in $PERIOD.

The trailing compile_error! will do fine, and this prevents it from matching on something that rustc thinks is a typo'd board name.

Jefe had a check that attempted to avoid building dump support on LPC55, but that check didn't work, because it was checking for the presence of one of our custom cfgs...but not doing the invocation in its build script that actually generates that cfg. rustc will start checking such things soon, which is how I found this.

This crate was generating code that required the _client crate_ to have certain features defined, which is almost certainly a mistake, since this crate has the same set of features. Changed to key off features in the code generator instead.

As of this commit, the following two classes of mistakes will be caught when using a recent rustc: 1. Detecting the armv6m, armv7m, or armv8m features without a build script that actually exposes them. 2. Misspelling a target_board.

We were apparently picking up this feature from a dep, which changed when I attempted a cargo update.

In the interest of not having to run around cleaning up every crate, I've adjusted the workspace lints to the least common denominator. This means one in particular -- clippy::wildcard_imports -- is no longer a warning. To avoid losing my progress on removing wildcard_imports, I've moved that into each fixed crate as a #![forbid].

Clippy gets mad about this now.

This commit contains some code changes that have to be made atomically with the toolchain, to avoid angering our warnings check in the build: - Removed now-stable asm_const feature - Fixed some addr_of instances that are now safe (woo!)

cbiffle force-pushed the cbiffle/cfg-check branch 5 times, most recently from a212a56 to 1d3d2ad Compare May 6, 2024 22:16

cbiffle changed the title ~~Cbiffle/cfg check~~ Fix various cfg-related crimes May 6, 2024

cbiffle force-pushed the cbiffle/cfg-check branch 2 times, most recently from 868fcf3 to 52287f2 Compare May 6, 2024 22:35

cbiffle marked this pull request as ready for review May 6, 2024 22:36

cbiffle requested review from flihp and labbott as code owners May 6, 2024 22:36

cbiffle requested a review from mkeeter May 6, 2024 22:36

mkeeter reviewed May 7, 2024

View reviewed changes

hawkw reviewed May 7, 2024

View reviewed changes

aapoalas reviewed May 7, 2024

View reviewed changes

cbiffle force-pushed the cbiffle/cfg-check branch from 52287f2 to 2e07a83 Compare September 18, 2024 22:33

aapoalas reviewed Sep 19, 2024

View reviewed changes

labbott reviewed Sep 19, 2024

View reviewed changes

cbiffle force-pushed the cbiffle/cfg-check branch from 349de39 to 174e864 Compare September 20, 2024 21:19

mkeeter approved these changes Sep 23, 2024

View reviewed changes

cbiffle force-pushed the cbiffle/cfg-check branch 4 times, most recently from e3ecb11 to cee048b Compare September 26, 2024 20:56

mkeeter approved these changes Sep 26, 2024

View reviewed changes

flihp reviewed Sep 26, 2024

View reviewed changes

flihp approved these changes Sep 26, 2024

View reviewed changes

cbiffle added 17 commits September 27, 2024 13:21

Rename STM32G0 Nucleo boards to have "nucleo" in their name.

149a2ff

These were named after the generic processor model, which isn't right.

Remove conditional code for unsupported psc-a rev.

3a71d5f

This code can't be built, because we have no board support for PSC A anymore.

Remove final vestiges of STM32H7B3 support.

88d7255

All of this code was statically unreachable and hasn't been built in... years, at this point. If we want it we can find it in version control.

hiffy: remove dead sidecar-a code.

93af602

sidecar: remove apparently dead traptrace feature.

5b7d43f

There's no way to turn this on, so it definitely hasn't been tested in $PERIOD.

gimlet: drop cfg for gimlet-a

459bcbb

The trailing compile_error! will do fine, and this prevents it from matching on something that rustc thinks is a typo'd board name.

stm32xx-i2c: expose target_board so cfgs work

8d909d9

build/i2c: use your own darn features

1401919

This crate was generating code that required the _client crate_ to have certain features defined, which is almost certainly a mistake, since this crate has the same set of features. Changed to key off features in the code generator instead.

Emit check-cfg lines for custom cfgs.

426b724

As of this commit, the following two classes of mistakes will be caught when using a recent rustc: 1. Detecting the armv6m, armv7m, or armv8m features without a build script that actually exposes them. 2. Misspelling a target_board.

stop using deprecated core::xxx::MAX constants.

7dff6e7

derive-idol-error: add missing syn feature

34b49a6

We were apparently picking up this feature from a dep, which changed when I attempted a cargo update.

Bump zeroize to fix "non-local impl" warning.

db77434

attest: prefer inspect_err to map_err

3fc976e

Clippy gets mad about this now.

fpga-regmap: convert single-case match to if let

28414e2

Bump toolchain to 2024-09-17

a744383

This commit contains some code changes that have to be made atomically with the toolchain, to avoid angering our warnings check in the build: - Removed now-stable asm_const feature - Fixed some addr_of instances that are now safe (woo!)

cbiffle force-pushed the cbiffle/cfg-check branch from cee048b to a744383 Compare September 27, 2024 20:21

cbiffle merged commit 39fccf8 into master Sep 27, 2024
106 checks passed

cbiffle deleted the cbiffle/cfg-check branch September 27, 2024 20:39

cbiffle mentioned this pull request Oct 1, 2024

Giant match statements in flash xtask are fragile, error-prone #1886

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Fix various cfg-related crimes #1774

Fix various cfg-related crimes #1774

cbiffle commented May 6, 2024 •

edited

Loading

mkeeter May 7, 2024

mkeeter May 7, 2024

mkeeter May 7, 2024

cbiffle Sep 18, 2024

mkeeter May 7, 2024

cbiffle Sep 18, 2024

mkeeter May 7, 2024

mkeeter May 7, 2024

mkeeter commented May 7, 2024

hawkw May 7, 2024

hawkw May 7, 2024

aapoalas left a comment

aapoalas May 7, 2024

aapoalas Sep 19, 2024

aapoalas May 7, 2024 •

edited

Loading

cbiffle commented May 7, 2024

cbiffle commented Sep 18, 2024

aapoalas left a comment

aapoalas Sep 19, 2024

aapoalas Sep 19, 2024

cbiffle Sep 19, 2024

aapoalas Sep 19, 2024

mkeeter commented Sep 19, 2024

cbiffle commented Sep 19, 2024

labbott Sep 19, 2024

cbiffle Sep 20, 2024

cbiffle commented Sep 20, 2024

cbiffle commented Sep 20, 2024

flihp Sep 26, 2024


		let values = boards.join(",");

		println!("cargo:rustc-check-cfg=cfg(target_board, values({values}))");

Fix various cfg-related crimes #1774

Fix various cfg-related crimes #1774

Conversation

cbiffle commented May 6, 2024 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeter commented May 7, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aapoalas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

aapoalas May 7, 2024 • edited Loading

Choose a reason for hiding this comment

cbiffle commented May 7, 2024

cbiffle commented Sep 18, 2024

aapoalas left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

mkeeter commented Sep 19, 2024

cbiffle commented Sep 19, 2024

Choose a reason for hiding this comment

Choose a reason for hiding this comment

cbiffle commented Sep 20, 2024

cbiffle commented Sep 20, 2024

Choose a reason for hiding this comment

cbiffle commented May 6, 2024 •

edited

Loading

aapoalas May 7, 2024 •

edited

Loading