gh-127604: Optimize -ldl usage on platforms that use dlopen for libFFI. #133081
Conversation
|
!buildbot iOS |
|
🤖 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: The builders matched are:
|
|
Nope... looks like that hasn't done it. I'll take another look in the morning if nobody beats me to it. |
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.
Wait how come? then where is the one coming from...? |
|
Actually, it looks like it worked because before we had (in the "Compile build Python" step): and now we have 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. |
|
Note that the duplicate gcc -ldl -o _bootstrap_python Modules/getbuildinfo.o [...] \
Programs/_bootstrap_python.o Modules/getpath.o -ldl -framework CoreFoundation |
|
@picnixz After a little more digging, I think I've found the source of the problem. The code added by #133040 is adding Apple platforms then do a similar check; appending to However there's also this check for Having So - one fix is to move the It seems like the pre-existing check for |
|
!buildbot iOS |
|
🤖 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: The builders matched are:
|
| 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?
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.
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)
#133040 modified the handling of
-ldl, resulting in multiple copies of-ldlbeing 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.faulthandler#127604