enforce 64‑bit off_t regardless of include order; prevent LTO type mismatch (Fixes #2938)#2943
Conversation
…t off_t to prevent LTO type mismatch (Fixes valkey-io#2938) Signed-off-by: Ada-Church-Closure <2574094394@qq.com>
|
Thank you! It seems like we need (https://github.com/valkey-io/valkey/actions/runs/20293241116/job/58411398779?pr=2943#step:3:1337) |
Codecov Report✅ All modified and coverable lines are covered by tests. Additional details and impacted files@@ Coverage Diff @@
## unstable #2943 +/- ##
============================================
+ Coverage 72.28% 73.84% +1.55%
============================================
Files 129 125 -4
Lines 70540 68898 -1642
============================================
- Hits 50993 50879 -114
+ Misses 19547 18019 -1528
🚀 New features to boost your workflow:
|
|
Thanks!I will handle this. |
…); server: assert 64-bit off_t; unit:
include fmacros early (Fixes valkey-io#2938)
Signed-off-by: Ada-Church-Closure <2574094394@qq.com>
|
I added fmacros.h at the top of headers that can be included standalone (entry/expire/lrulfu) and at the top of unit tests that include headers before server.h. Also reverted the sds.h change to avoid macro redefs in deps/libvalkey. The 32‑bitbuild should pass now; server.h keeps the static_assert as a guard against regressions. |
File seems to be added by mistake. Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
…smatch (valkey-io#2943) Fixes valkey-io#2938 - Root cause: On 32-bit builds, off_t depended on include order. lrulfu.c included <stdint.h>/<stdbool.h> via lrulfu.h before server.h, so _FILE_OFFSET_BITS=64 (from fmacros.h) was not in effect when glibc headers were first seen. This made off_t 32-bit in that TU, while 64-bit elsewhere, causing LTO linker “type mismatch” warnings and possible misoptimization. - Changes: 1. Include fmacros.h first in src/lrulfu.h to ensure feature macros apply before any system header. 2. Add a compile-time check in src/server.h: static_assert(sizeof(off_t) >= 8, "off_t must be 64-bit; ensure _FILE_OFFSET_BITS=64 is in effect before system headers"); so we fail fast if include order regresses. - Why this works: fmacros.h defines _FILE_OFFSET_BITS 64 and related feature macros. Ensuring it is seen first gives a consistent 64-bit off_t across all TUs. The static_assert turns future include-order mistakes into early compile-time failures instead of link-time notes/warnings. - Testing: - Built on 32-bit Debian: no LTO type-mismatch at link, binaries produced successfully. Only GCC 11 ABI notes about _Atomic alignment (“note: … changed in GCC 11.1”), which are informational (-Wpsabi) and do not affect correctness. - Risk: very low; only header include order + a defensive assert. No runtime changes. - Address CI feedback: add fmacros.h to unit sources that include headers before server.h; --------- Signed-off-by: Ada-Church-Closure <2574094394@qq.com> Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Ada-Church-Closure <2574094394@qq.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
|
This solution seems brittle, and relies on including a file which wouldn't otherwise be needed in almost every .h file. Would a better option be to use the |
With the static_assert, it doesn't seem too brittle to me, but without it I would agree.
That's an interesting idea, but that may be brittle too. We have make and cmake files to maintain now and I know the two builds are already not identical. We could do both, i.e. explicit #includes and the -include option. |
|
The issue is that you should include what you use. What we've done here is to include a file, which we don't use, because it has an indirect effect on something we use (or something used by something we use). This doesn't make any sense. Other than by trial and error, using the static assert, we don't know that we need this file. Also - the message from the assert:
doesn't really help. This should say something like "ensure that fmacros.h is included before any system headers".
Also of concern is that this "fix" is not applied universally or consistently. What we need is that for each compilation unit (.c file), we must include "fmacros.h" first. But instead, the PR applies a change in a small handful of .h files. Reordering of the .h files inside the .c file will break things. For example, if, in |
|
What matters most is to fix the failures as soon as possible. That's why I merged it quickly. As for whether it's better to include fmacros.h explicitly or using a compiler command line flag, I think of little practical importance, as long as we have the static assert. I wouldn't say the fmacros.h include file is unused. What it does is to define feature test macros, which need to be defined before including standard headers to affect some behavior, as documented in the man page feature_test_macros(7). IMO, if some .c or .h file relies on It would have been enough to define only the specific macro ( For reference, I also found this in the man page for off_t(3type): |
…smatch (valkey-io#2943) Fixes valkey-io#2938 - Root cause: On 32-bit builds, off_t depended on include order. lrulfu.c included <stdint.h>/<stdbool.h> via lrulfu.h before server.h, so _FILE_OFFSET_BITS=64 (from fmacros.h) was not in effect when glibc headers were first seen. This made off_t 32-bit in that TU, while 64-bit elsewhere, causing LTO linker “type mismatch” warnings and possible misoptimization. - Changes: 1. Include fmacros.h first in src/lrulfu.h to ensure feature macros apply before any system header. 2. Add a compile-time check in src/server.h: static_assert(sizeof(off_t) >= 8, "off_t must be 64-bit; ensure _FILE_OFFSET_BITS=64 is in effect before system headers"); so we fail fast if include order regresses. - Why this works: fmacros.h defines _FILE_OFFSET_BITS 64 and related feature macros. Ensuring it is seen first gives a consistent 64-bit off_t across all TUs. The static_assert turns future include-order mistakes into early compile-time failures instead of link-time notes/warnings. - Testing: - Built on 32-bit Debian: no LTO type-mismatch at link, binaries produced successfully. Only GCC 11 ABI notes about _Atomic alignment (“note: … changed in GCC 11.1”), which are informational (-Wpsabi) and do not affect correctness. - Risk: very low; only header include order + a defensive assert. No runtime changes. - Address CI feedback: add fmacros.h to unit sources that include headers before server.h; --------- Signed-off-by: Ada-Church-Closure <2574094394@qq.com> Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Ada-Church-Closure <2574094394@qq.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech>
…smatch (valkey-io#2943) Fixes valkey-io#2938 - Root cause: On 32-bit builds, off_t depended on include order. lrulfu.c included <stdint.h>/<stdbool.h> via lrulfu.h before server.h, so _FILE_OFFSET_BITS=64 (from fmacros.h) was not in effect when glibc headers were first seen. This made off_t 32-bit in that TU, while 64-bit elsewhere, causing LTO linker “type mismatch” warnings and possible misoptimization. - Changes: 1. Include fmacros.h first in src/lrulfu.h to ensure feature macros apply before any system header. 2. Add a compile-time check in src/server.h: static_assert(sizeof(off_t) >= 8, "off_t must be 64-bit; ensure _FILE_OFFSET_BITS=64 is in effect before system headers"); so we fail fast if include order regresses. - Why this works: fmacros.h defines _FILE_OFFSET_BITS 64 and related feature macros. Ensuring it is seen first gives a consistent 64-bit off_t across all TUs. The static_assert turns future include-order mistakes into early compile-time failures instead of link-time notes/warnings. - Testing: - Built on 32-bit Debian: no LTO type-mismatch at link, binaries produced successfully. Only GCC 11 ABI notes about _Atomic alignment (“note: … changed in GCC 11.1”), which are informational (-Wpsabi) and do not affect correctness. - Risk: very low; only header include order + a defensive assert. No runtime changes. - Address CI feedback: add fmacros.h to unit sources that include headers before server.h; --------- Signed-off-by: Ada-Church-Closure <2574094394@qq.com> Signed-off-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Co-authored-by: Ada-Church-Closure <2574094394@qq.com> Co-authored-by: Viktor Söderqvist <viktor.soderqvist@est.tech> Signed-off-by: Harkrishn Patro <bunty.hari@gmail.com>
Fixes #2938
1. Include fmacros.h first in src/lrulfu.h to ensure feature macros apply before any system header.
2. Add a compile-time check in src/server.h:
static_assert(sizeof(off_t) >= 8, "off_t must be 64-bit; ensure _FILE_OFFSET_BITS=64 is in effect before system headers");
so we fail fast if include order regresses.
turns future include-order mistakes into early compile-time failures instead of link-time notes/warnings.
- Built on 32-bit Debian: no LTO type-mismatch at link, binaries produced successfully. Only GCC 11 ABI notes about _Atomic alignment (“note: … changed in GCC 11.1”), which are informational (-Wpsabi) and do not affect correctness.