JACoders / OpenJK

Community effort to maintain and improve Jedi Academy (SP & MP) + Jedi Outcast (SP only) released by Raven Software
GNU General Public License v2.0
2.01k stars 612 forks source link

Object alignment requirements not being met #833

Closed xycaleth closed 8 years ago

xycaleth commented 8 years ago

What's the problem?

In C++, all types have an alignment requirement which states that an object must be aligned to a byte boundary when ever it is instantiated.

There are some cases in the code where this alignment requirement is not met. On x86, this is usually not a problem as unaligned memory accesses are automatically dealt with by the CPU. However, in some cases, the compiler can take advantage of the memory alignment and generate instructions which makes use of these assumptions, for example, generating SSE instructions for memory aligned to 16 bytes. In these cases, if the object does not adhere to the requirement, then some of its fields may not meet its alignment requirements - for SSE instructions, memory not aligned to 16 bytes will cause a general protection fault (similar to a segmentation fault).

Where is this a problem in the code?

One example is the GetMemory function in codemp/botlib/l_memory.cpp. This is used as generic memory allocation for botlib, but it does not adhere to any alignment requirements as it does not know what type will be stored in the memory. System memory allocators such as malloc get around this problem by returning memory with the "largest fundamental" alignment. That is, the largest alignment any object can have.

A concrete example of where this has gone wrong is on OS X 10.11, with the Apple Clang compiler. The following line in https://github.com/JACoders/OpenJK/blob/master/codemp/botlib/l_precomp.cpp#L3091:

source->defines = NULL;

Is seemingly harmless. However, defines has a 16-byte alignment, and the alignment requirement for source_t is also 16 bytes. With this information, clang is able to generate SSE instructions to set this field to zero:

    0x100106035 <+149>: xorps  %xmm0, %xmm0
->  0x100106038 <+152>: movaps %xmm0, 0x810(%r12)

0x810(%r12) is equivalent to %r12 + 0x810, and is the address of the defines field. %r12 is the base address of source. However, source is allocated from GetMemory and has returned:

r12 = 0x000000010329cc28

The resulting address is 0x10329D438, which is not 16-byte aligned.

How do you propose to fix it?

There could be any number of cases where this is an issue. @dionrhys proposes we fix this specific issue with GetMemory by returning an address aligned to alignof(std::max_aligned_t) - this will give the largest fundamental alignment.

Any other issues of the same nature should be fixed on a case-by-case basis.

mrwonko commented 8 years ago

I love your elaborate descriptions.