Repository navigation
cmake: add -fzero-init-padding-bits=unions to fix tests - #22
vvfedorenko wants to merge 1 commit into
Conversation
|
Which test fails? Why would I care about the unused 11 bytes for a UBLOXCFG_VALUE_t::U1 value? (And why 11? Isn't it 7?) Also, is "downgrading" GCC behaviour a good idea? I wouldn't know... But the fewer compile flags the better I guess? Edit: and older GCC don't know about "-fzero-init-padding-bits=unions". So you can't have that! :-) Right? |
1c53f1a to
8e8058c
Compare
In my case TEST("decode config data") was failing.
Because it may contain garbage, and initializing unions the way it's done via macros is actually Undefined Behavior. It's 11 because it is UBLOXCFG_KEYVAL_t, not only UBLOXCFG_VALUE_t. As the latter union contains uinte64_t/double field, it has to be aligned to 8 bytes boundary, making a hole of 4 bytes in UBLOXCFG_KEYVAL_t between id and val. And these bytes are also undefined and may contain garbage.
As the code was written the way that relies on old GCC behavior, it is ok to downgrade as immediate fix. Refactoring will be needed in the future to completely avoid UB.
Yeah, I've added a bit of logic in CMakeLists.txt to avoid adding this option to unsupported compilers |
18e7d12 to
41680a3
Compare
|
I see. I'll have a look at this test. I think we can refactor the checks to only consider the relevant part of the union memory. https://github.com/phkehl/ubloxcfg/blob/main/ubloxcfg/ubloxcfg.h#L230 says "unused bytes shall be 0x00". I think this could be "unused bytes have no meaning and shall be ignored". However, perhaps _raw or _bytes is used somewhere other than the tests, where it relies on them being 0. I'll have a look. Otherwise we can add your cmake magic. But some comments:
Btw, I'm curious, what are you doing (with ubloxcfg)? |
41680a3 to
534fc00
Compare
Even though this will fix tests, I wouldn't go this way - there is no good in keeping UB.
That comment actually made me a bit worried :)
I'm not an expert in CMake, that's why I was playing with it around. But I have checked the code and unions are used in ubloxcfg and ff, so I'll move the check to corresponding CMakeLists.txt files.
Well, obviously, configuring and monitoring Ublox receivers in Oscillatord. I'm currently packaging ubloxcfg in Fedora44/Fedora45 releases |
Starting from GCC 15.1 union initializers is not guaranteed to zero-initialize all field, but only specified. It means that UBLOXCFG_KEYVAL_ANY(CFG_UBLOXCFGTEST_U1, 42) will initialize 5 bytes only and keep stack garbage in the rest 11 bytes of the UBLOXCFG_KEYVAL_t union. It broke just 1 test, but apparently any initializer in the code can now have garbage. Confirmation can be found in the Caveats section of the link provided. Fix it by reverting GCC to old behavior. Link: https://gcc.gnu.org/gcc-15/changes.html Signed-off-by: Vadim Fedorenko <vadfed@meta.com>
534fc00 to
4cd76e5
Compare
|
I'd rather not rely on the unused bytes being zero-initialised. While you can make GCC 15.1+ behave that way, there may be someone not using the cmake or another compiler or ... So what you think about my attempt at fixing it: #24 I've added a "gcc:latest" (currently 15.2) CI, which is happy. (It tested it with the memcmp() test, which fails as expected.) Nice timing card! ❤️ |
Fair. clang has different way of handling this UB.
That's the refactoring I was talking about. Nice that you have done so quickly
Cool, that makes CI more reliable.
Thanks! |
Starting from GCC 15.1 union initializers is not guaranteed to zero-initialize all field, but only specified. It means that UBLOXCFG_KEYVAL_ANY(CFG_UBLOXCFGTEST_U1, 42)
will initialize 5 bytes only and keep stack garbage in the rest 11 bytes of the UBLOXCFG_KEYVAL_t union. It broke just 1 test, but apparently any initializer in the code can now have garbage. Confirmation can be found in the Caveats section of the link provided.
Fix it by reverting GCC to old behavior.
Link: https://gcc.gnu.org/gcc-15/changes.html