fix Mod_LoadMarksurfaces MAPJAMX and check sign

[lndpj]

Changes

• fix [MAPJAMX][Release/gcc] Segmentation fault: in Mod_LoadModel.part.0 () with --march=native #529
• warn don't quit if marksurface number is out out of range and set it to INT32_MAX
• make MAX_SANITY_LIGHTMAPS an integer constant and fix signed compare
• output the surface number in case of an error
• use int32_t instead of int because it allows 16-bit length otherwise
• use #pragma pack(push,1) / #pragma pack(pop) for gl_model.h structs

[andrey-budko]

I think this is because of unaligned access. If you download MAPJAMX from the in-game Mods menu (rather than from moddb.com), this breakpoint triggers when loading the start map:

short *in = (short *)(mod_base + l->fileofs);
if ((uintptr_t)in & 1)
    __debugbreak();

l->fileofs is 5079415 for maps/start.bsp, so in is unaligned (not sure why this is an issue on x86_64, though).

I just checked vkQuake - it uses ReadShortUnaligned workaround at this exact location:
Novum/vkQuake@c1b0234

[lndpj]

The error is not related to alignment. The odd address causes unaligned, slower read and doesn't have an effect outside of the read function that returns the value as an aligned (un)signed integer value. The endian swap access is bytewise and needs a

#pragma pack(push,1)
typedef union type_u
{
  int16_t word;
  struct { int8_t lo; int8_t hi; };
} type;
#pragma pack(pop)
type v = { .word = 324 };
static_assert(((v.word & 0x00ff) << 8) | ((v.word & 0xff00) >> 8) == (int16_t)SDL_SwapLE16((*(uint16_t*)&v.word));

The integer types range and sign causes the problem.
The expected value is an unsigned integer of 16-bit length converted to an 32-bit signed integer.
Using the l->fileofs as a value and printing it as a signed and unsigned integer of 16/32-bit value you get:

 int32_t a = 5079415;
uint32_t b = 5079415;
 int16_t c =  -32393;
uint16_t d =   33143;
static_assert((int)c >= 0 && (int)c <= INT16_MAX && (((int)(uint16_t)c <= UINT16_MAX) == d));

If the surface number value at an offset is equal to the above offset value and the result is not cast to uint16_t, the negative short return value gets promoted to int while the range check doesn't assert >= 0, triggering an out of bound access if used as an array index.

The architectures address width is irrelevant and has no influence on the problem.

See: Marksurfaces Technical Details

[andrei-drexler]

I haven't been able to repro the crash with -march=native on a Ryzen 7 7800x3d, but admittedly that is a very different CPU.

not sure why this is an issue on x86_64, though

@andrey-budko My hunch would be that this is a vectorization issue, with a scalar prologue only processing at most a fixed number of values (assuming natural 2-byte alignment) before proceeding to a SIMD loop using aligned loads, which fails due to the misaligned address. That would be consistent with the nanosleep workaround, which would also prevent loop vectorization. The int32_t casts, which should be no-ops, probably also trip up the vectorization for some reason, but this is all speculation on my part, I'd need to see the disassembly. I'm kind of surprised that the bounds checks wouldn't prevent vectorization to begin with, though.

If the surface number value at an offset is equal to the above offset value and the result is not cast to uint16_t, the negative short return value gets promoted to int while the range check doesn't assert >= 0, triggering an out of bound access if used as an array index.

@lndpj that explanation almost makes sense, except it doesn't. The existing code already casts the value to unsigned short, which is equivalent to uint16_t. All the values will be >= 0 after being converted to a 32-bit int (either plain int or int32_t, both are the same on x86_64). The code in the PR might avoid the codegen issue on your system, but not for the reasons you've given.

I've been trying to give you the benefit of the doubt and assume you're meaning well, but let me be very clear: this isn't helping, on the contrary. I'm spending time digging through AI slop instead of doing actual work, and there's plenty of work to be done and never enough time. It would be much more helpful if you actually tried to truly understand the issue and come up with a solution yourself instead of offloading all the work to an AI model.