Fix test failures when -fshort-enums is used #145
Conversation
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 Signed-off-by: julia <git.ts@trainwit.ch>
| api_make_guard_skip_word(guard), | ||
| env->page_directory, seL4_NilData); | ||
| test_eq(err, 0); | ||
| test_eq(err, (seL4_Error)seL4_NoError); |
There was a problem hiding this comment.
The other occurrences of seL4_NoError in this repo are just test_eq(err, seL4_NoError). The type of seL4_NoError is already the enum, so it looks to me like we should not need an explicit cast here. Or is there something special going on in this one?
(we would need to cast the 0, since that would be a literal with C's own arcane rules for that)
There was a problem hiding this comment.
(same for SUCCESS, really, unless there is a #define layer that I have overlooked)
There was a problem hiding this comment.
The other cases have err defined as type int, so the RHS matches the same type as the LHS. These ones are special because err is seL4_Error which then has size 1, not 4.
res has the type of test_result_t and not int.
I don't know if there's a better way to solve this generally, this seems really annoying because of C integer promotion rules.
Indanz
left a comment
Indanz
left a comment
There was a problem hiding this comment.
I acknowledge the problem, but I disagree with the solution: Surely the solution is to remove the sizeof check in the test_eq macro? What does that check add?
|
IDK, but probably a mix of:
I also didn't want to make changes that might break many more things, so took the simplest solution... |
But then compilers warn about it, at least with warning enabled (it's part of
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.
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. |
https://godbolt.org/z/bf8K6Krer
|
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: 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. |
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 https://godbolt.org/z/1j48TG9cj ^ Needed to be unsigned int / unsigned char.
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 |
|
The other alternative is we just pass through |
Agreed, not sure why we didn't think of that before. |
|
Hm. This happens for every object linked in, regardless of if I add There's a way to hide this ( Not sure if I like that approach now. |
3 participants
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:
Test with: seL4/seL4_libs#105