Macchina-CLI / libmacchina

A library providing access to all sorts of system information.
https://crates.io/crates/libmacchina
MIT License
68 stars 20 forks source link

fix: Compilation issue in armv7 #117

Closed DemonInTheCloset closed 2 years ago

DemonInTheCloset commented 2 years ago
grtcdr commented 2 years ago

What exactly is the compilation issue?

DemonInTheCloset commented 2 years ago
let disk_size = stats.f_blocks * stats.f_bsize as u64;

On x86_64 stats.f_block is a u64, while on AArch64 it's a u32

DemonInTheCloset commented 2 years ago

Sorry, my mistake, it's not AArch64 but armv7 stable-armv7-unknown-linux-gnueabihf

grtcdr commented 2 years ago

Well, casting in this case is not very optimal, we should perhaps conditionally handle these differences with the cfg! macro.

EDIT: It's not exactly necessary, more of a nitpick really. It'd be interesting to know which alternative is faster and more memory efficient.

I'm just guessing here, but it might be more efficient to use an if cfg!, while the performance difference between the two options would be negligible.

Unit tests would help a lot in this area, but we haven't gotten around to writing them...

DemonInTheCloset commented 2 years ago

A quick godbolt example should help: https://godbolt.org/z/9h4dj3q9W

Casting to u64 in the current impl doesn't affect the asm output at all, while casting to u64 in the armv7 impl adds 3 instructions, I'm not used to armv7 assembly so I can't be sure but casting to u32 seems to allow the operation to be done in 1 instruction instead of 4 (although the 3 extra instructions should be really cheap).

In this case using a cfg directive should help, I'll rework the pull request.

grtcdr commented 2 years ago

Thanks for the detailed explanation.

In this case using a cfg directive should help, I'll rework the pull request.

Great, I'll be watching over this PR.

DemonInTheCloset commented 2 years ago

It seems the libc::statfs internally uses usize and isize (so size_t and ssize_t), and therefore the types change on a 32-bit architecture like armv7.

grtcdr commented 2 years ago

Looks good to me, although I wonder if target_pointer_width could cause any issues in the future, I've never used and wasn't aware of its existence before.

DemonInTheCloset commented 2 years ago

I couldn't find any other cfg attribute which would work (ARM is not the problem but the 32-bit CPU).

It might cause issues when adding another target, which, for some reason, has a strange statfs, but in that case I would argue that it should be treated as the exception, not the rule.

In theory we could also handle 16-bit targets https://doc.rust-lang.org/reference/conditional-compilation.html#target_pointer_width.

grtcdr commented 2 years ago

I couldn't find any other cfg attribute which > would work (ARM is not the problem but the 32-bit CPU).

I see, I have no problem with this approach :)

It might cause issues when adding another > target, which, for some reason, has a strange statfs, but in that case I would argue > that it should be treated as the exception, not the rule.

I fully agree, and thank you for working on this!