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

update toolchain to nightly-2024-07-30 #322

Merged
merged 48 commits into from
Aug 22, 2024
Merged

update toolchain to nightly-2024-07-30 #322

merged 48 commits into from
Aug 22, 2024

Conversation

spookyvision
Copy link
Contributor

@spookyvision spookyvision commented Aug 10, 2024

this PR changes the toolchain to use nightly-2024-07-30 and fixes platforms/x86_64 dependencies so it uses root's Cargo.toml patch section for maitake crates.

closes #323

obsoletes #306

TODO for reviewers (roughly ordered by assumed importance):

@jspngh
Copy link
Contributor

jspngh commented Aug 11, 2024

Fyi, upgrading to a recent nightly will also break the Allwinner D1 build due to a new check at link-time:

relocation R_RISCV_JAL out of range: 1633230 is not in [-1048576, 1048575]; references 'abort'

The issue is in the start-up assembly of riscv-rt and is probably only noticeable with large binaries (like mnemos) and doesn't get hit with microcontrollers.
The most recent version of riscv-rt should be okay to use (the issue is still present, but only in case of multiple harts).

Upgrading riscv and riscv-rt might be a good idea, but might also be a lot of work,
since I think we also need to upgrade to embedded-hal(-async) v1.0.0.
An alternative is to use a patched version of riscv-rt v0.11.0

@hawkw
Copy link
Contributor

hawkw commented Aug 11, 2024

I'm looking into changing the mnemos-x86_64-bootloader build script so that we no longer use Cargo artifact deps, but I'll probably try to do that in a separate PR that we can rebase this onto.

hawkw added a commit that referenced this pull request Aug 11, 2024
The `mnemos-x86_64-bootloader` image is built by a separate
crate `mnemos-x86_64-bootloader`, which includes a build script that
runs the post-build workflow to take the `bootloader_api`-compatible
kernel binary and turn it into a bootable disk image that links with the
actual bootloader. At present, we use a Cargo artifact dependency to
build the actual kernel and expose it to the bootloader image build
script. This is the ideal solution for this, since it allows the
dependency between the build script and the kernel binary to be
expressed as a normal Cargo dependency.

However, using an artifact dep on a binary that's built for a different
target than the crate with the artifact dep is sadly broken: see
rust-lang/cargo#12358. We've somehow managed to find a way to avoid this
issue in the past, but updating to a recent nightly seems to have
brought it back (see #322). Therefore, this PR changes the build script
to instead shell out to a new `cargo` invocation that builds the kernel.
This is a bit unfortunate, since it means that the build output from
building the actual kernel isn't exposed to the user as nicely, and
there's probably less build caching. However, it works for now.

I've modified the `just build-x86` Just recipe to invoke `cargo check`
for the actual x86_64 kernel binary before building the bootable image,
to help ensure that the cargo build output is made visible to the user.

A nicer long-term solution would be to switch from a build script to
using a cargo `xtask`-like builder, that's invoked as a cargo `runner`
for that target, with the path to the builder binary passed in on the
CLI. This is what [mycelium does][1]. That approach is nicer because it
allows the kernel to be built using a normal cargo invocation that's
actually visible to the user, instead of secretly in a build script.

[1]: https://github.com/hawkw/mycelium/blob/e51eb8aa98e7609490fa674f408db32fd51caa70/.cargo/config.toml#L1-L2
@spookyvision spookyvision marked this pull request as ready for review August 13, 2024 02:59
@spookyvision spookyvision marked this pull request as draft August 13, 2024 03:02
@spookyvision
Copy link
Contributor Author

Fyi, upgrading to a recent nightly will also break the Allwinner D1 build due to a new check at link-time

that's good to know, thank you! Though at least on my system I can build, flash and run as-is (Lichee RV). Any idea why it doesn't break?

@spookyvision spookyvision marked this pull request as ready for review August 13, 2024 15:41
Copy link
Contributor

@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 looks good to me! I left a few comments, mostly about the lint fixes.

.github/workflows/ci.yml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Cargo.toml Outdated Show resolved Hide resolved
Comment on lines 97 to 100
After flashing, you should be able to see the hello server outputting `hello\n`
repeatedly on the UART port (which is not port used for FEL!).
Note that mnemOS changes the baud rate to 115200.

Copy link
Contributor

Choose a reason for hiding this comment

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

This seems good to mention, but I might also suggest using crowtty to see kernel traces (and it selects the correct baud rate by default).

@@ -94,11 +94,20 @@ xfel write 0x40000000 platforms/allwinner-d1/boards/target/riscv64imac-unknown-n
xfel exec 0x40000000
```

After flashing, you should be able to see the hello server outputting `hello\n`
repeatedly on the UART port (which is not port used for FEL!).
Copy link
Contributor

Choose a reason for hiding this comment

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

tiny nit:

Suggested change
repeatedly on the UART port (which is not port used for FEL!).
repeatedly on the UART port (which is not the port used for FEL!).

Copy link
Contributor Author

Choose a reason for hiding this comment

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

sorry for the mess, I had changed the text multiple times and maybe forgot to push my latest version…
currently it reads:

After flashing you can connect using just crowtty serial <UART-DEVICE>
(this is not the FEL device!). Alternatively any serial terminal will do - note
that we use 115200 baud unlike the builtin bootloader which is at 9600.

platforms/x86_64/core/src/interrupt.rs Outdated Show resolved Hide resolved
source/abi/src/bbqueue_ipc/bbbuffer.rs Outdated Show resolved Hide resolved
Comment on lines -774 to +781
#[cfg(feature = "thumbv6")]
#[cfg(not(target_has_atomic))]
Copy link
Contributor

Choose a reason for hiding this comment

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

perhaps we should just be using the portable-atomic crate here --- cc @jamesmunns ?

but, we can do that in a separate PR. AFAICT none of this matters since we aren't currently compiling MnemOS for thumbv6m or any other target without atomics...

source/spitebuf/src/lib.rs Outdated Show resolved Hide resolved
@jspngh
Copy link
Contributor

jspngh commented Aug 13, 2024

Fyi, upgrading to a recent nightly will also break the Allwinner D1 build due to a new check at link-time

that's good to know, thank you! Though at least on my system I can build, flash and run as-is (Lichee RV). Any idea why it doesn't break?

Interesting 😅
I'm running cargo build -p mnemos-d1 --bin mq-pro (same for lichee-rv) on your update-toolchain branch using the latest nightly, and I do run into the issue I described.

If you haven't done a cargo clean recently, could you give that a try?

@spookyvision
Copy link
Contributor Author

Interesting 😅 I'm running cargo build -p mnemos-d1 --bin mq-pro (same for lichee-rv) on your update-toolchain branch using the latest nightly, and I do run into the issue I described.
If you haven't done a cargo clean recently, could you give that a try?

ah, with your build command I do get the error you described. Good news though, this one works for me:
just build-d1 mq-pro

the difference seems to be --profile release; here's the fully expanded command:

❯ just build-d1 mq-pro
       Found cargo-objcopy
cargo build --profile release --package mnemos-d1 --bin mq-pro

(side note - are you using the latest nightly or the one specified by the project root's rust-toolchain.toml?)

@jspngh
Copy link
Contributor

jspngh commented Aug 14, 2024

ah, with your build command I do get the error you described. Good news though, this one works for me: just build-d1 mq-pro

the difference seems to be --profile release; here's the fully expanded command:

❯ just build-d1 mq-pro
       Found cargo-objcopy
cargo build --profile release --package mnemos-d1 --bin mq-pro

Just tried in release mode and indeed that works.
I wonder if it's due to the binary being smaller because of optimization or the abort symbol being placed at a different location. In any case, I think we can postpone upgrading riscv(-rt) to a different PR.

(side note - are you using the latest nightly or the one specified by the project root's rust-toolchain.toml?)

I tend to use the "default" nightly toolchain instead of a specific version from rust-toolchain.toml files (shame on me)
and that was the latest nightly at the time of writing.

@spookyvision spookyvision merged commit 8a1ccea into main Aug 22, 2024
10 checks passed
@spookyvision spookyvision deleted the update-toolchain branch August 22, 2024 14:47
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.

AFIT has been stabilized
3 participants