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
• uint32 -> uint32_t
• signed int -> int32_t
• sint32 -> int32_t
• unsigned short -> uint16_t
• ushort -> uint16_t
• signed short -> int16_t
• short -> int16_t
• unsigned char -> uint8_t
• ubyte -> uint8_t
• uint8 -> uint8_t
• signed char -> int8_t
• sbyte -> int8_t
• sint8 -> 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 (Already done)

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 me accidentally destroying my own work and then redoing it.

[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.

[bryanperris]

Please re-sync the branch, and then we can see if everything builds. I do like these type namings much better.

[GravisZro]

@bryanperris previous message from @JeodC indicates that updating now would be a moot point. I don't want to do this again only to be told "no" once again.

[JeodC]

@bryanperris previous message from @JeodC indicates that updating now would be a moot point. I don't want to do this again only to be told "no" once again.

That was 21 days ago and they are revisiting it as I said.

[bryanperris]

this is still in draft mode.

[GravisZro]

@JeodC I'll do it again but only if I can get a guarantee that it won't be another wasted effort.

[JeodC]

@JeodC I'll do it again but only if I can get a guarantee that it won't be another wasted effort.

I mean, it looks to me like bperris wants to review it. I'm not in a position to guarantee anything.

[GravisZro]

@bryanperris If I update this will you review it and do what you can to get it merged?

[bryanperris]

I already reviewed it, but I like to have Louis take a look too if that is ok.

[GravisZro]

@bryanperris So long as Louis is willing to do so. Also, this should be merged as soon as the review is done because the conflicts will be... many.

[bryanperris]

yep understood

[GravisZro]

comparing commit a1a8056afc76a544c783e09c3496fa1f4a7492d6 to the current

~/project/Descent3$ ./compare.sh 
diff: ./linux/netcon/lanclient/Release/Direct: No such file or directory
diff: ../builds.new/./linux/netcon/lanclient/Release/Direct: No such file or directory
diff: TCP~IP.so: No such file or directory
diff: ../builds.new/TCP~IP.so: No such file or directory
Files builds/linux/Descent3/Release/Descent3 and builds.new/linux/Descent3/Release/Descent3 are identical
executable checksums:
b0179f43529c085fc0c0578722bf93456ec1a011  builds/linux/Descent3/Release/Descent3
b0179f43529c085fc0c0578722bf93456ec1a011  builds.new/linux/Descent3/Release/Descent3

Success!

Don't forget to modify lib/d3_version.h.in to have static values if you test this too.

[GravisZro]

nothing like destroying your own work with a force push just when you finish. -_-

[GravisZro]

OK... it's done... again.

~/project/Descent3$ ./compare.sh 
diff: ./linux/netcon/lanclient/Release/Direct: No such file or directory
diff: ../builds.new/./linux/netcon/lanclient/Release/Direct: No such file or directory
diff: TCP~IP.so: No such file or directory
diff: ../builds.new/TCP~IP.so: No such file or directory
Files builds/linux/Descent3/Release/Descent3 and builds.new/linux/Descent3/Release/Descent3 are identical
executable checksums:
b0179f43529c085fc0c0578722bf93456ec1a011  builds/linux/Descent3/Release/Descent3
b0179f43529c085fc0c0578722bf93456ec1a011  builds.new/linux/Descent3/Release/Descent3

FML.

[Lgt2x]

Thanks for updating, this seems very reasonable to me. We'll make sure to merge this before anything else to make sure you don't have another painful conflict resolution

[GravisZro]

Thanks for the merge!