-
Notifications
You must be signed in to change notification settings - Fork 182
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
Conversation
app/demo-stm32f4-discovery/app.toml
Outdated
@@ -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} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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.
15b30ea
to
667e91a
Compare
app/gimletlet/app-mgmt.toml
Outdated
@@ -85,7 +85,7 @@ path = "../../drv/user-leds" | |||
name = "drv-user-leds" | |||
features = ["stm32h7"] | |||
priority = 2 | |||
requires = {flash = 2048, ram = 1024} |
There was a problem hiding this comment.
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.
There was a problem hiding this comment.
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."
There was a problem hiding this comment.
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...
667e91a
to
df7e5a0
Compare
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. |
@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? |
f093bcd
to
c6a6d0f
Compare
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. |
yesss... fix your pointer crimes...... |
I prefer to think of them as pointer peccadillos (mostly because it's funnier sounding) |
I.... did some work this morning to update this, and went to push it, and got
... why did I name two branches 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." |
c6a6d0f
to
072ec41
Compare
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. |
072ec41
to
67ae577
Compare
drv/stm32xx-gpio-common/Cargo.toml
Outdated
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 } |
There was a problem hiding this comment.
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.
b16a995
to
1575993
Compare
Okay! This (assuming tests pass) should now be good to go. |
Unfortunately, this seems to be blocked on rust-lang/rust#90357 😭 |
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. |
1575993
to
02a2f7e
Compare
This has now been updated; it also got a bit smaller! @cbiffle ready for a re-review whenever :) |
3b49812
to
ddb2441
Compare
@@ -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"); |
There was a problem hiding this comment.
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...
There was a problem hiding this comment.
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).
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
One note below
ddb2441
to
eb6db32
Compare
eb6db32
to
f3c664c
Compare
f3c664c
to
6e828f1
Compare
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!