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

64 bit big endian issues #426

Open
KungFuJesus opened this issue Sep 20, 2020 · 5 comments · May be fixed by #580
Open

64 bit big endian issues #426

KungFuJesus opened this issue Sep 20, 2020 · 5 comments · May be fixed by #580
Labels
bug Something isn't working

Comments

@KungFuJesus
Copy link

KungFuJesus commented Sep 20, 2020

Describe the bug
On 64 bit platforms (compiling with GCC), there seems to be an aliasing issue within a union for the displaylist struct. The union in question is DynUnion. 2/3 of the union members are 64 bit types on 64 bit platforms (all pointers), but the third is a 32 bit signed integer. Somewhere, the compiler's optimizer or perhaps the code itself is taking a liberty with this size, only copying the first half of the word into the address. On little endian architectures, this is not a big deal, on big endian it means that you get the wrong half of the integer and when processing the display list, instead of getting a parameter for the alpha value, you get an invalid enum (0). This is a show stopping bug, as the game won't start, and gd_printf fails at this point as the entire GL context fails to draw the framebuffer after this (you only get to the title screen before the goddard intro). I had to patch the gd_printf() function to tell me where the code was actually hanging up by doing an addition vprintf() with the va_list.

Fixing this bug was just a matter of promoting the type in that union to a signed 64 bit integer.

Additionally I had to patch the makefile to not sure -march, as for some reason powerpc GCC still varies for these flags (probably dates back to Apple's contributions, but that's not a difficult thing to address).

To Reproduce
Steps to reproduce the behavior:
1.) Compile on powerpc64 (big endian) linux
2.) Launch game

Expected behavior
The game to launch

Desktop (please complete the following information):

  • OS: Gentoo ppc64, using nouveau graphics

Additional context
There are still some endianness bugs hiding somewhere in this code. Some of this is due to endianness bugs directly in the nouveau stack (there's a byteswapping texture bug still there that I haven't found the root cause of yet). But, unlike with other games, GL_UNPACK_SWAP_BYTES=1 does not invert the issue, indicating that there's likely some other endianness bugs hiding in gfx_pc.c with regard to the type interpretation (somewhat funny, as the PNG textures are natively big endian, as were most of the assets originally in the game).

Here's the patch to make the game actually launch on powerpc64 linux:

diff --git a/src/goddard/gd_types.h b/src/goddard/gd_types.h
index 0857cc0..547923e 100644
--- a/src/goddard/gd_types.h
+++ b/src/goddard/gd_types.h
@@ -27,11 +27,11 @@ struct GdColour {
 union DynUnion {
     void *ptr;
     char *str;
-    s32 word;
+    s64 word;
 };
@fgsfdsfgs
Copy link
Collaborator

This is interesting. People have gotten this working just fine on 32-bit powerpc platforms, so no one noticed this until now. You should probably also create an issue or PR in the original sm64-port repo as well.

@KungFuJesus
Copy link
Author

Perhaps related (at least, people following that issue will want to know about this): #334

@KungFuJesus
Copy link
Author

It also appears the project this is forked from doesn't have issue tracking, only PRs. While I'd like to submit this PR directly with some justification, it may be nice to have a little bit of dialogue with other developers to see if this behavior has any real drawbacks (I'm pretty sure the struct allocation is going to be a 64 bit type anyway, so I don't think this is inflating memory usage).

@fgsfdsfgs
Copy link
Collaborator

Fortunately that type is most likely only used in goddard (the mario head intro part), so it probably doesn't affect the rest of the game. The much more widely used Gfx struct should already "properly" 64-bit, but who knows what else is in there.

@KungFuJesus
Copy link
Author

KungFuJesus commented Oct 2, 2020

The good news: I've isolated the byte swapped texture colors to be strictly a nouveau endianness bug (printed the RGBA values from little and big endian machines from host memory - they are the same). So this may be the only big endian bug present.

That having been said - a lot of the code assumes little endian and manually swaps bytes from buffers in big endian order (and recombines them via bit shifts and logical &s). I do wonder how much the compiler is successfully ignoring this and how much of the code could benefit from a manual macro to not do this.

Nouveau bug in question:
https://gitlab.freedesktop.org/mesa/mesa/-/issues/1167

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Something isn't working
Projects
None yet
Development

Successfully merging a pull request may close this issue.

3 participants