Correctly link libfastjson against libm on glibc ≥ 2.42 #171
Conversation
Pull Request rsyslog#167 modified libfastjson.pc to add an explicit dependency on -lm through pkg-config. While this worked around missing modf() resolution under glibc ≥ 2.42, it pushed the responsibility onto applications. The correct solution is for libfastjson itself to link against libm so that dependent applications do not need to adjust their linker flags. This commit removes the pkg-config modifications introduced by commit 919f7d2.
glibc ≥ 2.42 no longer provides modf() through implicit linkage, so any library using modf() must explicitly link libm. The previous approach conditionally added -lm only if isnan() failed to link, which is no longer sufficient and did not guarantee that libfastjson itself linked against libm. Replace the isnan-based conditional check with an explicit AC_CHECK_LIB([m], [modf]) so that -lm is always added when required. This ensures libfastjson links directly against libm, allowing dependent applications to link only against libfastjson without needing to add -lm themselves. Fixes: rsyslog#167
Summary of ChangesHello @Whissi, I'm Gemini Code Assist1! I'm currently reviewing this pull request and will post my feedback shortly. In the meantime, here's a summary to help you and other reviewers quickly get up to speed! This pull request addresses a critical linking issue for libfastjson on newer glibc versions (2.42 and above). It ensures that math functions, specifically modf(), are correctly resolved by explicitly linking libfastjson against libm during the build configuration process. This change prevents runtime errors and ensures compatibility with modern glibc environments, providing a robust and correct solution compared to previous workarounds. Highlights
Using Gemini Code AssistThe full guide for Gemini Code Assist can be found on our documentation page, here are some quick tips. Invoking Gemini You can request assistance from Gemini at any point by creating a comment using either
Customization To customize Gemini Code Assist for GitHub experience, repository maintainers can create a configuration file and/or provide a custom code review style guide (such as PEP-8 for Python) by creating and adding files to a Limitations & Feedback Gemini Code Assist may make mistakes. Please leave feedback on any instances where its feedback is incorrect or counter productive. You can react with 👍 and 👎 on @gemini-code-assist comments. If you're interested in giving your feedback about your experience with Gemini Code Assist for Github and other Google products, sign up here. You can also get AI-powered code generation, chat, as well as code reviews directly in the IDE at no cost with the Gemini Code Assist IDE Extension. Footnotes
|
![gemini-code-assist[bot]](https://avatars.githubusercontent.com/in/956858?s=60&v=4){kind=small}
There was a problem hiding this comment.
Code Review
This pull request aims to fix an issue with linking against libm on newer glibc versions by explicitly linking libfastjson with libm. The changes correctly update the pkg-config file to move -lm to Libs.private. However, the change in configure.ac is insufficient to actually link the libfastjson.so shared library against libm, because it only modifies the LIBS variable which is not used when linking the library with libtool. I've left a detailed comment on configure.ac with a suggested fix, which will require modifying Makefile.am as well.
| AC_TRY_LINK([#include <math.h>], [float f = 0.0; return isnan(f)], [], [LIBS="$LIBS -lm"]) | ||
| fi | ||
| # Force linking against libm (needed for modf on glibc >= 2.42) | ||
| AC_CHECK_LIB([m], [modf], [LIBS="$LIBS -lm"],[AC_MSG_ERROR([libm (math library) is required for modf()])]) |
There was a problem hiding this comment.
This change is insufficient to link libfastjson.so against libm. The LIBS variable is not used by libtool for linking shared libraries; instead, dependencies should be added to the _LIBADD variable in Makefile.am.
To correctly link libfastjson.so against libm, you'll need to modify both configure.ac and Makefile.am.
-
In
configure.ac, create and substitute a variable for the math library. You can replace the current line with:LIBM="" AC_CHECK_LIB([m], [modf], [LIBM="-lm"], [AC_MSG_ERROR([libm (math library) is required for modf()])]) AC_SUBST([LIBM]) LIBS="$LIBS $LIBM"
This creates a
LIBMvariable forMakefile.amand also updatesLIBSforpkg-config. -
In
Makefile.am(which you'll need to add to this PR), use this new variable:libfastjson_la_LIBADD = libfastjson-internal.la $(LIBM)
This will ensure libfastjson.so is correctly linked against libm, achieving the goal of this pull request.
There was a problem hiding this comment.
I believe this is a false positive. I am matching existing style which already writes directly into $LIBS variable and I verified that the resulting libfastjson.so is now linked against libm:
$ objdump -x /usr/lib64/libfastjson.so.4.3.0 | grep NEEDED | grep libm
NEEDED libm.so.6
There was a problem hiding this comment.
my autotools knowledge is a bit rusty at this point but his looks correct to me. Thanks @Whissi !
There was a problem hiding this comment.
I believe this is a false positive. I am matching existing style which already writes directly into
$LIBSvariable and I verified that the resultinglibfastjson.sois now linked againstlibm:
The generated Makefile.in looks like this:
Makefile.in: $(AM_V_CCLD)$(libfastjson_internal_la_LINK) $(libfastjson_internal_la_OBJECTS) $(libfastjson_internal_la_LIBADD) $(LIBS)
Which would confirm that LIBS is actually passed to the linker flags.
2 participants
This PR fixes the handling of
modf()and other math functions on systems using glibc-2.42 or later.glibc ≥ 2.42 no longer provides implicit linkage for certain math functions (in particular the IFUNC-resolved modf()), which means
libraries must explicitly link against libm. The previous workaround introduced in #167 added
-lmto thepkg-configfile, but did not fix the underlying issue: libfastjson itself was not linking againstlibm.This PR implements the correct solution. Thank you @mbiebl for the feedback!
Note: Autotools will automatically add
Libs.private: -lmtolibfastjsonif needed for static builds.