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 toolchain to last night's nightly #438

Merged
merged 1 commit into from
Jul 28, 2022
Merged

Conversation

steveklabnik
Copy link
Contributor

@steveklabnik steveklabnik commented Feb 24, 2022

The decision to re-purpose the asm feature flag was changed, which is great. Let's get us on a version of the toolchain that uses the new flags.

This won't be quite ready just yet; I tested using the f4 demo; gonna use one CI run to show me what else breaks so I can fix it up. Now ready!

@@ -50,7 +50,7 @@ path = "../../drv/stm32fx-rcc"
name = "drv-stm32fx-rcc"
features = ["f4"]
priority = 1
requires = {flash = 4096, ram = 1024}
requires = {flash = 8192, ram = 1024}
Copy link
Contributor Author

Choose a reason for hiding this comment

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

This now needs 720 more bytes of text and 884 bytes of rodata. Curse you, powers of two!

(But I haven't determined why yet, I am guessing that it is either simply "new compiler generates different code" or "cortex-m@0.7.3 uses more for some reason". We'll see more once I get the whole build going, I guess.

Copy link
Contributor Author

Choose a reason for hiding this comment

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

See fe85db8 for all of the various size changes.

@@ -85,7 +85,7 @@ path = "../../drv/user-leds"
name = "drv-user-leds"
features = ["stm32h7"]
priority = 2
requires = {flash = 2048, ram = 1024}
Copy link
Collaborator

Choose a reason for hiding this comment

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

This seems very suspicious – the user-leds task should be tiny, so this would be a good starting place to investigate the size changes.

Copy link
Contributor Author

@steveklabnik steveklabnik Feb 24, 2022

Choose a reason for hiding this comment

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

99% of the changes in size are on the order of 100-200 bytes; only one of them was around 1000 bytes.

The issue is that these must be powers of two, and so if you need 2049 bytes of flash, you end up needing to put in 4096, even if almost all of it is wasted space.

Basically all of these changes are "we were just under a power of two and are now just barely over a power of two."

Copy link
Collaborator

Choose a reason for hiding this comment

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

I did some detective work (arm-none-eabi-objdump -d --demangle -t user_leds | grep " F .text" | sort -k 6 | cut -w -f5-) and found that the main change is in core::fmt::Formatter::pad, which went from 690 bytes to 1970 bytes!

Here's the full set of functions that changed size

4c4
< 000002b2      core::fmt::Formatter::pad
---
> 000007b2      core::fmt::Formatter::pad
13d12
< 00000014      drv_user_leds::idl::InOrderUserLedsImpl::closed_recv_fail
15c14
< 00000208      main
---
> 0000020c      main

I found this Rust commit which touched the function recently, but dunno if it's worth digging further...

@steveklabnik
Copy link
Contributor Author

Rebased.

I heard from a fairly reputable source that Rust 2021 supposedly made panics not use the formatting machinery, with the express goal of reducing binary size. However, it's our dependencies, not us, that are the problem here. I changed all of our packages to 2021 and it didn't change anything.

@steveklabnik
Copy link
Contributor Author

@cbiffle during the all hands you had said that you had investigated a bit too, but I forget what you said, and it didn't make it to a comment here. Is this okay to merge?

@steveklabnik steveklabnik force-pushed the bump-toolchain branch 5 times, most recently from f093bcd to c6a6d0f Compare March 17, 2022 17:13
@steveklabnik
Copy link
Contributor Author

Since this still has not landed and it's been a long while, and due to rust-lang/rust#95228, I plan on updating this tomorrow to use the nightly that ^ has landed in, so we can try it out.

@Gankra
Copy link

Gankra commented Mar 30, 2022

yesss... fix your pointer crimes......

@mkeeter
Copy link
Collaborator

mkeeter commented Mar 31, 2022

I prefer to think of them as pointer peccadillos

(mostly because it's funnier sounding)

@steveklabnik
Copy link
Contributor Author

steveklabnik commented Mar 31, 2022

I.... did some work this morning to update this, and went to push it, and got

> git push origin update-toolchain
Enumerating objects: 63, done.
Counting objects: 100% (63/63), done.
Delta compression using up to 32 threads
Compressing objects: 100% (29/29), done.
Writing objects: 100% (33/33), 3.14 KiB | 1.05 MiB/s, done.
Total 33 (delta 24), reused 0 (delta 0), pack-reused 0
remote: Resolving deltas: 100% (24/24), completed with 23 local objects.
remote:
remote: Create a pull request for 'update-toolchain' on GitHub by visiting:
remote:      https://github.com/oxidecomputer/hubris/pull/new/update-toolchain
remote:
To https://github.com/oxidecomputer/hubris
 * [new branch]        update-toolchain -> update-toolchain

... why did I name two branches update-toolchain and bump-toolchain, sigh.

I have to go move some furniture, so this will get updated this afternoon instead of this morning, I was so excited, haha. So much for "wake up a bit early to get going on this soon."

@steveklabnik
Copy link
Contributor Author

I have tortured my poor computer so much today. Sorry, computer.

I completely redid this branch from scratch, with today's nightly. I tested every single app, even those not on CI.

92e126b in particular will end up breaking by tomorrow thanks to upstream; we will have to fix this before landing.

I adjusted every app that had size suggestions except the lpc55 ones because those were not powers of 2, which felt like introducing a lot of churn as code changes land. I kept these in separate commits so that if we don't want them, I can easily take them out of this PR.

git = "https://github.com/oxidecomputer/stm32-rs-nightlies"
branch = "stm32g0b1-initial-support"
default-features = false
stm32g0 = { git = "https://github.com/oxidecomputer/stm32-rs-nightlies", branch = "20220401-snap", default-features = false, optional = true }
Copy link
Contributor Author

Choose a reason for hiding this comment

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

i do prefer the old way of doing this, but the codebase overwhelmingly uses one line, so I've normalized this, since I was here.

@steveklabnik steveklabnik force-pushed the bump-toolchain branch 2 times, most recently from b16a995 to 1575993 Compare April 1, 2022 19:34
@steveklabnik
Copy link
Contributor Author

Okay! This (assuming tests pass) should now be good to go.

@steveklabnik
Copy link
Contributor Author

Unfortunately, this seems to be blocked on rust-lang/rust#90357 😭

@steveklabnik
Copy link
Contributor Author

rust-lang/rust#95685 was merged and so this is now unblocked! It's deeply bitrotted of course, but I'm going to pick this back up now.

@steveklabnik
Copy link
Contributor Author

This has now been updated; it also got a bit smaller!

@cbiffle ready for a re-review whenever :)

@steveklabnik steveklabnik force-pushed the bump-toolchain branch 3 times, most recently from 3b49812 to ddb2441 Compare July 26, 2022 20:47
@@ -1287,6 +1287,7 @@ fn link(
cmd.arg("-m").arg(m);
cmd.arg("-z").arg("common-page-size=0x20");
cmd.arg("-z").arg("max-page-size=0x20");
cmd.arg("-rustc-lld-flavor=ld");
Copy link
Contributor Author

Choose a reason for hiding this comment

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

FWIW: I don't know why I need to make this change. In theory this is supposed to be detected, but isn't now? The docs on this are effectively non-existent...

Copy link
Contributor

Choose a reason for hiding this comment

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

A brief recap about what led to needing to provide -rustc-lld-flavor:

Calling ld directly to link is a bit of a pain as you need to deal with all the platform specific logic (e.g. like where to find crt1.o) which you can avoid by invoking it via cc/gcc/clang. Up until recently, you couldn't pass the linker via an absolute path to gcc but instead had to add a search path that would have the ld binary. Hence why $SYSROOT/lib/rustlib/$HOST_TARGET/bin/gcc-ld exists. This is what happens when you pass -Zgcc-ld=lld (soon to be stabilized as -Clink-self-contained=linker -Clinker-flavor=gcc-lld) to rustc to use the bundled rust-lld.

Now, lld decides what flavour to use based on either the name of the binary invoked or via the first argument (i.e., arg[0] or arg[1]). Unfortunately, rust couldn't just ship ld and ld64 as symlinks to rust-lld so the gcc-ld folder just had multiple copies which meant some bloat.

All's fine for Hubris at this point because we just directly invoked $SYSROOT/lib/rustlib/$HOST_TARGET/bin/gcc-ld/ld and so lld operated with the ld flavour.

Around Oct last year to reduce the bloat (3 copies of rust-lld), the binaries under gcc-ld were replaced with a small tool lld-wrapper that would call ../rust-lld with the right flavor. So now the ld and ld64 under bin/gcc-ld/ were lld-wrapper compiled with different cfg flags choosing the flavour to pass to ../rust-lld. Up until this point, I'd expect hubris to still work when invoking this ld.

But this was still a pain, with the duplication and lacked some other support. So in May-ish, it was simplified to a single copy that would correctly place the flavour argument as the first one passed to rust-lld via -rustc-lld-flavor. Although, looks like this did come with some downsides that might mean further changes coming.

Looking at hubris, looks like we're manually invoking the linker here and not through cc/gcc/clang. I would say we should just call rust-lld directly instead of using whatever is in under bin/gcc-ld (really all that is an implementation detail for rustc hence the name of the flag).

Copy link
Collaborator

@cbiffle cbiffle left a comment

Choose a reason for hiding this comment

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

One note below

build/xtask/src/config.rs Outdated Show resolved Hide resolved
build/xtask/src/config.rs Outdated Show resolved Hide resolved
@steveklabnik steveklabnik merged commit 9ab7b11 into master Jul 28, 2022
@steveklabnik steveklabnik deleted the bump-toolchain branch July 28, 2022 17:50
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.

5 participants