rust-console / gba

A crate that helps you make GBA games
https://docs.rs/gba
Apache License 2.0
658 stars 50 forks source link

Using asm and global_asm from core::arch module. #157

Closed mattlgy closed 2 years ago

mattlgy commented 2 years ago

Update to pull asm and global_asm from core::arch now that they live there.

Fixes https://github.com/rust-console/gba/issues/156

mattlgy commented 2 years ago

Hit another issue when trying to compile for the currently nightly:

error: invalid register `r8`: high registers (r8+) cannot be used in Thumb-1 code
   --> .../gba/src/sync/statics.rs:100:59
    |
100 |       out("r2") _, out("r3") _, out("r4") _, out("r5") _, out("r8") _,
    |                                                           ^^^^^^^^^^^

(and similar across that file)

I've just started diving into gba/asm/arm stuff for rust and only vaguely understand the above problem. Lemme know if there is a quick fix I can add to this PR, or I can keep going down that rabbit hole.

Lokathor commented 2 years ago

That's an error regarding how instruction_set and asm interact. it's not fixable within the crate, it's a rustc problem.

mattlgy commented 2 years ago

Dang. With this commit I was able to build the hello world example on nightly-2021-10-01, which is after moving asm into core::arch but still not completely up to date. So at least closer the HEAD.

Lokathor commented 2 years ago

For now I think you should just comment out all the offending functions, and i think they're all called as part of a match expression elsewhere so fix up that too. Basically they're intended to provide semi-atomic copying for large values, and missing out on the larger variations just cuts how big of a value you can use. So, they're in some sense "non-essential".

I know the person that helped implement instruction_set and they might be able to do the PR to rustc to fix this, until then we'll just have some commented out code.

mattlgy commented 2 years ago

I added some unimplementeds instead, for a bit better experience incase anyone was using that fn. I can just comment out the whole fn if that's better.

Seems to compile and build with the current nightly now!

Lokathor commented 2 years ago

The way it is now, every possible code path of the function is a panic it looks like. Since it will panic no matter what, we should just comment out the whole function.

mattlgy commented 2 years ago

Fair. I could move the panic to the top of the function? Granted this might not be a concern in this setting, but I hate just disappearing functions from a lib/api without good errors for users of it.

In any case, I'll comment out the whole thing unless you get back and prefer the panic route.

Lokathor commented 2 years ago

It's a little painful for a function to go away, I agree, but it's still better than having a panic lurking a potentially rare code path.

mattlgy commented 2 years ago

Good point, better to explode at build time than at run time. fn removed and update the transfer fn.

mattlgy commented 2 years ago

Just to follow up, I think I have all your suggestions incorporated and this compiles against nightly. No rust, just wanted to make sure you are not waiting on me.

Lokathor commented 2 years ago

no no no, all my fault, i had meant to get to this earlier today in fact but got caught up in things.