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

Separating out assembly instructions #177

Closed
h33p opened this issue Sep 7, 2020 · 5 comments
Closed

Separating out assembly instructions #177

h33p opened this issue Sep 7, 2020 · 5 comments

Comments

@h33p
Copy link
Contributor

h33p commented Sep 7, 2020

This is a really great project for x86 internals. We are working on a cross-platform memory introspection toolkit, and the page table structures are what we use to test our implementation against.

Unfortunately, because assembly instructions are tied with x86 target, it is not possible to compile the crate on other architectures, or on win32 with stable toolchain, for that matter. Thus, we had to fork out just the structure part of this crate to provide cross-platform compilation, but it is not a very sustainable solution either.

The question is, would there be interest in separating out all platform-dependent functions (mainly the ones using assembly instructions) under a (default) feature flag? I could prepare a PR, if that's fine.

@phil-opp
Copy link
Member

phil-opp commented Sep 7, 2020

All the assembly instructions should be gated with #[cfg(target_arch = "x86_64")]. Our CI checks that the crate successfully builds on i686-unknown-linux-gnu and I just manually verified that it also builds on armv7a-none-eabi. For building on stable, we have external assembly files as a replacement for inline assembly. You need to disable the default "nightly" feature and enable the "stable" feature for that: cargo build --no-default-features --features stable.

In combination, I see no reason why the crate shouldn't build on win32. Could you try building with the mentioned build command and report the error, if any? Also, the full name of your target would be useful for reproducing the issue.

@h33p
Copy link
Contributor Author

h33p commented Sep 7, 2020

It fails to recognize the assembly file type. cargo:warning=cl : Command line warning D9024 : unrecognized source file type 'src/asm\asm.s', object file assumed. I would assume the assembly format is only valid in the eyes of GCC/Clang compilers. In addition, the asm module itself is not gated behind target_arch, thus stable compilation for arm should break too.

Hence, I thought creating a instructions feature (or some other name) would be a good idea, since then all of the functions, and the assembly module would be gated away, and the feature could be checked in the build script to throw a meaningful error message if built with for a different target architecture.

@phil-opp
Copy link
Member

phil-opp commented Sep 7, 2020

Thanks for the info! I can reproduce the compilation errors and it also seems like the "stable" feature is not supported on Windows anyway:

- name: "Run cargo build for stable"
run: cargo build --no-default-features --features stable
if: runner.os != 'Windows'

After looking through our current feature gates, I agree that an instructions feature (enabled by default) would be useful! I also like the idea to check in the build script that the target architecture is x86_64 when the feature is enabled, and otherwise throw a meaningful error message. So if you have the time, I'd be happy to merge a PR for this.

Some related things that I also worth changing in my opinion:

  • The "stable" feature should be renamed to something like "external_asm" because this describes better what it does. We should also document that the feature is not supported on Windows. Ideally, we could also check this in the build script to throw a meaningful error in this case.
  • The wrfsbase and related functions should be moved to the instructions module too, and only re-exported from the registers module (gated on the new instructions feature).
  • We need to think of some different approach for the array-init crate. Right now, we're using it for the PageTable::new function when the nightly feature is not implemented. However, there is no way to enable an optional dependency when a feature is not activated as far as I know. So I think the best solution for now is to change the feature gate of that method to #[cfg(feature = "array-init")].

I think it might make sense to implement the last two items together with the instructions feature. The first item can be implemented as a follow-up.

@h33p
Copy link
Contributor Author

h33p commented Sep 10, 2020

Okay, thank you for giving me guidance! I will prepare a PR in the next few days

@josephlr
Copy link
Contributor

This was fixed in #179

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

No branches or pull requests

3 participants