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

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

Merged
merged 2 commits into from
Jan 2, 2022

Conversation

mattlgy
Copy link
Contributor

@mattlgy mattlgy commented Dec 28, 2021

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

Fixes #156

@mattlgy
Copy link
Contributor Author

mattlgy commented Dec 28, 2021

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
Copy link
Member

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

@mattlgy
Copy link
Contributor Author

mattlgy commented Dec 28, 2021

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
Copy link
Member

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
Copy link
Contributor Author

mattlgy commented Dec 30, 2021

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
Copy link
Member

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
Copy link
Contributor Author

mattlgy commented Dec 30, 2021

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
Copy link
Member

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
Copy link
Contributor Author

mattlgy commented Dec 30, 2021

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

@mattlgy
Copy link
Contributor Author

mattlgy commented Jan 2, 2022

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
Copy link
Member

Lokathor commented Jan 2, 2022

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

@Lokathor Lokathor merged commit 093146f into rust-console:main Jan 2, 2022
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.

asm! and global_asm! moved to core::arch
2 participants