← Back to index

Possible Bug in "Brain" Code

eteran
#8697
I believe that I have spotted a bug in the code. I'm not a paying member who can report on the github issues page, so I figured I'd post here (excuse any typos, copying from youtube stream by hand).

Here's what I see in the code:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
struct brain_hero
{
    entity *Head;
    entity *Body;
    entity *Glove;
};

// ... only need one to point out the issue that I think I see ...

struct brain
{
    brain_id ID;
    brain_type Type;

    union
    {
        entity *Array;
        brain_hero Hero;
        brain_monstar Monstar;
        brain_familiar Familiar;
        brain_snake Snake;
    };
};


So here's the issue that I think I see (it's hard to describe without a whiteboard, but will do my best):

"brain_hero" is memory layout compatible with "entity *Segments[3]" because it is 3 "entity *" elements adjacent in memory, but is not memory layout compatible with "entity *Array".

To demonstrate, let's assume that we are really looking at "brain_hero" brain, but are accessing via the "Array" member.

Basically if you use Array[1], because you've used a union to type pun, it will basically be doing this: "((entity *)Hero.Head)[1]". Which is of course, not the same as Hero.Body, which is what would would have expected. Compilers can't really warn about this because the union says "this is what i want". And I suspect that any usage of Array (if any?) may have worked by being lucky.

Fortunately the fix is trivial. Simply change the definition to be like this instead:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
struct brain
{
    brain_id ID;
    brain_type Type;

    union
    {
        entity *Array[1];
        brain_hero Hero;
        brain_monstar Monstar;
        brain_familiar Familiar;
        brain_snake Snake;
    };
};


This will be memory layout compatible with the other types in the union. And accessing Array indexes beyond 1 is fine in practice (for the same reasons that the infamous "struct hack" works).
eteran
#8698
As a proof of concept, I wrote this little test program:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
#include <stdio.h>

typedef int brain_id;
enum brain_type { Hero_ID };

struct entity {
	int x, y;
};

struct brain_hero
{
    entity *Head;
    entity *Body;
    entity *Glove;
};

struct brain
{
    brain_id ID;
    brain_type Type;

    union
    {
        entity *Array;
        brain_hero Hero;
    };
};


int main() {
	brain b;
	printf("%p ==? %p\n", &b.Array[1], &b.Hero.Body);
}


which outputs:

1
0x8 ==? 0x7fff0615d940


However, my "fixed" version seems to work correctly:

 1
 2
 3
 4
 5
 6
 7
 8
 9
10
11
12
13
14
15
16
17
18
19
20
21
22
23
24
25
26
27
28
29
30
31
32
33
#include <stdio.h>

typedef int brain_id;
enum brain_type { Hero_ID };

struct entity {
	int x, y;
};

struct brain_hero
{
    entity *Head;
    entity *Body;
    entity *Glove;
};

struct brain
{
    brain_id ID;
    brain_type Type;

    union
    {
        entity *Array[1];
        brain_hero Hero;
    };
};


int main() {
	brain b;
	printf("%p ==? %p\n", &b.Array[1], &b.Hero.Body);
}


Which outputs:

1
0x7ffc5c38bf70 ==? 0x7ffc5c38bf70
Edited by eteran on
Casey Muratori
#8699
I'm not sure what bug you're reporting here, though? Unless I'm mis-remembering, we do not use Array[] as the way to access these, we just use the pointer at the location in memory.

- Casey
eteran
#8700
Well, any deference of the Array member will result in garbage data. If you aren't actually doing that (yet), then this won't result in any negative behavior of the code.

So, if you are just using it as a sentinel to find the start of the union, I suppose that this is less a bug in of itself, and more of a "potential bug" if the you do ever choose to access the entities by array index through this member. Of course, the question then becomes:

Why make Array of type "entity *"? Any usage of it beyond taking its address will probably crash. Making it "entity* Array[1]" will serve the same purpose, but not have the downside of being able to be misused if this nuance is forgotten down the line.

Regardless, thanks for the cool series. It's been interesting to follow :-)
eteran
#8701
Ironically, looking back at episode #288, it used to be defined as "entity *Array[16]". Which is effectively equivalent to my suggestion. I forget which episode the [16] was dropped.
Casey Muratori
#8702
Yeah I don't really know why it's defined the way it's defined. We use a utility function for accessing things, so it doesn't actually ever get used outside of that - it's just there as a marker.

So yes, this is not a bug.

- Casey
Reply to Thread
← Back to index