Use may_alias with unaligned reads to fix miscompilation on GCC

lassipulkkinen commented

2025-02-13 16:48:16 -06:00

(Migrated from github.com)

When XXH_FORCE_MEMORY_ACCESS==1, which is the default on supported compilers unlike stated in the README, a strict aliasing violation occurs in XXH_read64, resulting in miscompilation on GCC (but not Clang) in some oddly specific circumstances.

The following code reproduces the problem on x86_64 GCC 14.2.1 when compiled with -O3:

#define XXH_INLINE_ALL

#include <inttypes.h>
#include <stdio.h>
#include <xxhash.h>

int main() {
	// it seems this has to be exactly 24 bytes.
	union {
		char x[24];
		// force 8-byte alignment without making
		// aliasable with uint64_t.
		void *y[3];
	} data = {.x = "garblegarblegarblegarble"};
	uint64_t hash = XXH64(&data, sizeof(data), 0);
	printf("%016"PRIx64"\n", hash);
	return 0;
}

A bogus -Wuninitialized warning is produced if enabled, and the resulting program outputs an incorrect hash.

While this definitely looks like a compiler bug in some ways, I see no reason to assume aligned(1) alone should excempt the type from aliasing restrictions regardless, and adding may_alias does fix the problem.

The separate strict aliasing bug when XXH_FORCE_ALIGN_CHECK==1, discussed in #383, still remains.

When `XXH_FORCE_MEMORY_ACCESS==1`, which is the default on supported compilers unlike stated in the README, a strict aliasing violation occurs in `XXH_read64`, resulting in miscompilation on GCC (but not Clang) in some oddly specific circumstances. The following code reproduces the problem on x86_64 GCC 14.2.1 when compiled with -O3: ```c #define XXH_INLINE_ALL #include <inttypes.h> #include <stdio.h> #include <xxhash.h> int main() { // it seems this has to be exactly 24 bytes. union { char x[24]; // force 8-byte alignment without making // aliasable with uint64_t. void *y[3]; } data = {.x = "garblegarblegarblegarble"}; uint64_t hash = XXH64(&data, sizeof(data), 0); printf("%016"PRIx64"\n", hash); return 0; } ``` A bogus `-Wuninitialized` warning is produced if enabled, and the resulting program outputs an incorrect hash. While this definitely looks like a compiler bug in some ways, I see no reason to assume `aligned(1)` alone should excempt the type from aliasing restrictions regardless, and adding `may_alias` does fix the problem. The separate strict aliasing bug when `XXH_FORCE_ALIGN_CHECK==1`, discussed in #383, still remains.

👍 1

Cyan4973 commented

2025-02-19 18:20:08 -06:00

(Migrated from github.com)

XXH64() exposes a void* pointer as input argument,
so it should be enough to allow aliasing.

However, with inlining via XXH_INLINE_ALL, I guess the compiler can pass through this interface layer, and access directly the code below it, skipping the void* intermediate stage.
I don't know C well enough to tell if the compiler is allow to do that (i.e. ignore void* contract), or is well within its right.
Either way, I can see how this can be an immediate problem.

The proposed fix is simple enough, and it should do exactly as advertised, i.e. allow aliasing, even when inlining.

That being said, this issue has not been exposed in CI so far, showing that we probably need to amend the test suite to produce the problem, observe its fix, and ensure the property remains guaranteed over time.

Since you also provide a test case in the issue description, I'll try to use that as a basis for the new test.

`XXH64()` exposes a `void*` pointer as input argument, so it should be enough to allow aliasing. However, with inlining via `XXH_INLINE_ALL`, I guess the compiler can pass through this interface layer, and access directly the code below it, skipping the `void*` intermediate stage. I don't know C well enough to tell if the compiler is allow to do that (i.e. ignore `void*` contract), or is well within its right. Either way, I can see how this can be an immediate problem. The proposed fix is simple enough, and it should do exactly as advertised, i.e. allow aliasing, even when inlining. That being said, this issue has not been exposed in CI so far, showing that we probably need to amend the test suite to produce the problem, observe its fix, and ensure the property remains guaranteed over time. Since you also provide a test case in the issue description, I'll try to use that as a basis for the new test.

thesamesam commented

2025-03-05 07:04:49 -06:00

(Migrated from github.com)

The compiler is allowed to do this. You have to cast back to the original type (or a compatible one) for it to be legal, i.e. void* isn't a firewall for aliasing.

Using may_alias is fine as a workaround, just not all compilers support it.

The compiler is allowed to do this. You have to cast back to the original type (or a compatible one) for it to be legal, i.e. `void*` isn't a firewall for aliasing. Using `may_alias` is fine as a workaround, just not all compilers support it.

Cyan4973 commented

2025-06-15 20:31:20 -05:00

(Migrated from github.com)

I've been trying to reproduce the problem, by using exactly the example provided in this PR.

It's still not clear to me if this is a compilation issue (warning produced during compilation) or a runtime issue (strict aliasing violated at runtime, hopefully detectable with the UB sanitizer).
I tried both, but with no reproduction so far.

Here is my exact compilation line :

cc -g -fsanitize=undefined -O3 -Wall -Wuninitialized  -DXXH_FORCE_MEMORY_ACCESS=1   test_alias.c   -o test_alias

I've been using both gcc-9 and gcc-10.

The system I'm testing on doesn't have access to gcc-14.
It could be that this version of gcc is required to reproduce the issue.
In which case, I will need to test this scenario on another system.

edit: another test with mingw64, using gcc-15.1 compiler,
still unable to reproduce the mentioned warning.

I've been trying to reproduce the problem, by using exactly the example provided in this PR. It's still not clear to me if this is a compilation issue (warning produced during compilation) or a runtime issue (strict aliasing violated at runtime, hopefully detectable with the UB sanitizer). I tried both, but with no reproduction so far. Here is my exact compilation line : ``` cc -g -fsanitize=undefined -O3 -Wall -Wuninitialized -DXXH_FORCE_MEMORY_ACCESS=1 test_alias.c -o test_alias ``` I've been using both gcc-9 and gcc-10. The system I'm testing on doesn't have access to `gcc-14`. It could be that this version of `gcc` is required to reproduce the issue. In which case, I will need to test this scenario on another system. _edit_: another test with `mingw64`, using `gcc-15.1` compiler, still unable to reproduce the mentioned warning.

thesamesam commented

2025-06-15 20:36:16 -05:00

(Migrated from github.com)

UBSAN doesn't detect aliasing problems, though it will detect unaligned access. Clang has a new sanitizer for this (-fsanitize=type) but it's very primitive for now.

I think the aliasing violation is clear here though, a char* gets read as xxh_unalign64*.

UBSAN doesn't detect aliasing problems, though it will detect unaligned access. Clang has a new sanitizer for this (`-fsanitize=type`) but it's very primitive for now. I think the aliasing violation is clear here though, a `char*` gets read as `xxh_unalign64*`.

👍 1

Cyan4973 commented

2025-06-18 18:10:42 -05:00

(Migrated from github.com)

Finally, I could reproduce the warning using gcc-13.3.0.
Let's now try to create a new CI test based on this warning.

Finally, I could reproduce the warning using `gcc-13.3.0`. Let's now try to create a new CI test based on this warning.

Cyan4973 commented

2025-06-18 18:49:31 -05:00

(Migrated from github.com)

OK, we now have a failing test in #1047,
so if all goes according to expectations,
rebasing the failing test on top of this patch will fix the test.

OK, we now have a failing test in #1047, so if all goes according to expectations, rebasing the failing test on top of this patch will fix the test.

Rows
Columns

Use may_alias with unaligned reads to fix miscompilation on GCC #1013