Fix test failures when -fshort-enums is used#145
Fix test failures when -fshort-enums is used#145midnightveil wants to merge 2 commits intoseL4:masterfrom
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>
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. |
|
I've also hit this now (trying to build without docker for a change), and I don't see a way to remove the warning without just suppressing them, which does not sound like a good idea. Overall, I'd actually be happy with the casts as in the original PR. From everything we've been discussing, they are the lowest impact change. Trying to re-architect what |
|
My original objections still stand:
The root cause is the wrong assumption in the check macros, anything else is one work-around or another for that problem. |
So you'd remove the size assertion in here? https://github.com/seL4/seL4_libs/blob/4ce71fca2adbf1c2add7eabf5670633a70177a2f/libsel4test/include/sel4test/test.h#L239-L266 Wouldn't that make it more likely that something even more obscure is going wrong in all these |
|
I think we'd need to add two pairs of TYPES_COMPATIBLE checks then: one for a and one for b. That or I guess we really should be checking TYPES_COMPATIBLE(a, b) which I don't know why we didn't do originally instead of the size. [ It's not, strictly speaking, transitive, because whilst it might be the case that "enum X" ~ "int" and "enum Y" ~ "int", it is not the case that "enum X" ~ "enum Y" ]. Otherwise we'd run into the issue now I think where we see the short enum on the left, use the short enum printf specifier, but then also use it for the int on the right, since dispatch goes off the type of A. I think this is fine because varargs puts everything on the stack as an int, but I don't know if that's technically UB to do that or not. We really want 2 things:
In most other cases e.g. equality of numbers the default C promotion to int, or to the larger of a and b for binary operators should be fine. tldr: I think it's mostly about the printing. This would be a rework of the test_op macro, but it wouldn't break users, I don't think. |
Yes.
Maybe, but very unlikely. Worst case the top bits of I find all that macro code overly complex for no gain. It does all the type checking, and then it still needs to cast things. The sad thing is, because of those macros, you get punished for using the right enum type instead of If you don't want to remove the size check, changing these locations to also use |
|
Ok, fine, let's fix the macro. In our own code, these macros are used exclusively in @midnightveil, your plan sounds good to me. Do you have time to give this a go? |
Yeah, I can give this a shot today. (I think this PR is the easier fix, it's not like this code needs to be perfect, but I can fix it properly too) |
Ref: seL4/sel4test#145 (comment) This will fix the code's assumption that a & b are the same type, instead preferring to just format each separately, and also to compare with the operator naively, which should work for our purposes. Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
Ref: seL4/sel4test#145 (comment) This will fix the code's assumption that a & b are the same type, instead preferring to just format each separately, and also to compare with the operator naively, which should work for our purposes. Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
Ref: seL4/sel4test#145 (comment) This will fix the code's assumption that a & b are the same type, instead preferring to just format each separately, and also to compare with the operator naively, which should work for our purposes. I've verified locally that this allows me to compile locally with a toolchain that sets -fshort-enums, and also that simulation tests for {x86_64,aarch64,aarch32,riscv64} all pass with these changes. I've also tested that when it does fail it does manage to print an error correctly. Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
|
Closing this in favour of seL4/seL4_libs#111. |
Ref: seL4/sel4test#145 (comment) This will fix the code's assumption that a & b are the same type, instead preferring to just format each separately, and also to compare with the operator naively, which should work for our purposes. I've verified locally that this allows me to compile locally with a toolchain that sets -fshort-enums, and also that simulation tests for {x86_64,aarch64,aarch32,riscv64} all pass with these changes. I've also tested that when it does fail it does manage to print an error correctly. Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
Ref: seL4/sel4test#145 (comment) This will fix the code's assumption that a & b are the same type, instead preferring to just format each separately, and also to compare with the operator naively, which should work for our purposes. I've verified locally that this allows me to compile locally with a toolchain that sets -fshort-enums, and also that simulation tests for {x86_64,aarch64,aarch32,riscv64} all pass with these changes. I've also tested that when it does fail it does manage to print an error correctly. Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
Ref: seL4/sel4test#145 (comment) This will fix the code's assumption that a & b are the same type, instead preferring to just format each separately, and also to compare with the operator naively, which should work for our purposes. I've verified locally that this allows me to compile locally with a toolchain that sets -fshort-enums, and also that simulation tests for {x86_64,aarch64,aarch32,riscv64} all pass with these changes. I've also tested that when it does fail it does manage to print an error correctly. Signed-off-by: Julia Vassiliki <julia.vassiliki@unsw.edu.au>
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