[BUG] Odamex doesn't build on FreeBSD

[Bill-Paul]

Describe the bug
I've tried compiling both Odamex 10.2.0 and 10.3.0 on FreeBSD/amd64 13.1-RELEASE using the released source tarballs, but neither one builds out of the box. In the end I was able to coerce it into building, but not without having to make some code changes. Note that FreeBSD 13.1 uses LLVM/Clang 13.0 as its native compiler and that's what I tried to use. I think these problems are not actually FreeBSD-specific, but just cases of Clang being a bit more picky (though in some cases I'd say it's justified). Note that several of them indicate that the Odamex code is not 64-bit clean (or at least not as clean as it could be).

Clang also issues many warnings, most of which seem harmless, but they're for things that probably should be fixed. (I'll include a build log to show you what I mean.)

The problems I ran into are the same in both the 10.2.0 and 10.3.0 releases. I was able to build 10.3.0 using the same patches that I made for 10.2.0.

I'll try to list the most salient issues here:

1. Configuration. I decided to use cmake to build. FreeBSD 13.1 includes cmake 3.24. I used this command to configure:

% mkdir build
% cd build
% cmake	\
	-DCMAKE_BUILD_TYPE=Release				\
	-DCMAKE_C_COMPILER_ID=Clang				\
	-DCMAKE_CXX_COMPILER_ID=Clang				\
	-DCMAKE_CXX_FLAGS="-isystem /usr/local/include"		\
	-DCMAKE_CXX_LINK_FLAGS=-L/usr/local/lib			\
	-DUSE_INTERNAL_PNG=True					\
	--install-prefix=/usr/local/odamex ..

The reason for using the -isystem command has to do with the Google protobufs library. Odamex includes its own version of protobufs in the source tarball, however depending on what packages you have installed, FreeBSD may already have the Google protobufs library and header files present under /usr/local. Unfortunately they're different versions, and they conflict rather badly. Using the -isystem flag here ensures that the Odamex build favors the header files included with the Odamex source instead of the ones packaged with FreeBSD.

The -L/usr/local/lib flag is needed so that the link command can find libX11. It's in /usr/local/lib on FreeBSD, unlike Linux which has it in /usr/lib.

This is not necessarily something that needs to be fixed, but I'm writing it down here anyway in case some other lunatic besides me wants to try building on FreeBSD too.

2. Compiler errors. Most of the errors (except for one) have to do with Clang complaining about casts between integers and pointers or type mismatches between 32-bit and 64-bit types. I'm including a tarball with diffs from 10.2.0 that show the changes I made to address them.

The most egregious case, and probably the main reason I'm reporting this, is p_acs.cpp.

Here's the thing: Doom has this enum called mobjtype_t. This is actually something from the original vanilla Doom code, though in the Odamex code it's been extended with many additional values. It defines most of the types of "things" that Doom manipulates (the player, monsters, weapons, etc...). Now, again, this is an enum. However in p_acs.cpp, there are numerous places where data of this type are... cast to pointers.

For example, there is this:

static DoomEntity DoomWeaponNames[9] = {
    {"Fist", (mobjtype_t)NULL}, // Default weapon, no entity
    {"Pistol", (mobjtype_t)NULL}, {"Shotgun", MT_SHOTGUN},
    {"Chaingun", MT_CHAINGUN},    {"RocketLauncher", MT_MISC27},
    {"PlasmaRifle", MT_MISC28},   {"BFG9000", MT_MISC25},
    {"Chainsaw", MT_MISC26},      {"SuperShotgun", MT_SUPERSHOTGUN}};

and this:

mobjtype_t FindDoomEntity(const char* type, DoomEntity list[], int size)
{
        int i;
        for (i = 0; i < size; i++)
        {
                if (strcmp(list[i].Name, type) == 0)
                {
                        return list[i].Type;
                }
        }

        return (mobjtype_t)NULL;
}

and this:

        //const TypeInfo* info = TypeInfo::FindType(name);
        mobjtype_t info;

        {
                // Find an entity through cycles
                info = FindDoomEntity(typestr, DoomMonsterNames, NUMMONSTERS);

                if (info == (mobjtype_t)NULL)
                        info = FindWeaponEntity(typestr);
                if (info == (mobjtype_t)NULL)
                        info = FindDoomEntity(typestr, DoomAmmoNames, 8);
                if (info == (mobjtype_t)NULL)
                        info = FindDoomEntity(typestr, DoomKeyNames, 6);
                if (info == (mobjtype_t)NULL)
                        info = FindDoomEntity(typestr, DoomPowerNames, 6);
                if (info == (mobjtype_t)NULL)
                        info = FindDoomEntity(typestr, DoomHealthArmorNames, 9); // Ch0wW
                if (info == (mobjtype_t)NULL)
                        info = FindDoomEntity(typestr, DoomDecorationNames, 60); // Ch0wW
        }

Clang really doesn't like this, and honestly I can't say that I blame it. Look, I know we were all taught that enums in C are really just ints behind the scenes, but it's still not really a great idea to typecast them like this. Be aware that when building on a 64-bit system, a pointer (even a NULL pointer) is 64-bits wide, while an int is still only 32-bit wide. You don't want to twist the compiler's arm to try to make it squeeze a 64-bit peg into a 32-bit hole.

And in this case it doesn't even seem to be necessary, because on closer inspection I found that someone actually added an MT_NULL value to the mobjtype_t definition. So, why doesn't FindDoomEntity() just return MT_NULL instead of playing typecasting games?

The other issues I found were:

In client/src/m_menu.cpp:M_Responder(), there are variables ch and ch2 which are ints that are being compared to NULL:

                if (messageRoutine)
                {
                        if (ch == NULL && ch2 != NULL)
                                messageRoutine(ch2);
                        else
                                messageRoutine(ch);
                }

This should probably be NUL instead of NULL, but I also found that in some places ch is compared to -1. For the time being, I just used 0 instead of NULL, but this may not be right. Someone who actually knows what the code is supposed to do should confirm.

In common/m_wdlstats.cpp, the 'id' members of the WDLPlayer, WDLPlayerSpawn and WDLItemSpawn structures should be unsigned long instead of int. The code does this:

        WDLPlayer wdlplayer = {
            ::wdlplayers.size() + 1,
            player->id,
            player->userinfo.netname,
            player->userinfo.team,
        };

Here, wdlplayers.size() is an unsigned long, which doesn't fit into an int.

Also, there is this code:

        // Add the event to the log.
        WDLEvent evt = {WDL_EVENT_SPAWNITEM, NULL,     NULL,        ::gametic, {ax, ay, az},
                        {0, 0, 0},           itemtype, itemspawnid, 0,         0};
        ::wdlevents.push_back(evt);

Notice the two NULL values used to initialize the 'activator' and 'target' fields. Now, the WDLEvent structure is defined like this:

struct WDLEvent
{
        WDLEvents ev;
        int activator;
        int target;
        int gametic;
        fixed_t apos[3];
        fixed_t tpos[3];
        int arg0;
        int arg1;
        int arg2;
        int arg3;
};

And yet, in m_wdlevents.h, we have this:

void M_LogWDLEvent(
        WDLEvents event, player_t* activator, player_t* target,
        int arg0, int arg1, int arg2, int arg3
);
void M_LogActorWDLEvent(
        WDLEvents event, AActor* activator, AActor* target,
        int arg0, int arg1, int arg2, int arg3
);

Notice that in WDLEvent, 'activator' and 'target' are ints, but these functions seem to expect them to be pointers.

Again, there is the problem of potentially stuffing a 64-bit pointer value into a place where only a 32-bit int should go.

In common/stringtable.cpp, there is this:

void StringTable::prepareIndexes()
{
        // All of the default strings have index numbers that represent their
        // position in the now-removed enumeration.  This function simply sets
        // them all up.
        for (size_t i = 0; i < ARRAY_LENGTH(::stringIndexes); i++)
        {
                OString name = *(::stringIndexes[i]);
                StringHash::iterator it = _stringHash.find(name);
                if (it == _stringHash.end())
                {
                        TableEntry entry = {std::make_pair(false, ""), 0xFF, i};
                        _stringHash.insert(std::make_pair(name, entry));
                }
        }
}

Here a TableEntry is being initialized with an 'index' value of 'i' but 'index' is an int and 'i' is a size_t (which is 64 bits wide for a 64-bit build).

Finally, in common/crc32.cpp, the code relies on __BYTE_ORDER to figure out if the system is big or little endian. The problem is that Clang doesn't define this. Instead it uses BYTE_ORDER, which GCC uses too. I ended up adding this:

/* Clang defines __BYTE_ORDER__ instead of __BYTE_ORDER */

#ifndef __BYTE_ORDER

#if __BYTE_ORDER__ == __ORDER_LITTLE_ENDIAN__
#define __BYTE_ORDER __LITTLE_ENDIAN
#else
#define __BYTE_ORDER __BIG_ENDIAN
#endif

#endif

3. Compiler warnings. Clang emits many warnings when I build. A lot of them seem like trivial things, such as variables declared in functions and then not used as well as variables set and and not used. Others are a more esoteric C++ issues that I'm not quite sure how to address. I've attached a log of my build that shows all of the warnings emitted during a build of 10.3.0. I strongly suggest not letting them slide.

Build that the bug occurred in
10.2.0
10.3.0

To Reproduce

Find a FreeBSD 13.1 system and try to configure and build Odamex as I've described. Alternatively, you might be able to reproduce many of these issues and warnings using Clang on a Linux host.

Expected behavior

Odamex should compile with Clang without my having to tweak it.

Screenshots, NetDemos, & Crash Dumps

You can find my configuration script and patches that address the issues I mentioned above here:

http://people.freebsd.org/~wpaul/odamex

Before anyone asks, no, I am not going to submit a pull request. While I have a lot of experience with C, I'm not a C++ expert, and I'm not sure that all of my fixes are correct. They're good enough to let me get myself running on my FreeBSD system, but they should be triaged and fixed by people who know the code better.

Additional context
Here's my build log showing the warnings:
odamex-log-build.txt