Skip to content

Add big endian support #771

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.

Sign up for GitHub

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

Open
nopjne wants to merge 1 commit into
base: master
Choose a base branch
from
Open

Add big endian support #771

Show file tree
Hide file tree
Changes from all commits
Commits
Show all changes
1 commit
Select commit
File filter

Filter by extension

Filter by extension
Conversations
Failed to load comments.
Loading
Jump to
Jump to file
Failed to load files.
Loading
Diff view
Diff view
23 changes: 23 additions & 0 deletions src/formats/internal/memreader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -62,14 +62,22 @@ uint8_t memread_ubyte(memreader *reader) {

uint16_t memread_uword(memreader *reader) {
uint16_t r;
#ifdef BIG_ENDIAN_BUILD
r = __builtin_bswap16(*((uint16_t *)(reader->buf + reader->pos)));
#else
memcpy(&r, reader->buf + reader->pos, sizeof(r));
#endif
reader->pos += sizeof(r);
return r;
Comment on lines 64 to 71
Copy link
Member

@Nopey Nopey Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

buf+pos is not insufficiently aligned to be a (uint16_t *)

please use the memcpy from the existing code, and keep your #ifdef block as small as possible (your sd_peek_dword is a good example of how it should look).

Copy link
Author

@nopjne nopjne Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I've thought about this, and it hasn't been an issue with the current game assets. I don't want to slow it down more than needed.

Copy link
Member

@Nopey Nopey Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

on a platform with "free" unaligned reads (x86, N64?), memcpy'ing the four bytes should compile to equivalent assembly as the hazardous uint16 reads (which are innocuous on x86(/n64?) without ubsan).

Dereferencing an unaligned pointer is undefined behavior, and traps on more-or-less common architectures (the ARM family, which even supported big endian back in armv5).

Have you observed a pessimization when using memcpy in these functions, or a big difference in assembly? I would expect GCC to be very good at optimizing a four byte memcpy.

}

uint32_t memread_udword(memreader *reader) {
uint32_t r;
#ifdef BIG_ENDIAN_BUILD
r = __builtin_bswap32(*((uint32_t *)(reader->buf + reader->pos)));
#else
memcpy(&r, reader->buf + reader->pos, sizeof(r));
#endif
reader->pos += sizeof(r);
return r;
}
Expand All @@ -83,22 +91,37 @@ int8_t memread_byte(memreader *reader) {

int16_t memread_word(memreader *reader) {
int16_t r;
#ifdef BIG_ENDIAN_BUILD
r = __builtin_bswap16(*((uint16_t *)(reader->buf + reader->pos)));
#else
memcpy(&r, reader->buf + reader->pos, sizeof(r));
#endif
reader->pos += sizeof(r);
return r;
}

int32_t memread_dword(memreader *reader) {
int32_t r;
#ifdef BIG_ENDIAN_BUILD
r = __builtin_bswap32(*((int32_t *)(reader->buf + reader->pos)));
#else
memcpy(&r, reader->buf + reader->pos, sizeof(r));
#endif
reader->pos += sizeof(r);
return r;
}

float memread_float(memreader *reader) {
float r;
#ifdef BIG_ENDIAN_BUILD
uint32_t fl;
memcpy(&fl, reader->buf + reader->pos, sizeof(fl));
fl = __builtin_bswap32(fl);
reader->pos += sizeof(fl);
#else
memcpy(&r, reader->buf + reader->pos, sizeof(r));
reader->pos += sizeof(r);
#endif
return r;
}
Comment on lines 114 to 126
Copy link
Member

@Nopey Nopey Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

I think memread_float (and similar: memwrite_float, sd_read_float, etc..)
should be implemented in terms of their respective integer read-write functions.

for memread_float, this would look like:

   uint32_t u = memread_dword(reader);
   float f;
   memcpy(&f, &u, 4);
   return f;

this way, they don't need to perform any byte swapping and can be focused on just the uint-to-float bit cast.

Copy link
Author

@nopjne nopjne Nov 18, 2024 •
edited
Loading

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Oh,, I see your point.

Copy link
Member

@Nopey Nopey Nov 19, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

memread_dword performs a byteswap.

memread_float is currently only used to read some tournament mode pilot variables we ignore, unsure what you mean by tested and working


Expand Down
32 changes: 31 additions & 1 deletion src/formats/internal/memwriter.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -7,7 +7,6 @@
#include "formats/internal/memwriter.h"
#include "formats/internal/writer.h"
#include "utils/allocator.h"

#define GROW 64

#define CHECK_SIZE \
Expand Down Expand Up @@ -57,27 +56,58 @@ void memwrite_ubyte(memwriter *writer, uint8_t value) {
}

void memwrite_uword(memwriter *writer, uint16_t value) {
#ifdef BIG_ENDIAN_BUILD
uint16_t t = value;
t = __builtin_bswap16(t);
memwrite_buf(writer, (char *)&t, 2);
#else
memwrite_buf(writer, (char *)&value, 2);
#endif
}

void memwrite_udword(memwriter *writer, uint32_t value) {
#ifdef BIG_ENDIAN_BUILD
uint32_t t = value;
t = __builtin_bswap32(t);
memwrite_buf(writer, (char *)&t, 4);
#else
memwrite_buf(writer, (char *)&value, 4);
#endif
}

void memwrite_byte(memwriter *writer, int8_t value) {
memwrite_buf(writer, (char *)&value, 1);
}

void memwrite_word(memwriter *writer, int16_t value) {
#ifdef BIG_ENDIAN_BUILD
int16_t t = value;
t = __builtin_bswap16(t);
memwrite_buf(writer, (char *)&t, 2);
#else
memwrite_buf(writer, (char *)&value, 2);
#endif
}

void memwrite_dword(memwriter *writer, int32_t value) {
#ifdef BIG_ENDIAN_BUILD
int32_t t = value;
t = __builtin_bswap32(t);
memwrite_buf(writer, (char *)&t, 4);
#else
memwrite_buf(writer, (char *)&value, 4);
#endif
}

void memwrite_float(memwriter *writer, float value) {
#ifdef BIG_ENDIAN_BUILD
uint32_t t;
memcpy(&t, &value, 4);
t = __builtin_bswap32(t);
memwrite_buf(writer, (char *)&t, 4);
#else
memwrite_buf(writer, (char *)&value, sizeof(float));
#endif
}

void memwrite_fill(memwriter *writer, char content, int len) {
Expand Down
53 changes: 53 additions & 0 deletions src/formats/internal/reader.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -110,12 +110,18 @@ uint8_t sd_read_ubyte(sd_reader *reader) {
uint16_t sd_read_uword(sd_reader *reader) {
uint16_t d = 0;
sd_read_buf(reader, (char *)&d, 2);
#ifdef BIG_ENDIAN_BUILD
d = __builtin_bswap16(d);
#endif
return d;
}

uint32_t sd_read_udword(sd_reader *reader) {
uint32_t d = 0;
sd_read_buf(reader, (char *)&d, 4);
#ifdef BIG_ENDIAN_BUILD
d = __builtin_bswap32(d);
#endif
return d;
}

Expand All @@ -128,18 +134,31 @@ int8_t sd_read_byte(sd_reader *reader) {
int16_t sd_read_word(sd_reader *reader) {
int16_t d = 0;
sd_read_buf(reader, (char *)&d, 2);
#ifdef BIG_ENDIAN_BUILD
d = __builtin_bswap16(d);
#endif
return d;
}

int32_t sd_read_dword(sd_reader *reader) {
int32_t d = 0;
sd_read_buf(reader, (char *)&d, 4);
#ifdef BIG_ENDIAN_BUILD
d = __builtin_bswap32(d);
#endif
return d;
}

float sd_read_float(sd_reader *reader) {
float f = 0;
#ifdef BIG_ENDIAN_BUILD
uint32_t r = 0;
sd_read_buf(reader, (char *)&r, 4);
r = __builtin_bswap32(r);
memcpy(&f, &r, sizeof(f));
#else
sd_read_buf(reader, (char *)&f, 4);
#endif
return f;
}

Expand All @@ -152,12 +171,18 @@ uint8_t sd_peek_ubyte(sd_reader *reader) {
uint16_t sd_peek_uword(sd_reader *reader) {
uint16_t d = 0;
sd_peek_buf(reader, (char *)&d, 2);
#ifdef BIG_ENDIAN_BUILD
d = __builtin_bswap16(d);
#endif
return d;
}

uint32_t sd_peek_udword(sd_reader *reader) {
uint32_t d = 0;
sd_peek_buf(reader, (char *)&d, 4);
#ifdef BIG_ENDIAN_BUILD
d = __builtin_bswap32(d);
#endif
return d;
}

Expand All @@ -170,18 +195,27 @@ int8_t sd_peek_byte(sd_reader *reader) {
int16_t sd_peek_word(sd_reader *reader) {
int16_t d = 0;
sd_peek_buf(reader, (char *)&d, 2);
#ifdef BIG_ENDIAN_BUILD
d = __builtin_bswap16(d);
#endif
return d;
}

int32_t sd_peek_dword(sd_reader *reader) {
int32_t d = 0;
sd_peek_buf(reader, (char *)&d, 4);
#ifdef BIG_ENDIAN_BUILD
d = __builtin_bswap32(d);
#endif
return d;
}

float sd_peek_float(sd_reader *reader) {
float f = 0;
sd_peek_buf(reader, (char *)&f, 4);
#ifdef BIG_ENDIAN_BUILD
f = __builtin_bswap16(f);
#endif
Comment on lines 214 to +218
Copy link
Member

@Nopey Nopey Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

__builtin_bswap16 does not take an argument of type float, it takes a uint16_t.
The __builtin_bswap16(f) call is equivalent to __builtin_bswap16((uint16_t)f), which is why the N64 build was encountering floating point errors.

Also: bswap16 is the wrong number of bits, should be bswap32.

(same as other review comment: please implement sd_peek_float in terms of sd_peek_dword)

Copy link
Author

@nopjne nopjne Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Well, the exceptions were definitely from doing float f = __buildin_bswap32((float)x);
The only reason this function is wrong is because nothing in the engine calls it, and I just didn't hit the issue.

return f;
}

Expand Down Expand Up @@ -213,6 +247,16 @@ int sd_read_line(const sd_reader *reader, char *buffer, int maxlen) {
if(fgets(buffer, maxlen, reader->handle) == NULL) {
return 1;
}
#ifdef BIG_ENDIAN_BUILD
int len = maxlen;
for(int i = 0; i < len / 4; i += 1) {
((int *)buffer)[i] = __builtin_bswap32(((int *)buffer)[i]);
Copy link
Member

@Nopey Nopey Nov 18, 2024

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

char *buffer is insufficiently aligned to be a int*
same comment applies in sd_read_str.

please memcpy to a uint32_t local variable (and uint16_t local var for the final-2-bytes bytes-swap)

}

if((len & 3) == 2) {
((uint16_t *)buffer)[(len / 2) - 1] = __builtin_bswap16(((uint16_t *)buffer)[(len / 2) - 1]);
}
#endif
return 0;
}

Expand All @@ -231,6 +275,15 @@ void sd_read_str(sd_reader *r, str *dst) {
if(len > 0) {
char *buf = omf_calloc(1, len + 1);
sd_read_buf(r, buf, len);
#ifdef BIG_ENDIAN_BUILD
for(int i = 0; i < len / 4; i += 1) {
((int *)buf)[i] = __builtin_bswap32(((int *)buf)[i]);
}

if((len & 3) == 2) {
((uint16_t *)buf)[(len / 2) - 1] = __builtin_bswap16(((uint16_t *)buf)[(len / 2) - 1]);
}
#endif
str_from_c(dst, buf);
omf_free(buf);
} else {
Expand Down
20 changes: 19 additions & 1 deletion src/formats/internal/writer.c
View file
Open in desktop
Original file line number Diff line number Diff line change
Expand Up @@ -15,7 +15,6 @@ struct sd_writer {

sd_writer *sd_writer_open(const char *file) {
sd_writer *writer = omf_calloc(1, sizeof(sd_writer));

writer->handle = fopen(file, "wb");
writer->sd_errno = 0;
if(!writer->handle) {
Expand Down Expand Up @@ -76,10 +75,16 @@ void sd_write_ubyte(sd_writer *writer, uint8_t data) {
}

void sd_write_uword(sd_writer *writer, uint16_t data) {
#ifdef BIG_ENDIAN_BUILD
data = __builtin_bswap16(data);
#endif
sd_write_buf(writer, (char *)&data, 2);
}

void sd_write_udword(sd_writer *writer, uint32_t data) {
#ifdef BIG_ENDIAN_BUILD
data = __builtin_bswap32(data);
#endif
sd_write_buf(writer, (char *)&data, 4);
}

Expand All @@ -88,15 +93,28 @@ void sd_write_byte(sd_writer *writer, int8_t data) {
}

void sd_write_word(sd_writer *writer, int16_t data) {
#ifdef BIG_ENDIAN_BUILD
data = __builtin_bswap16(data);
#endif
sd_write_buf(writer, (char *)&data, 2);
}

void sd_write_dword(sd_writer *writer, int32_t data) {
#ifdef BIG_ENDIAN_BUILD
data = __builtin_bswap32(data);
#endif
sd_write_buf(writer, (char *)&data, 4);
}

void sd_write_float(sd_writer *writer, float data) {
#ifdef BIG_ENDIAN_BUILD
uint32_t fl;
memcpy(&fl, &data, sizeof(fl));
fl = __builtin_bswap32(fl);
sd_write_buf(writer, (char *)&fl, sizeof(fl));
#else
sd_write_buf(writer, (char *)&data, sizeof(float));
#endif
}

void sd_write_fill(sd_writer *writer, char content, size_t len) {
Expand Down