Refactor to fixed width integer types: proven to be free of errors

[GravisZro]

Pull Request Type

• GitHub Workflow changes
• Documentation or Wiki changes
• Build and Dependency changes
• Runtime changes
  • Render changes
  • Audio changes
  • Input changes
  • Network changes
  • Other changes

Description

Changes non-specific types (e.g. unsigned int) to their stdint specified counterparts (e.g. uint32_t).

• unsigned int -> uint32_t
• uint -> uint32_t
• signed int -> int32_t
• unsigned short -> uint16_t
• ushort -> uint16_t
• signed short -> int16_t
• short -> int16_t
• unsigned char -> uint8_t
• ubyte -> uint8_t
• signed char -> int8_t
• sbyte -> int8_t

I avoided modifying anything that could have the the "long" keyword as part of it because of 64-bit compatibility issues that have been fully addressed. See also: #233

Yes, this is is a massive refactor and looking at every line is a waste of time but I have proven that no errors were introduced in the process. I did this by comparing the object code and executables generated before and after the patches were applied. By ensuring the binary files were identical, it's a certainty that no errors were introduced.

However, to make a build deterministic, you must define D3_GIT_HASH as something constant and it must be a Release build. This is how I built each one.

echo "#undef D3_GIT_HASH" >> lib/d3_version.h.in
echo "#define D3_GIT_HASH \"\"" >> lib/d3_version.h.in
cmake --preset linux
cmake --build --preset linux --config Release

here's the script, to compare object code, libraries and binaries files.

#!/bin/sh

REPODIR=$PWD

BUILD0="builds"
BUILD1="builds.original"

cd $BUILD0

for file in $(find -type f | grep -E '\.(o|so|a)$')
do
  diff $file ../$BUILD1/$file
done

cd $REPODIR

diff -s $BUILD0/linux/Descent3/Release/Descent3 $BUILD1/linux/Descent3/Release/Descent3
echo executable checksums:
shasum $BUILD0/linux/Descent3/Release/Descent3
shasum $BUILD1/linux/Descent3/Release/Descent3

output:

$ ./compare_builds.sh 
Files builds/linux/Descent3/Release/Descent3 and builds.original/linux/Descent3/Release/Descent3 are identical
executable checksums:
d2c10d31c6e87e16121d2aac6cfa20dca0adb552  builds/linux/Descent3/Release/Descent3
d2c10d31c6e87e16121d2aac6cfa20dca0adb552  builds.original/linux/Descent3/Release/Descent3

Checklist

• I have tested my changes locally and verified that they work as intended.
• I have documented any new or modified functionality.
• I have reviewed the changes to ensure they do not introduce any unnecessary complexity or duplicate code.
• I understand that by submitting this pull request, I am agreeing to license my contributions under the project's license.

Additional Comments

• Great care was taken to avoid modifying historical comments.
• Matching was done case specifically and to "whole words" in order ensure no variable names were altered.
• This should be merged before conflicts arise.
• Forced push updates are merely rebasing the branch on main

[Jayman2000]

I think this is a great PR. I strongly prefer types like int_32_t over types like int. I tested this on NixOS 23.11, and I was indeed able to produce identical object files and executable files.

However, to make a build deterministic, you must define D3_GIT_HASH as something constant and it must be a Release build. This is how I built each one.

echo "#undef D3_GIT_HASH" >> lib/d3_version.h.in
echo "#define D3_GIT_HASH \"\"" >> lib/d3_version.h.in
cmake --preset linux
cmake --build --preset linux --config Release

Instead of modifying lib/d3_version.h.in, I used nix-shell --pure in order to make sure that git was not on my PATH. It would be nice if we could run cmake -D GIT_HASH="whatever", but that’s a topic for another pull request.

---

I have a few questions/suggestions:

• I don’t think that “standard types” is the right term here. I know that the header is called “cstdint”, but I don’t think that “standard” is a good way of describing these types, especially because they seem to be less common than char, short, int, etc. I would rename this PR to “Refactor to fixed-width types: proven to be free of errors”.
• Does your script prove that the generated .so files are identical?
• I have a few very minor suggestions for your script:
  • It looks like the indentation for your for loop got messed up.
  • I would replace $(find -type f | grep -E "\.o$") with $(find -type f -name '*.o').
  • Why did you decide to calculate hashes for the executables instead of using diff like you did for the .o files?

[GravisZro]

@Jayman2000

• I've updated the title to reflect the proper terminology.
• I modified the script to ensure .so, .a, .o, and the final binary are diffed.
• The checksum is merely there to make it clear that they are in fact the same file and that the script isn't somehow goofing.

At this point, updating the scripting any further is code golf because find is obscenely powerful. This was purpose made script but there is an argument to be made for having the option of building fully deterministic builds.

[JeodC]

Thanks, but this will not be reviewed for a while. We are currently working on importing icculus' diffs from the 2020 linux and mac ports of this game and then bringing windows up to feature parity--as a result, we're avoiding large-scale code changes for now to help prevent conflicts. Other refactors are not on the docket at the moment. I understand this somewhat changes the "1.5 stable release" milestone--this will be expanded on in the coming weekly update.

If you would like, keep this as a draft. Otherwise, we can revisit later.

[GravisZro]

@JeodC

Thanks, but this will not be reviewed for a while.

The entire point of proving that it builds identical binaries as before the changes was to ensure that no manual review was required.