abaddon icon indicating copy to clipboard operation
abaddon copied to clipboard

Fix emojis.bin reading on big-endian systems

Open dragontamer8740 opened this issue 1 year ago • 3 comments

This works, but the code itself might or might not be good enough for you the way I implemented it.

It at least works on my 32-bit big-endian PowerBook G4... and I think the preprocessor if's I used should prevent it from interfering with Windows. But I don't know how it will play out on the BSD's, for instance, which may or may not define __BYTE_ORDER__ and __ORDER_BIG_ENDIAN__.

Endianness could be checked at runtime with something like this instead, and avoid a whole lot of headaches.

int e = 1; // Check Endianness
    if ((int)*((unsigned char *)&e) == 1) { // Little Endian
        ......
    }
    else { // Big Endian
        ......
    }

Stealing from https://stackoverflow.com/a/2182184 , here's some relatively portable 32-bit endian-swapping code.

swapped = ((num>>24)&0xff) | // move byte 3 to byte 0
                ((num<<8)&0xff0000) | // move byte 1 to byte 2
                ((num>>8)&0xff00) | // move byte 2 to byte 1
                ((num<<24)&0xff000000); // byte 0 to byte 3

But I thought I'd give you some code and also let you know there was a problem, at the very least. I can confirm that my code makes emojis decode correctly on Big Endian PowerPC Linux.

Note that if anyone else tries this, in order to get it to actually build on PowerPC 32-bit, I had to add the -latomic flag manually to the linking command... that's not because of my code, it was already like that. But just in case someone is trying to do this in the future and stumbles on this, there you go.

dragontamer8740 avatar Aug 11 '22 22:08 dragontamer8740

i would use the portable version so theres no need to rely on the header. i bet the compiler is probably smart enough to produce the same code as __bswap_32. as for detecting endianness you can probably use TestBigEndian in cmake and add a compile definition if it passes. apart from that just change stdio.h to cstdio and put spaces around the = so everything consistent

ouwou avatar Aug 11 '22 22:08 ouwou

I have never done anything in cmake except building others' programs. Not sure how best to handle that.

It only has to be checked one time at run-time if stored globally, though.

As for the rest, might get to that sometime soon. Mostly just wanted to make an issue but also show a proof of concept.

dragontamer8740 avatar Aug 11 '22 22:08 dragontamer8740

something like this should work. then its just an #ifdef

diff --git a/CMakeLists.txt b/CMakeLists.txt
index 44ca621..cc6e4a8 100644
--- a/CMakeLists.txt
+++ b/CMakeLists.txt
@@ -33,6 +33,12 @@ if (WIN32)
     add_compile_definitions(NOMINMAX)
 endif ()
 
+include(TestBigEndian)
+test_big_endian(IS_BIG_ENDIAN)
+if (IS_BIG_ENDIAN)
+    add_compile_definitions(ABADDON_IS_BIG_ENDIAN)
+endif ()
+
 configure_file(${PROJECT_SOURCE_DIR}/src/config.h.in ${PROJECT_BINARY_DIR}/config.h)
 
 file(GLOB_RECURSE ABADDON_SOURCES

ouwou avatar Aug 11 '22 23:08 ouwou

@ouwou Alright, this looking better?

Building this on the powerbook take a VERY long time (on the order of one to two hours), so it's still compiling there. I expect it will work. I actually think that's faster than trying to set up a working cross-compilation environment right now. emojis.cpp compiles successfully, just got to wait for it all to build and link and then I can test it.

BTW, CMake documentation says that TestBigEndian is deprecated in favor of CMAKE_<LANG>_BYTE_ORDER.

dragontamer8740 avatar Aug 13 '22 22:08 dragontamer8740

yeah i used TestBigEndian since the minimum cmake version declared in CMakeLists is <3.20 and i figured it just be nicer to not change it. i think in later versions TestBigEndian uses CMAKE_<LANG>_BYTE_ORDER anyways and try_compile in older versions.

and yeah i know compile time is pretty rough. i think its because nearly everything ends up transiently including the entirety of gtkmm. i think cmake's unity build feature can speed it up a good bit so maybe ill mess with that.

also you might have to re-open since this should probably target master instead of attachments (which is merged now)

ouwou avatar Aug 13 '22 22:08 ouwou

Alright, made a new PR https://github.com/uowuo/abaddon/pull/100 .

dragontamer8740 avatar Aug 13 '22 23:08 dragontamer8740