gh-127604: Optimize -ldl usage on platforms that use dlopen for libFFI. - #133081
gh-127604: Optimize -ldl usage on platforms that use dlopen for libFFI.#133081freakboy3742 merged 2 commits into
Conversation
Sorry, something went wrong.
|
!buildbot iOS |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit 297f242 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133081%2Fmerge The command will test the builders whose names match following regular expression: iOS The builders matched are:
|
Sorry, something went wrong.
|
Nope... looks like that hasn't done it. I'll take another look in the morning if nobody beats me to it. |
Sorry, something went wrong.
picnixz
left a comment
There was a problem hiding this comment.
It looks fine and I'm pretty sure there is a cleaner and alternative way to do it but I'm also pretty sure that it would require more steps and a more complicate configuration so I think we should live with this first.
More generally, I think we need to have a function that cleans up the flags to remove duplicate ones.
Sorry, something went wrong.
Wait how come? then where is the one coming from...? |
Sorry, something went wrong.
|
Actually, it looks like it worked because before we had (in the "Compile build Python" step): ld: warning: ignoring duplicate libraries: '-ldl' ld: warning: ignoring duplicate libraries: '-ldl' ld: warning: ignoring duplicate libraries: '-ldl' ld: warning: ignoring duplicate libraries: '-ldl' ld: warning: ignoring duplicate libraries: '-ldl' ld: warning: ignoring duplicate libraries: '-ldl' and now we have ld: warning: ignoring duplicate libraries: '-ldl' ld: warning: ignoring duplicate libraries: '-ldl' ld: warning: ignoring duplicate libraries: '-ldl' ld: warning: ignoring duplicate libraries: '-ldl' So two warnings go eliminated. It remains to check what the others are. We also have 2 warnings less in the "Compile host Python" step. |
Sorry, something went wrong.
|
Note that the duplicate -ldl appear in only one command: gcc -ldl -o _bootstrap_python Modules/getbuildinfo.o [...] \
Programs/_bootstrap_python.o Modules/getpath.o -ldl -framework CoreFoundation |
Sorry, something went wrong.
|
@picnixz After a little more digging, I think I've found the source of the problem. The code added by #133040 is adding -ldl to the top-level LDFLAGS. This effectively means that every link command gets -ldl injected. Apple platforms then do a similar check; appending to LIBFFI_LDFLAGS. The initial form of this PR removes that addition, removing most of the duplicate library usage if the LDFLAGS version was used, avoiding most of the duplicates. However there's also this check for dl on L3714, which looks for dlopen; on success, this adds -ldl to LIBS. Having -ldl in both LIBS and LDFLAGS leads to the the remaining handful of duplicated cases, because LDFLAGS ends up being part of PY_LDFLAGS, which is part of PY_CORE_LDFLAGS - and many of the places that include PY_CORE_LDFLAGS also include LIBS. So - one fix is to move the LDFLAGS addition until after L3714, and only append to LDFLAGS if the dl check failed. I've done that in the updated version of the PR, but I'm not completely convinced that's the right approach. It seems like the pre-existing check for dl (that is documented to exist to support SunOS/Solaris, but is also picked up by macOS/iOS) seems incomplete/incompatible with how it's being added in service of faulthandler. It feels like it might be more appropriate to add -ldl only to the linking commands for faulthandler - which, by the Makefile, would be MODULE_FAULTHANDLER_LDFLAGS - but at that point I'm into the weeds of autoconf macros and it's not clear to me how that would be injected cleanly. |
Sorry, something went wrong.
|
!buildbot iOS |
Sorry, something went wrong.
|
🤖 New build scheduled with the buildbot fleet by @freakboy3742 for commit f6a117a 🤖 Results will be shown at: https://buildbot.python.org/all/#/grid?branch=refs%2Fpull%2F133081%2Fmerge The command will test the builders whose names match following regular expression: iOS The builders matched are:
|
Sorry, something went wrong.
picnixz
left a comment
There was a problem hiding this comment.
It works but I don't know if there is a better autoconf-way to do it
Sorry, something went wrong.
| dnl only add -ldl to LDFLAGS if it isn't already part of LIBS (GH-133081) | ||
| AS_VAR_IF([ac_cv_require_ldl], [yes], [ | ||
| AS_VAR_IF([ac_cv_lib_dl_dlopen], [yes], [], [ | ||
| AS_VAR_APPEND([LDFLAGS], [" -ldl"]) |
There was a problem hiding this comment.
Would autoconf be smart enough if we were to just use AC_CHECK_LIB for dladdr1 as we did for dlopen in that it wouldn't readd the -ldl flag or is it the only way to write these extra checks?
Sorry, something went wrong.
There was a problem hiding this comment.
Honestly, I don't know - I know enough autoconf to get by, but not much more.
Unfortunately, I don't have ready access to a development platform that would use the dladdr1 check (at least, I don't think I do), so it's difficult for me to experiment with this.
My concern would be whether it would add the flag to LIBS or LDFLAGS though - if it's added to LIBS, we'll be back in the same situation as before. Based on what I'm seeing in other AC_CHECK_LIB usage, it looks like it would get added to LIBS by default, and any strategy that got it into LDFLAGS would essentially be no better than AS_VAR_APPEND.
Sorry, something went wrong.
There was a problem hiding this comment.
I see. Let's keep it that way (it works, it's a bit fragile, but we'll manage if there are issues in the future)
Sorry, something went wrong.
#133040 modified the handling of -ldl, resulting in multiple copies of -ldl being included in link commands.
#133071 modified this handling to minimise a lot of those usages; but on platforms that use dlopen() (macOS and iOS), there was still some duplicated use. This PR cleans up that usage.