FOV memory issue
Posted: Mon Aug 12, 2013 12:21 pm
Hey!
I am (as of not long ago) the package maintainer of tome4 in AUR.
I know the language C quite well, and I have noticed a bug in fov/fov.c.
It caused my compilation of tome4 to crash every time as the game was attempting to render the background of the main menu.
Putting a watch on global_shape showed that this variable was written to inside LARGE_ASS_FOV_DEFINE_OCTANT, and further investigations lead me to line 599 and GET_BUFFER:
This bit of code is for allocating small buffers at high intervals (I guess), where buffer_data is the preallocated pool of memory.
Problem is that if buffer_data->index after line 580 ends up near the end of the buffer in such a way that the requested chunk of memory can't fit within the pool's boundaries it will usually cause a segmentation fault (or the infamous general protection fault from XP days).
We need a check here to make sure that the requested chunk is placed correctly within the pool (buffer_data); here is my suggestion:
Unfortunately we must also remove the 'hurray', because I cannot see how we may avoid branching here 
Luckily most of the time the if will fail, and modern CPU's exploit this and are able to preload assembly as if the if will always fail. See http://en.wikipedia.org/wiki/Branch_predictor.
The bottom line is that this branch should have absolutely no impact on performance
I have fixed this in the AUR package with a homebrew patch, so please tell me if you implement this fix and I must update the package
BR Thomas
I am (as of not long ago) the package maintainer of tome4 in AUR.
I know the language C quite well, and I have noticed a bug in fov/fov.c.
It caused my compilation of tome4 to crash every time as the game was attempting to render the background of the main menu.
Putting a watch on global_shape showed that this variable was written to inside LARGE_ASS_FOV_DEFINE_OCTANT, and further investigations lead me to line 599 and GET_BUFFER:
Code: Select all
578 #define GET_BUFFER(target, buffer_data, len) \
579 /* hurray, no branching, modulus, or malloc! */ \
580 buffer_data->index = (buffer_data->index + buffer_data->prev_len) & (FOV_BUFFER_SIZE - 1); \ \
581 buffer_data->prev_len = len; \
582 target = buffer_data->buffer + buffer_data->index;
Problem is that if buffer_data->index after line 580 ends up near the end of the buffer in such a way that the requested chunk of memory can't fit within the pool's boundaries it will usually cause a segmentation fault (or the infamous general protection fault from XP days).
We need a check here to make sure that the requested chunk is placed correctly within the pool (buffer_data); here is my suggestion:
Code: Select all
578 #define GET_BUFFER(target, buffer_data, len) \
579 /* hurray, no branching, modulus, or malloc! */ \
580 buffer_data->index = (buffer_data->index + buffer_data->prev_len) & (FOV_BUFFER_SIZE - 1); \
581 if (buffer_data->index+len >= FOV_BUFFER_SIZE) { \
582 buffer_data->index = 0; \
583 } \
584 buffer_data->prev_len = len; \
585 target = buffer_data->buffer + buffer_data->index;
Luckily most of the time the if will fail, and modern CPU's exploit this and are able to preload assembly as if the if will always fail. See http://en.wikipedia.org/wiki/Branch_predictor.
The bottom line is that this branch should have absolutely no impact on performance
I have fixed this in the AUR package with a homebrew patch, so please tell me if you implement this fix and I must update the package
BR Thomas