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

silence warning about cast #482

Merged
merged 1 commit into from
Apr 28, 2024
Merged

Conversation

Freax13
Copy link
Member

@Freax13 Freax13 commented Apr 25, 2024

The lint exists to warn about incompatibilities with different architectures, but this code only runs on x86-64 anyway, so portability is not a concern.

This fixes a nightly breakage.

@Freax13 Freax13 requested review from phil-opp and josephlr April 25, 2024 17:40
Copy link
Member

@phil-opp phil-opp left a comment

Choose a reason for hiding this comment

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

Thanks a lot! As mentioned inline, I think it would be good to add a target_pointer_width check. Otherwise this seems ready to be merged!

src/structures/idt.rs Outdated Show resolved Hide resolved
platform: [ubuntu-latest, macos-latest, windows-latest]
platform: [ubuntu-latest, macos-12, windows-latest]
Copy link
Member

Choose a reason for hiding this comment

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

I guess we should also decide how to deal with compilations for ARM-based systems. Throwing lots of errors in inline asm is not that nice. I guess we have the following options:

  • Add a top-level compile_error when building for non-x86 systems.
  • Less drastic: Invoke a compile_error when the instructions or abi_x86_interrupt features are enabled on non-x86 systems.
  • Use conditional compilation to disable all functions that use inline asm or core::arch::x86_64 on non-x86 architectures.
    • Either on a per-function base, or by making the instructions feature a no-op on non-x86_64 systems.

I'm in favor of option 2 or 3 because I think that parts of this crate could still be useful on ARM-based systems. For example, you could use the VirtAdd::try_new function in a build system to verify that a configured address is valid on the target system.

(It's probably best to do this in a follow-up PR as it's not directly related to this change. I just wanted to start the discussion.)

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we should also decide how to deal with compilations for ARM-based systems. Throwing lots of errors in inline asm is not that nice. I guess we have the following options:

  • Add a top-level compile_error when building for non-x86 systems.

  • Less drastic: Invoke a compile_error when the instructions or abi_x86_interrupt features are enabled on non-x86 systems.

  • Use conditional compilation to disable all functions that use inline asm or core::arch::x86_64 on non-x86 architectures.

    • Either on a per-function base, or by making the instructions feature a no-op on non-x86_64 systems.

I'm in favor of option 2 or 3 because I think that parts of this crate could still be useful on ARM-based systems. For example, you could use the VirtAdd::try_new function in a build system to verify that a configured address is valid on the target system.

Option 1 would be a breaking change. Option 2 and 3 are not mutually exclusive: If we only add a compiler_error!, but don't disable the asm! code, the user will still be confronted with all the error messages our uses of asm! will cause. That said, it's likely the error generated by compiler_error! is one of the (if not the) first error messages shown to the user.

(It's probably best to do this in a follow-up PR as it's not directly related to this change. I just wanted to start the discussion.)

Agreed.

Copy link
Member Author

Choose a reason for hiding this comment

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

I guess we should also decide how to deal with compilations for ARM-based systems. Throwing lots of errors in inline asm is not that nice. I guess we have the following options:

  • Add a top-level compile_error when building for non-x86 systems.

  • Less drastic: Invoke a compile_error when the instructions or abi_x86_interrupt features are enabled on non-x86 systems.

  • Use conditional compilation to disable all functions that use inline asm or core::arch::x86_64 on non-x86 architectures.

    • Either on a per-function base, or by making the instructions feature a no-op on non-x86_64 systems.

I'm in favor of option 2 or 3 because I think that parts of this crate could still be useful on ARM-based systems. For example, you could use the VirtAdd::try_new function in a build system to verify that a configured address is valid on the target system.

Option 1 would be a breaking change.

Our CI even tests that ARM builds work:

- name: "Build on non x86_64 platforms"
run: |
cargo build --target i686-unknown-linux-gnu --no-default-features --features nightly
cargo build --target thumbv7em-none-eabihf --no-default-features --features nightly

Copy link
Member

Choose a reason for hiding this comment

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

If we only add a compiler_error!, but don't disable the asm! code, the user will still be confronted with all the error messages our uses of asm! will cause.

Good point!

@phil-opp
Copy link
Member

I just removed the Test (macos-latest) CI job from the list of required checks to make this PR mergable. We should make the macos-12 required after merging.

@Freax13 Freax13 requested a review from phil-opp April 26, 2024 18:56
@phil-opp phil-opp merged commit de3188d into rust-osdev:master Apr 28, 2024
11 of 12 checks passed
@phil-opp
Copy link
Member

I opened a follow-up PR to implement the needed cfg changes: #483

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