declare code as C99 compliant, include stdbool.h unconditionally

[fabiangreffrath]

Fixes #1722

[turol]

Explicitly setting the C standard is fine but we shouldn't change the includes or boolean type.

[fabiangreffrath]

Explicitly setting the C standard is fine but we shouldn't change the includes or boolean type.

Third, Chocolate Doom doesn't even start if we keep #typedef bool boolean.

[suve]

What do you mean by "doesn't even start"? Crashes right away?

Including <stdbool.h> adds the #define bool _Bool macro, so keeping typedef bool boolean means you're using the C99 _Bool type.

[fabiangreffrath]

What do you mean by "doesn't even start"? Crashes right away?

It fails with

R_InitSprites: Sprite TROO frame I has rotations and a rot=0 lump

That's here, I guess the first boolean check it approaches at run-time:

#1  0x00005555555ab72e in R_InstallSpriteLump (lump=<optimized out>, frame=<optimized out>, rotation=<optimized out>, flipped=flipped@entry=false) at r_things.c:137
137		I_Error ("R_InitSprites: Sprite %s frame %c has rotations "
(gdb) l
132		return;
133	    }
134		
135	    // the lump is only used for one rotation
136	    if (sprtemp[frame].rotate == false)
137		I_Error ("R_InitSprites: Sprite %s frame %c has rotations "
138			 "and a rot=0 lump", spritename, 'A'+frame);
139			
140	    sprtemp[frame].rotate = true;
141	

Including <stdbool.h> adds the #define bool _Bool macro, so keeping typedef bool boolean means you're using the C99 _Bool type.

Well, that doesn't make it any better, or does it?

[suve]

Ok, so this was fun. After playing around with Godbolt, it seems to me that when performing checks against _Bool variables, GCC generates the following assembly:

• For some_var == false checks, the assembly checks that some_var != 1.
• For some_var == true checks, the assembly checks that some_var != 0.

This might seem a bit strange at first, but the C standard says that a _Bool can only have two values: 0 or 1. Any type conversion from other type into a _Bool must result in either 0 or 1 being assigned to the _Bool value. Given this assumption, x == 0 and x != 1 are equivalent statements.

Now, if you look at the code inside src/doom/r_things.c, you'll notice this little gremlin:

memset (sprtemp,-1, sizeof(sprtemp));

The variable sprtemp is of type spriteframe_t, and the spriteframe_t.rotation field has a type of _Bool. So now you've got a _Bool that holds -1, which is an illegal value - if you apply the rules mentioned above, hilarity ensues:

• some_var == false gets evaluated as -1 != 1, which is true!
• some_var == true gets evaluates as -1 != 0, which is also true!

[fabiangreffrath]

Thank you for the analysis! The problem with Doom code is that it assumes anything non-0 to evaluate to true.

[suve]

Well yeah, the _Bool type works similarly - assigning any non-zero value to it will produce true (or rather, 1). So stuff like bool_var = integer_var; or even bool_var = 42; will work fine. But any memset() call that might modify a _Bool variable, or a union with a _Bool member could potentially introduce this buggy behaviour.

[fabiangreffrath]

See, and this is why I insist on boolean begin either of type int, or enum which translates to int literal.

[turol]

Ah, the joys of undefined behavior. Nasal demons can't be killed with a shotgun.

Please add a comment to the boolean typedef to explain why it must be int.

With C99 we have to be slightly careful since the coding style calls for all variable declarations at the beginning of a block.

@fragglet Do you have an opinion on this? If not, I'll approve this PR after the comment is added.

[fragglet]

Late reply, but I'm very much in favour of this change. C99 is >25 years old now, we shouldn't be wedded to ANSI C forever. Plus the stdbool.h header is something that can be "soft emulated" even on non-C99 compilers if necessary (as with stdint.h, which we already use)

It would be great to see the boolean type replaced with bool entirely as a follow-up. It sounds like that currently causes some problems though. I expect that any blockers to doing it rely on undefined / implementation defined behavior (like the example @suve gives), which ought to be fixed anyway. Or in other words, doing this is a good way to uncover bugs we weren't previously aware of.

[turol]

I added a draft PR for ubsan workflow #1724

It fails as expected but the ubsan output is lost. Is there an easy way to make quickcheck preserve that? After that we just need someone to go and fix all the problems...