Fix the various misuage of size_t

[Kenzzer]

Currently working on a PR for Sourcemod that aims at enabling /WX on msvc, this means dealing with the various narrowing or extending of variables, when compiling the project on 64 bits.

The warnings are mostly size_t to unsigned int conversion, which are no longer valid.

[dvander]

Appreciate the effort in trying to cut these down, but... this is a lot of churn, not obviously fixing any bugs. A few things concern me:

• The lexer is unable to produce tokens after reaching INT_MAX bytes in the combined source stream. This prevents almost every data structure in the compiler from overflowing int, which is why we're so fast and loose with int types in the compiler.
• The VM is a different story, but I see some changes in here that don't like right. Eg, replacing int with long in binary search (it should be size_t, and long should never be used in cross-platform code).
• I see stuff like "uint64_t = uint32_t" getting changed to "uint32_t = uint32_t". If MSVC is complaining about assigning a 32-bit unsigned value to a 64-bit unsigned value, that is a bad warning. And the fix could introduce a real bug. Auditing this stuff is really difficult.

Here's my advice, in order of preference:

(1) Disable the /W option that is producing these spam warnings.
(2) Disable /WX.
(3) Limit (1) or (2) to the compiler, and fix everything else. It will still take time to audit.
(4) Moonshot - ditch msvc. Industry uses clang these days, even for Windows builds, maybe it just works?

[Kenzzer]

Since my goal is to keep the warnings active and eventually enable /WX for Sourcemod. I'd like to avoid that solution.

I kept size_t around, since I assumed it was the desired final type. But in light of your comment over the lexer, I assume it woudn't be wrong to transform (most of ) the size_t instances to uint32_t then ?

I cannot poke further at this for now, but as soon as I'm able I'll update this PR.

[dvander]

It's a poor tradeoff to potentially introduce subtle bugs to fix problems that don't exist. Disabling the warnings or changing compilers is your best bet.

I wouldn't bother with the lexer. I'd entertain a look at the vm/ fixes but it'll take time to carefully audit each change.

[Kenzzer]

I'm having troubles understanding how we could introduce potential bug by limiting the range of all the size_t variables, especially when they're already cast in some places of the current codebase to uint32_t.

As size_t is an unsigned 32bits int on 32 bits, any bugs that would already exist are intrinsic to the codebase rather than the variable type. Restricting size_t to unsigned 32bits int on 64 bits shouldn't introduce any bugs, we have no chance of overflowing the variable since all the instances that would be corrected by this PR are containers that have no reason to output a number above 4294967295

I'm very willing to see this through and input as much effort as you'd expect from me to ensure your worries are quelled. I don't know the codebase so I'm sure you're justified in your concerns. My intention isn't to give a false sense of fixing bugs with this PR, that has never been the goal.

I'd like to satisfy the compilers so we stop using warning supressors. I do believe its overkill to enable a global warning surpression flag, because we've a dozen of lines in the sourcepawn/amtl submodule that would make the compiler yell. All the while those warnings could have been useful into other sections of the larger sourcemod project, which could have potentially indicated bugs to the code contribution's author.

So if we don't address the issue by converting all unsigned int instances to size_t, or with the would be new PR size_t to uint32_t. Then all that's left for me to do would be :

• 1 - Edit the AMBuilder file, so that the /WX /W3 flags are removed specifically for the sourcepawn submodule. Which can work fine, however it will make the script a little messier to read through. That also means the extensions of sourcemod that might include some sourcefiles from sourcepawn (like dhooks) will also have to edit their AMBuilder file to do the very same thing in regard to stripping some compiler flags. And now we've looped back to the initial issue, which is that sourcepawn submodule will dictate compiler flags for sections of larger project, and yes that does sound like I'm inventing the issue here. Since it technically is dhook's fault in the first place to have included files from sourcepawn into its compilation, but I still think there's merit to pondering this aspect. It however remains the best compromise so far.

• 2 - Add pragma in the files to supress those warnings. This would solve the issue, but then comes down to preferences. Is it really a good idea to have those warning supressors in the code, and maybe forget they're even there ?

• 3 - Like you suggested, supress the warnings altogether and never look back. But then this PR (or its would be successor) lose all meaning, and didn't need to exist in the first place and it means I've been exhausting your time needlessly.

I suppose I'm going to roll with the 1st solution, and edit the AMBuilder file to add/strip compiler flags so sourcepawn compiles fine. And meanwhile sourcemod will be able to benefit from extended warnings.

[dvander]

"I'm having troubles understanding how we could introduce potential bug by limiting the range of all the size_t variables"

It could cause overflows, eg:

    size_t GetSize();
    size_t SomethingElse() { return GetSize() * 4; }

Changing GetSize() to uint32_t could introduce an overflow on x64. Sure, it could already overflow on x86, but generally on x86 allocations will fail long before you start overflowing size_t, which is not true on x64.

The change in the binary search code to use long has similar problems. On both x86 it could cause a signed overflow, and yes that already exists, but that also means nothing is being fixed, so it's just churn. On Windows x64, long is 32-bit, so it could cause a legit overflow.

My point is this stuff is hard to audit, it takes time, and the changes can be invasive. If there are actual bugs here, let's fix them properly. You're the boss of the compiler, not vice versa :)

[Kenzzer]

You're the boss of the compiler, not vice versa :)

I can absolutely agree with that. Despite closing this PR, I definitively want to come back to this at some point. It'd be nice not to surpress warnings, and if that means I've to rewrite some sections of the codebase to ensure overflows can't happen instead of convenient c-cast then I will. I'll apply this mindset to sourcemod and amtl first though, since they're the two projects I'd really like to remove warning surpressors from.