CallableCustomMethodPointerBase: Make _setup strict-aliasing safe

[corngood]

I found a bug when compiling a production build of godot 4 (template) for x86_64-linux using GCC 14:

ERROR: Signal 'changed' is already connected to given callable '' in that object.
   at: connect (core/object/object.cpp:1451)

I tracked this down to incorrect optimisation of CallableCustomMethodPointerBase::_setup, caused by invalid pointer aliasing.

There are various ways this could be fixed. I went with one that gives the same results as the current implementation. I chose to use char since the language spec references it, but uint8_t would probably also be okay in practice.

I can provide a repro as a nix expression, but it's very sensitive to the compiler configuration, so it might be difficult to reproduce otherwise.

hash_murmur3_buffer might be a good candidate if we don't mind changing the result. However, it looks like it also uses invalid aliasing:

static _FORCE_INLINE_ uint32_t hash_murmur3_buffer(const void *key, int length, const uint32_t seed = HASH_MURMUR3_SEED) {
	// Although not required, this is a random prime number.
	const uint8_t *data = (const uint8_t *)key;
	const int nblocks = length / 4;

	uint32_t h1 = seed;

	const uint32_t c1 = 0xcc9e2d51;
	const uint32_t c2 = 0x1b873593;

	const uint32_t *blocks = (const uint32_t *)(data + nblocks * 4);

I'm not sure if it's worth trying to proactively fix things like this, do a more exhaustive search, or just turn on -fno-strict-aliasing.

[corngood]

@rune-scape @akien-mga excuse the pings, you were involved in the last substantial PR in this area

[Ivorforce]

The fix looks sound; this should never have used uint32_t* in the first place for raw data.

hash_murmur3_buffer might be a good candidate if we don't mind changing the result.

I agree it would be better to just hash a buffer, but I'm not familiar with the code. So I don't know if this would break compatibility somehow.

However, it looks like it also uses invalid aliasing:

This should probably be fixed too.

[corngood]

This should probably be fixed too.

I added a commit to fix this. It's not as thoroughly tested as the other one.

I can pull it into another PR if you prefer.

Edit: I ran a few tests to confirm the results are unchanged when it's called in practice, so I think it's probably solid.

[Ivorforce]

The code looks good! I only have a few comment and one style request now :)

[Ivorforce]

The code looks good to me, thanks!

It should probably be tested before merging.
I also can't comment on whether this has any implications on GDExtension semantics (though I don't think it does).

[corngood]

It should probably be tested before merging.

Let me know if there's anything you're interesting in testing in particular. I did run a bit with some assertions that the new functions match the results of the old functions. I also ran the test suite locally.

I've only tested on x86_64-linux with gcc 14.