Skip to content

Teeny Tiny patch#1

Merged
ReaQwQ merged 5 commits into
ManaOS-dev:masterfrom
Nipfswd:Yep/Patch-1
Jul 3, 2026
Merged

Teeny Tiny patch#1
ReaQwQ merged 5 commits into
ManaOS-dev:masterfrom
Nipfswd:Yep/Patch-1

Conversation

@Nipfswd

@Nipfswd Nipfswd commented Jul 1, 2026

Copy link
Copy Markdown
Contributor

No description provided.

@ReaQwQ ReaQwQ left a comment

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Thanks for the patch! This is a helpful developer-experience cleanup, especially adding rust-toolchain.toml and making run.sh handle target setup more explicitly.

I found one lint issue that needs to be fixed before merging, plus two small cleanup/consistency notes. Nothing here looks conceptually wrong; the main issue is that Rust recently changed the CPUID intrinsic safety contract for x86_64/i686-style targets, so the added unsafe block now trips this repo's warning-deny lint.

Validation I ran locally:

  • cargo fmt --check: passed
  • cargo check --target x86_64-unknown-uefi: passed, with an unused_unsafe warning
  • cargo clippy --all-targets --all-features -- -D warnings: failed on the unused unsafe block
  • just lint: failed at the same unused unsafe block
  • just architecture-boundaries: passed
  • userland clippy for x86_64-unknown-none: passed
  • bash -n run.sh: passed

One small consistency request: could we also mirror the new target setup in run.bat so Windows users get the same setup path as run.sh?

The top-level kernel build runs build.rs, which builds userland demos for x86_64-unknown-none, so Windows users can still hit the manual “install this target” failure path even though run.sh now handles it. Keeping x86_64-unknown-none in rust-toolchain.toml looks right; this is mainly about keeping run.sh and run.bat consistent.

Comment thread src/arch/x86_64/cpu.rs Outdated
pub fn has_apic() -> bool {
let cpuid = core::arch::x86_64::__cpuid(1);
// SAFETY, CPUID leaf 1 is available on all AMD64 CPUs.
let cpuid = unsafe { core::arch::x86_64::__cpuid(1) };

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Could you remove this unsafe block?

I see why it was added, since __cpuid used to be an unsafe intrinsic. Recent stdarch changed that for x86_64/i686-style targets, though, so with the toolchain selected by this PR, core::arch::x86_64::__cpuid(1) is safe and this currently fails clippy as unused_unsafe.

Removing the unsafe { ... } wrapper and the safety comment should fix the lint failure.

Reference: rust-lang/stdarch#1935

Comment thread .gitignore Outdated

# Files
GEMINI.md
nul

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Tiny cleanup: this line seems to include a hidden private-use character after nul (EF 80 8D in UTF-8). Could you replace it with plain nul, or remove the line if it was accidental?

While touching this file, adding a final newline would also keep the diff cleaner.

Comment thread rust-toolchain.toml
@@ -0,0 +1,7 @@
[toolchain]
channel = "nightly"

Copy link
Copy Markdown
Collaborator

Choose a reason for hiding this comment

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

Question/nit: since channel = "nightly" isn't date-pinned, this toolchain could
silently shift over time — today's unused_unsafe situation with __cpuid is
actually a good example of how a nightly-only safety-contract change can flip a
build from passing to failing between two people's local runs (or between CI runs
on different days).

Would it make sense to pin this to a dated nightly (e.g. nightly-2026-06-15) for
reproducibility? Not blocking this PR, just flagging for discussion.

Nipfswd and others added 2 commits July 2, 2026 16:18
@ReaQwQ ReaQwQ merged commit adca1bc into ManaOS-dev:master Jul 3, 2026
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.

2 participants