jasonwhite / syscalls

Raw Linux system calls for Rust.
Other
109 stars 14 forks source link

Support ARMv4T Thumb mode (ARM7TDMI) #47

Closed cottnn closed 7 months ago

cottnn commented 7 months ago

Currently if you try to compile for ARMv4T architecture with Thumb mode enabled, you get errors like this:

error: instruction variant requires ARMv6 or later
  |
note: instantiated into assembly here
 --> <inline asm>:1:2
  |
1 |     mov r2, r7
  |     ^
jasonwhite commented 7 months ago

Do you know which instruction should be used instead of mov on ARMv4T in thumb-mode? Or can you point me to some ARMv4T reference?

Also note that Rust doesn't have great support for thumb mode in inline assembly, so the current implementation is kind of hacky. It isn't currently possible to compile both thumb and non-thumb inline assembly into the same binary as far as I know.

cottnn commented 7 months ago

Sorry for not including more details initially, here is the ARM7TDMI reference (first core to implement ARMv4T ISA): https://documentation-service.arm.com/static/5e8e1323fd977155116a3129 In particular, page 42 contains summary of mov variants in ARMv4T Thumb mode.

Page 58 describes what "low" and "high" registers are:

Registers r0–r7 are known as the low registers. Registers r8–r15 are known as the high registers.

mov variants on page 42 contain no "Low to Low" variant, which is what mov r2, r7 is, I suppose.

I also found this forum post, and while the question is about ARMv6-M, I think it could be related: https://community.arm.com/support-forums/f/architectures-and-processors-forum/45501/regarding-the-documentation-on-the-t1-encoding-of-the-mov-instruction-on-armv6-m-architecture

If [architecture < armv6] and [both registers are low], then generate the [flag-setting encoding for "adds Rd, Rm, #0"]

I asked ChatGPT and it suggested pushing value of e.g. r7 onto the stack, and then popping it to r2 to achieve a move. I'm not familiar with ARM, but I suppose it could work. There's probably a better way though.

cottnn commented 7 months ago

Related Zig issue: https://github.com/ziglang/zig/issues/2738

MOV Rd, Rm In architectures before ARMv6, either Rd or Rm, or both, must be a Hi register. In ARMv6 and above, this restriction does not apply.

PR to fix: https://github.com/ziglang/zig/pull/2750

cottnn commented 7 months ago

@jasonwhite ping, I provided more details above in case you consider adding support

jasonwhite commented 7 months ago

This seems to compile, but I don't really know if it is correct:

diff --git a/src/syscall/arm_thumb.rs b/src/syscall/arm_thumb.rs
index 99ab7cb..f0239a2 100644
--- a/src/syscall/arm_thumb.rs
+++ b/src/syscall/arm_thumb.rs
@@ -194,10 +194,10 @@ pub unsafe fn syscall6(
 ) -> usize {
     let mut ret: usize;
     asm!(
-        "mov {temp}, r7",
-        "mov r7, {n}",
+        "movs {temp}, r7",
+        "movs r7, {n}",
         "svc 0",
-        "mov r7, {temp}",
+        "movs r7, {temp}",
         n = in(reg) n as usize,
         temp = out(reg) _,
         inlateout("r0") arg1 => ret,
@@ -206,7 +206,7 @@ pub unsafe fn syscall6(
         in("r3") arg4,
         in("r4") arg5,
         in("r5") arg6,
-        options(nostack, preserves_flags)
+        options(nostack)
     );
     ret
 }

MOVS updates the condition flags as well, which I think is fine. Thoughts?

cottnn commented 7 months ago

@jasonwhite I'm not very familiar with ARM or assembly in general, but the way Zig implemented this is in this PR: https://github.com/ziglang/zig/pull/2750 They replaced mov with eors instructions, which I think also updates condition flags: https://github.com/ziglang/zig/pull/2750#discussion_r297364877 I'm not sure which approach is better

jasonwhite commented 7 months ago

Yeah, I saw that. I'm not sure why they went with multiple eors instead of one movs, which seems to build just fine for armv4t-*, armv5te-*, and thumbv7neon-* :shrug:.

I don't have an easy way to run the tests in an emulator for this platform, so I'll go ahead and merge my PR and see if anyone complains.

cottnn commented 7 months ago

@jasonwhite I think you only changed syscall6 implementation

jasonwhite commented 7 months ago

@cottnn I believe that's the only syscall that needed to be changed. syscall6 is the only syscall where r0-r7 are all used. I suspect the register allocator couldn't find an appropriate register to use with mov.

cottnn commented 7 months ago

@jasonwhite For some reason I still can't compile on v0.6.17, it always seems to pick r7 for {temp} and {n} which triggers the error. What's interesting is that it also generates mov r7, r7, which is just a NOP. I changed all mov to movs in arm_thumb.rs and it worked fine after that. Could this be a bug in rustc/LLVM somewhere?

$ rustc --version
rustc 1.78.0-nightly (3cbb93223 2024-03-13)

I can make a minimal example of the issue in a repo if that would help.

jasonwhite commented 7 months ago

@cottnn Well, that's weird! I have the same nightly version and I can build it just fine. Here's how I was testing it:

$ cat .cargo/config.toml
[target.armv4t-none-eabi]
rustflags = ["-C", "target-feature=+thumb-mode"]

[unstable]
build-std = ["core"]
$ rustc +nightly --version
rustc 1.78.0-nightly (3cbb93223 2024-03-13)
$ cargo +nightly build --target armv4t-none-eabi --no-default-features

Which target triple are you testing on?

cottnn commented 7 months ago

@jasonwhite I created a repo to reproduce the error: https://github.com/cottnn/armv4t_syscalls

jasonwhite commented 7 months ago

@cottnn Thanks, I can reproduce the issue in your repo. I'll push a fix later today to update all mov to movs.

jasonwhite commented 7 months ago

@cottnn Okay, I think I know why it wasn't failing for me. Removing #[inline] also reproduces the problem. I suspect codegen just wasn't happening for these functions and so the error couldn't manifest.


As you pointed out, the assembly for syscall6 is:

movs    r6, r7
movs    r7, r7
svc #0
movs    r7, r6

It's weird, but I think this is fine. r6 is chosen as {temp} and r7 already had the syscall number in it so it moves into itself. It's a noop for our purposes, but it's not technically a noop because it has the side effect of changing flags. Otherwise, I think it'd get optimized out.


All movs have now been converted to movss as of c7bf159ad947ec7fffc53f6c13707b75a2a11233.