Plugins (gforce): Fix GCC warnings

[kaixiong]

No description provided.

[hartwork]

@kaixiong I would personally do the refactoring and the bugfix in two separate commits but it's alright. Does it fix the full issue already — is anything missing?

[kaixiong]

@hartwork, sorry for the late reply. There's quite a lot more GCC warnings in GForce that are suppressed by the flags -fno-strict-aliasing, -Wno-unknown-warning-option and -Wno-sometimes-uninitialized. This PR is meant to deal with this.

Among the more serious warnings are those related to the type-punning of longs to floats. Presumably GForce was written for 32-bit machines where the long integers are 32-bit. I haven't looked far enough yet to tell why the program doesn't just blow up from this.

[hartwork]

@kaixiong how would you like to continue?

[kaixiong]

@hartwork, I have the complete fix locally using memcpy for type-punning. It's pending a test.

I'm currently blocked by lv-tool crashing out with SIGILL errors (!) when it executes some AVX code generated by ORC. Still troubleshooting.

[kaixiong]

The type punning is now done using memcpy because:

1. It is valid in both standard C and C++. Type-punning via unions is only supported by GCC.
2. Supported on pretty much any version of C and C++.
3. C++20's std::bit_cast cannot be used for casting between types of different sizes (e.g. long and float on LP64 systems).
4. Does not break strict aliasing rules.
5. GCC and Clang optimizes memcpy away into simple word moves. See here for the dissassembly.

This patch also probably fixes memory safety and calculation errors in G-Force when type punning from a type to a larger type on 64-bit platforms i.e.

• float to long - LP64 (Unix)
• float to void* - LP64 (Unix), LLP64 (Windows)

[hartwork]

@kaixiong I found two size-related assumptions here that are neither asserted nor guaranteed to what I can find:

[hartwork]

This implicitly relies on sizeof(float) == 4 and breaks if not, and would seem to break otherwise, at least in theory. I have a hard time finding guarantees for float being 4 bytes in size on all platforms. Is status quo good enough? Should we add a size-related assertion?

[kaixiong]

This is a good point, I've always thought this was guaranteed by the C/C++ standard. (And it looks like we can't even be sure it's IEEE754..). Having some sort of check is desirable, is it better do it here or more globally? I think the assumption is pervasive in the code base. What are your thoughts on this?

[hartwork]

@kaixiong I think I'd favor local changes over global ones because that part of the code base is not in active development.

[hartwork]

Will this work on both big- and little-endian machines if float and long are of different size? My impression is: likely not. Should we use int32_t instead of long (on the inside or both inside and outside)? Should we add a size-related assertion?

[kaixiong]

I forgot about potential issues with endianness. LP64 has 64-bit long integers, so this is a big problem.

Let me study the code a bit more to see how the long integer was stored in the first place.

[kaixiong]

@hartwork so.. I studied the script compiler and VM code and concluded that int32_t is the right type to use.

However, the logic is still a bit funky. If I understand everything correctly, the script bytecode only deals with 32-bit floats, except for the builtin seed function. By this I mean that:

• the compiler parses number literals (with or without decimal points) into floats and encodes them as such.
• its registers and memory cells all store 32-bit floats

This explains the float and float* type punning of the program counter (PC) during decoding.

seed accepts an integer argument which it uses to randomize seeds for a call to srand(). It is the only exception I can find. However, it's not logically correct to type pun a float value (either an immediate value or fetched from a memory cell) into int32_t. Suppose the script contains one of the following lines:

seed(10)   // argument is an immediate value

10 is read by the compiler, encoded as 10.0 in IEEE754 single-precision (0x41200000) and written into the bytecode. When the VM decodes the instruction, it gets back 0x41200000 but reinterprets it as a signed 2-complement integer 1092616192. A similar thing happens when a variable value is the argument instead.

This doesn't really destroy the script's effects because it's just a seed number which gets modded by 31. So ultimately, this interpretation change from float to uint32_t doesn't seem to matter in practice.

Since the original code type puns, I've kept the type pun and changed it from long to int32_t instead.

G-Force seems to be written in 2000, under the assumption that float and long are both 32-bit. This is true on LP32 platforms. While I like the idea of having a size assertion, I think the check needs to be done elsewhere, maybe during plugin initialization.

[hartwork]

While I like the idea of having a size assertion, I think the check needs to be done elsewhere, maybe during plugin initialization.

@kaixiong I'll approve the pull request now… but I'm in favor of local assertions for this rather than a single global one that no future developer will ever find or even look for in practice when they start asking if the very memcpy in front of their eyes is safe.

[kaixiong]

@hartwork Thank you. I understand the concern but I see the local assertion as too ad hoc. The other parts of the script component are a bigger suspect to me for its liberal use of type punning and variable-width integers. Those parts are sensitive to type sizes as well. I suspect there are other things wrong with G-Force and will revisit this again.

[hartwork]

1. Type-punning via unions is only supported by GCC.

@kaixiong PS: Do you have a source on this^^? I was not aware.

[kaixiong]

1. Type-punning via unions is only supported by GCC.

@kaixiong PS: Do you have a source on this^^? I was not aware.

@hartwork This applies to C++ (not C) from what I understand. There is a concept of active member for a C++ union. There is at most one such member at any given time. In type punning, we write data into a member a (the active member), then read it out from another member b before the lifetime of a ends. It is (potentially) undefined behaviour.