extendhfsf2_test.c seems to exhibit undefined behavior

[atrosinenko]

The extendhfsf2_test.c unit test looks suspicious. When debugging optimization-related test failures on MSP430, I have finally managed to make all tests either pass or not, not depending on the optimization level. Except extendhfsf2_test. It passed with -O0, -O1, -Os, -Oz but not with -O2 and higher. After a bit of debugging, turned out that the test__extendhfsf2 function was inlined.

How to reproduce

Compile the compiler-rt builtins with

$ CFLAGS=-O2 cmake ../compiler-rt -G Ninja -DCOMPILER_RT_INCLUDE_TESTS=ON -DCOMPILER_RT_BUILD_BUILTINS=ON -DCMAKE_C_COMPILER=$llvmbin/clang -DCMAKE_CXX_COMPILER=$llvmbin/clang++ -DLLVM_CONFIG_PATH=$llvmbin/llvm-config
$ ninja
$ ninja check-builtins

Everything is OK. Then apply the following patch:

--- a/compiler-rt/test/builtins/Unit/extendhfsf2_test.c
+++ b/compiler-rt/test/builtins/Unit/extendhfsf2_test.c
@@ -18,7 +18,7 @@
 
 float __extendhfsf2(uint16_t a);
 
-int test__extendhfsf2(uint16_t a, float expected)
+int __attribute__((always_inline)) test__extendhfsf2(uint16_t a, float expected)
 {
     float x = __extendhfsf2(a);
     int ret = compareResultH(x, expected);

and add -O2 to the compiler parameters in test/builtins/Unit/X86_64LinuxConfig/Output/extendhfsf2_test.c.script.

While these changes are not expected to change the behavior, now running extendhfsf2_test.c.script prints:

error in test__extendhfsf2(0xc248) = -3.140625, expected -3.141593

For me, it is the call to compareResultH seems suspicious because if expects two uint16_t arguments but two floats are passed.

If I add the following patch in addition to forcefully inlining:

@@ -26,7 +26,7 @@ int test__extendhfsf2(uint16_t a, float expected)
     if (ret){
         printf("error in test__extendhfsf2(%#.4x) = %f, "
                "expected %f\n", a, x, expected);
-    }
+    } else printf("OK\n");
     return ret;
 }

it just prints lots of OK and exits successfully.

[atrosinenko]

With some clumsy and possibly incorrectly rewritten compiler command line (because plain -fsanitize=undefined does not work for such setup due to -nodefaultlibs, etc.) I got the following UBSan diagnostics:

/old_ssd/ast/msp430-projects/llvm-project/compiler-rt/test/builtins/Unit/extendhfsf2_test.c:24:30: runtime error: nan is outside the range of representable values of type 'unsigned short'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /old_ssd/ast/msp430-projects/llvm-project/compiler-rt/test/builtins/Unit/extendhfsf2_test.c:24:30 in 
/old_ssd/ast/msp430-projects/llvm-project/compiler-rt/test/builtins/Unit/extendhfsf2_test.c:24:33: runtime error: nan is outside the range of representable values of type 'unsigned short'
SUMMARY: UndefinedBehaviorSanitizer: undefined-behavior /old_ssd/ast/msp430-projects/llvm-project/compiler-rt/test/builtins/Unit/extendhfsf2_test.c:24:33 in 
error in test__extendhfsf2(0xc248) = -3.140625, expected -3.141593

[asl]

This might be an ubsan fault after all :) Or something within the pipeline that has some similar weird requirements.

[atrosinenko]

This particular diagnostic message can surely be ubsan fault because the compiler command line was very hackish and "best-effort". But the fact everything "works correctly" when adding more debug output seems very suspicious. On the other hand, technically, this can be some controlled UB usage by the compiler developers. :)

Another question is why two float arguments are passed for comparison to compareResultH(uint16_t result, uint16_t expected)? As I understand, uint16_t here is a substitution for some "IEEE754 type with half-precision" but actual arguments are traditional single-precision floats. As I understand, the compiler tries to convert them to integral type according to their numerical value while the expected behavior here is probably reinterpret_cast.

Looking at other such tests:

llvm-project/compiler-rt/test/builtins/Unit/extendsfdf2vfp_test.c

Lines 26 to 31 in 40ae542

	double actual = __extendsfdf2vfp(a);
	double expected = a;
	if (actual != expected)
	printf("error in test__extendsfdf2vfp(%f) = %f, expected %f\n",
	a, actual, expected);
	return actual != expected;

Here doubles are just compared with !=.

llvm-project/compiler-rt/test/builtins/Unit/extendsftf2_test.c

Lines 26 to 34 in 40ae542

	long double x = __extendsftf2(a);
	int ret = compareResultLD(x, expectedHi, expectedLo);
	if (ret)
	{
	printf("error in test__extendsftf2(%f) = %.20Lf, "
	"expected %.20Lf\n", a, x, fromRep128(expectedHi, expectedLo));
	}
	return ret;

Here long double is checked against its binary representation (represented by two uint64_t values) by compareResultLD.

In the test__extendhfsf2 compareResultF was probably expected.

[asl]

Most probably here're we're dealing with uint16_t being a storage-only representation of half precision FPs on ARM. This is storage-only type, it is implicitly converted to / from float on every operation per specification.

I would simply disable hfsf builtin for us, it does not make any sense.