Sanitizer fixes and plProduct static init fix

H-uru / Plasma

Cyan Worlds's Plasma game engine

http://h-uru.github.io/Plasma/

GNU General Public License v3.0

202 stars 80 forks source link

Sanitizer fixes and plProduct static init fix #1585

Closed dpogue closed 1 month ago

dpogue commented 2 months ago

plDetectorLog was initializing before plProduct had initialized its static strings, leading to some weird stuff where the User Data Folder path was missing "Uru Live"
Was running with AddressSanitizer to try to track down some GL Pipeline issues, and ended up tracking down a bunch of other issues and warnings first.
- It is (apparently) undefined behaviour to cast non-memory-aligned values to int types, which we were doing in a few spots (and are still doing in a bunch more spots). This fixes the easy ones in plUUID and hsStream. Also, memcpy is a built-in now so there should be no speed benefit to doing casting.
- fRegisteredForTime (and a few other values) were never being zero initialized in plTransitionMgr
- plBufferedFileReader's fBuffer is created with new[] but was being released with delete

colincornaby commented 2 months ago

Thanks - the address sanitizer stuff was bugging me for a while. Hopefully this is the same issues I was seeing.

dpogue commented 2 months ago

I have dropped the change to plDetectorLog and instead made more of the plProduct strings use std::string_view.

Hoikas commented 2 months ago

Looks like we have a test failing on macOS.

dpogue commented 2 months ago

Looks like we have a test failing on macOS.

The pnUUID test was failing on the string comparison due to case differences. I've worked around that for now by using EXPECT_STRCASEEQ but I wanted to raise it for discussion.

It seems that Linux and Windows stringify plUUID with lowercase letters. On macOS it's using uppercase letters. I suspect the plUUID AsString() is only used for debugging, but do we want to enforce lowercase consistency across all OSes?

dpogue commented 2 months ago

Okay, this should be good to go now. I opted to enforce that stringified UUIDs are lowercase for consistency with existing Windows code.

colincornaby commented 2 months ago

I'll toss in that just for "technically correct"-ness - UUIDs should always be compared case insensitively.

https://datatracker.ietf.org/doc/html/rfc4122

The hexadecimal values "a" through "f" are output as lower case characters and are case insensitive on input.

Apple's implementation looks out of line from the spec (I'd be curious as to why) as they output in upper case - but not in a way that should actually impact UUID use. Something to keep in mind for any sort of comparisons we do with UUIDs though.

(Backtracking through the UUID specifications is kind of weird in since their use was spread so wide, by my understanding is RFC4122 restates the UUID specification.)

dpogue commented 2 months ago

Something to keep in mind for any sort of comparisons we do with UUIDs though.

Any comparisons of UUIDs should be done bitwise by the plUUID class, this only came to light because I added a unit test that checked the stringified output against a known value.