Fix test failures when -fshort-enums is used

[midnightveil]

When compiled with arm-none-eabi-gcc (13.3.0, and others), the default ABI has -fshort-enums on by default. This seem to also be the case for x86 with gcc (GCC) 15.1.1 20250521 (Red Hat 15.1.1-2).

Due to C's arcane integer promotion rules, the enum members such as SUCCESS or seL4_NoError are of type "int" (generally signed 32 bit), whereas the size of seL4_Error or test_result_t is 8 bits (a char).

For reference: https://godbolt.org/z/zoKzhqxc7

This produces test output like:

Error: res (size 1) != SUCCESS (size 4), use of test_eq incorrect

Test with: https://github.com/seL4/seL4_libs/pull/105

[midnightveil]

IDK, but probably a mix of:

• things like inequality (>) have odd behaviour when done on types of mix signedness e.g. (int)(-1) < (unsigned int)(1) can return false since -1 becomes 255.

• size checking produces a better error when types aren't compatible (especially now since it can be compiled time checked)

I also didn't want to make changes that might break many more things, so took the simplest solution...

[Indanz]

* things like inequality (>) have odd behaviour when done on types of mix signedness e.g. (int)(-1) < (unsigned int)(1) can return false since -1 becomes 255.

But then compilers warn about it, at least with warning enabled (it's part of -Wextra). Checking size doesn't help at all for mixed signedness problems.

* size checking produces a better error when types aren't compatible (especially now since it can be compiled time checked)

But different sized values are compatible when they have the same sign and are integers. AFAIK only floating point values are incompatible when they have different sizes, small chance the test accidentally pass. We can add a check for comparing floats with doubles instead of the size check.

I also didn't want to make changes that might break many more things, so took the simplest solution...

We have a wrong assumption in the check macro's (because they're part of the same code base that defined SUCCESS), which extends to all comparisons with enums, so to me the simple, low risk solution seems to remove those checks. That won't break any existing code and I don't see how it can cause future code to break.

If we keep that arbitrary size check, all users will stumble into this subtle enum size thing on some platforms with some toolchains. The static assert won't catch problems before they get into the code, only afterwards. So all it does is break the tests now and then for users for no good reason, like what happened to you now. The static assert just makes it easier to debug, it doesn't avoid the problem.

Removing the size checks does avoid and solve the problem, now and in the future.

[midnightveil]

But different sized values are compatible when they have the same sign and are integers. AFAIK only floating point values are incompatible when they have different sizes, small chance the test accidentally pass. We can add a check for comparing floats with doubles instead of the size check.

https://godbolt.org/z/bf8K6Krer

enum value { A, B } is never compatible with its int even if the sizeof fits, that wouldn't make sense. Also, two different enum types are not compatible either. We'd have to add support for every enum we'd ever want to use to this type compatibility test. (And, as a bonus, since we use this compatibility test, and not say, _Generic, we can't even make this fail to compile).

[Indanz]

https://godbolt.org/z/bf8K6Krer

enum value { A, B } is never compatible with its int even if the sizeof fits, that wouldn't make sense. Also, two different enum types are not compatible either. We'd have to add support for every enum we'd ever want to use to this type compatibility test. (And, as a bonus, since we use this compatibility test, and not say, _Generic, we can't even make this fail to compile).

Okay, enums are weird in C, we can't fix that (and apparently my use of compatible is different than the official use). But using __builtin_types_compatible_p doesn't fix the original problem:

_Static_assert(__builtin_types_compatible_p(typeof(SUCCESS), typeof(xxx_value)));

fails too, so it has exactly the same problems as the sizeof tests does.

So the rest of what I said still applies. Extra debug checks should help find real bugs, not cause false positives and subtle problems themselves. If you can't make them work for enums, remove them.

[midnightveil]

Okay, enums are weird in C, we can't fix that (and apparently my use of compatible is different than the official use). But using __builtin_types_compatible_p doesn't fix the original problem:

The code is using it already for type-based dispatch; my PR to seL4_libs did make it work. I appear to have confused myself slightly here; the type of the enum was never "int". (oops).

https://godbolt.org/z/1j48TG9cj

^ Needed to be unsigned int / unsigned char.

So the rest of what I said still applies. Extra debug checks should help find real bugs, not cause false positives and subtle problems themselves. If you can't make them work for enums, remove them.

But actually — the existing checks seem to be needed for type-based dispatch. I guess I'd need to think about it more and see if it's possible, but I don't see how you'd easily have a test_eq(a, b) print out the values of a and b without this type-dispatch, and unless we hardcode all the possibilities? Like, you could just pass in the format specifier at every callsite, and maybe that's nicer, but also it's significantly more effort than these 5 fixes.