Fix the GNU IFUNC error in alpine (non-glibc) environment#2133
Fix the GNU IFUNC error in alpine (non-glibc) environment#2133zuiderkwast merged 2 commits intovalkey-io:unstablefrom
Conversation
zhulipeng
changed the title
--------- Signed-off-by: Lipeng Zhu <[email protected]> Co-authored-by: Guo Wangyang <[email protected]>
Codecov ReportAll modified and coverable lines are covered by tests ✅
Additional details and impacted files@@ Coverage Diff @@
## unstable #2133 +/- ##
============================================
+ Coverage 71.18% 71.23% +0.04%
============================================
Files 122 122
Lines 66046 66046
============================================
+ Hits 47013 47046 +33
+ Misses 19033 19000 -33 🚀 New features to boost your workflow:
|
zhulipeng
added
the
run-extra-tests
Fusl
left a comment
Fusl
left a comment
There was a problem hiding this comment.
Should change the name of the temp file from foo to something more descriptive and exclude it from .gitignore or use an actual temporary file in /tmp using a random file suffix if we can do that logic in Makefiles.
| #define ATTRIBUTE_TARGET_AVX512 | ||
| #endif | ||
|
|
||
| #if defined(__linux__) && (defined(__GNUC__) && (__GNUC__ > 4) || defined(__clang__) && (__clang_major__) > 5) |
There was a problem hiding this comment.
Can't we just add defined(__GLIBC__) to this check? Like we did here:
One more thing: Does ifunc require linux or just glibc? It's possible to run freebsd with glibc. There's a debian variant with this combo.
There was a problem hiding this comment.
The ifunc feature requires ELF. Even though BSD systems have glibc, I'm not sure if there are any gaps between the loader and runtime linker compared to Linux.
--------- Signed-off-by: Lipeng Zhu <[email protected]> Co-authored-by: Guo Wangyang <[email protected]>
zuiderkwast
left a comment
zuiderkwast
left a comment
There was a problem hiding this comment.
Lgtm. If we have linux and glibc, we should be safe. We have cmake and make parallel builds so it's better to do it in the c file only.
zuiderkwast
changed the title
r-devulap
left a comment
r-devulap
left a comment
There was a problem hiding this comment.
Might be an overkill, but if we want a more robust approach, we could add a check to Makefile, something along the lines:
# In your Makefile
# Use a temporary file to test if 'ifunc' is supported
IFUNC_TEST := \
'#include <stdio.h>\n'\
'__attribute__((ifunc("resolve_func"))) int foo();\n'\
'int real_foo() { return 42; }\n'\
'int (*resolve_func(void))(void) { return real_foo; }\n'\
'int main() { return foo(); }\n'
check_ifunc_support:
echo -e $(IFUNC_TEST) > test_ifunc.c && \
$(CC) $(CFLAGS) test_ifunc.c -o test_ifunc 2>/dev/null && \
echo "IFUNC_SUPPORTED = 1" > ifunc.mk || \
echo "IFUNC_SUPPORTED = 0" > ifunc.mk
rm -f test_ifunc.c test_ifunc
-include ifunc.mk
# Now you can use $(IFUNC_SUPPORTED) in your Makefile
ifeq ($(IFUNC_SUPPORTED),1)
$(info IFUNC is supported)
else
$(info IFUNC is NOT supported)
endif
|
The first version of this PR already had a very similar approach: 72effba#diff-3e2513390df543315686d7c85bd901ca9256268970032298815d2f893a9f0685R153-R166 To simplify things we decided not to add a compiler check step but use one of the existing flags instead. |
|
Ah, apologies. I missed that commit :) |
…2133) Follow up of valkey-io#2099 Fix https://github.com/valkey-io/valkey/actions/runs/15221314860/job/42817270745, https://github.com/valkey-io/valkey/actions/runs/15221314860/job/42817271735 --------- Signed-off-by: Lipeng Zhu <[email protected]> Co-authored-by: Guo Wangyang <[email protected]> Signed-off-by: shanwan1 <[email protected]>
4 participants
Follow up of #2099
Fix https://github.com/valkey-io/valkey/actions/runs/15221314860/job/42817270745,
https://github.com/valkey-io/valkey/actions/runs/15221314860/job/42817271735