ld-wrapper: Optimize expanding rpath #27657
Conversation
orivej
force-pushed
the
optimize-rpath
branch
2 times, most recently
from
d11383f
to
7385c67
Compare
|
Tested building |
|
Relying on |
|
Thanks for the suggestions! When Associative array |
orivej
force-pushed
the
optimize-rpath
branch
2 times, most recently
from
e2a8520
to
8b129a2
Compare
|
This: turned out to be even faster than this so I updated the pull request (and also replaced |
|
I have added |
|
The |
|
Yes, thank you for bringing up I've tested building |
| if [ -f "$i/lib$j.so" ]; then | ||
| addToRPath $i | ||
| for dir in "${libDirs[@]}"; do | ||
| for file in "$dir"/lib*.so; do |
There was a problem hiding this comment.
wait, how the hell have we been hard-coding .so on Darwin?! CC @copumpkin
| addToRPath $i | ||
| for dir in "${libDirs[@]}"; do | ||
| for file in "$dir"/lib*.so; do | ||
| if [ "${libs[${file##*/}]}" ]; then |
There was a problem hiding this comment.
Is it possible for file to bind a path with a trailing slash?
There was a problem hiding this comment.
Above you mention "$dir"/*, but then you changed * to lib*.so, so I think it's not needed anymore.
There was a problem hiding this comment.
What is not needed anymore? I take it the idea here is to establish a basename => dir mapping, whereas the generated items will contain the dir so you need to take the basename (aka ${file##*/}).
There was a problem hiding this comment.
Oh right, pruning the dirname, duh.
There was a problem hiding this comment.
file must end with .so so it can not end with /, even if lib*.so is a directory, which is not necessary to support, and testing if an entry is a file or a directory is slow.
${file##*/} is needed to take the basename of the file because libs contains only file names and dir/lib*.so returns dir/liba.so, dir/libb.so, etc.; calling basename $file here is too slow since basename is not a bash builtin.
There was a problem hiding this comment.
If you meant that the whole conditional if [ "${libs[${file##*/}]}" ] should be removed, it is not the case because the main purpose of this loop is to filter out directories with libraries none of which are needed.
There was a problem hiding this comment.
@orivej I just forgot that the expanded glob would include the dirname; simple as that 😊.
|
I have replaced with because we do not want to add a directory for a library provided by a previous directory. |
|
After the last change, if there are both |
|
@orivej thanks for this; with all the recent improvements, |
orivej
force-pushed
the
optimize-rpath
branch
2 times, most recently
from
6b7552d
to
7ae3d26
Compare
|
@Ericson2314 Good idea! Done. @nh2 I have removed the piece of This shoud take care of the last 1/3 second you observed in #27609 (comment) |
| # to add the dynamic linker path to rpath. | ||
| n=$((n + 1)) | ||
| ;; | ||
| [^-]*.so | [^-]*.so.*) |
There was a problem hiding this comment.
Would ? in [^-]*.so | [^-]*.so.? replicate the original regex more accurately?
There was a problem hiding this comment.
No, it captures libraries with versions longer than one character, like libavformat.so.57.
There was a problem hiding this comment.
Ah, I misread the original regex.
| # First, find all -L... switches. | ||
| allParams=("${params[@]}" ${extra[@]}) | ||
| nParams=${#allParams[@]} | ||
| n=0 |
There was a problem hiding this comment.
declare -i n=0 I think will allow one to... [see below]
| case "$p" in | ||
| -L) | ||
| addToLibPath "${p2}" | ||
| n=$((n + 1)) |
There was a problem hiding this comment.
...make these n+=1 or let ++n. (Do ++n not n++ or risk a non-0 exit status---that bit me recently.)
There was a problem hiding this comment.
Great, thanks! Done.
orivej
force-pushed
the
optimize-rpath
branch
2 times, most recently
from
19a1698
to
2533ffe
Compare
I think so, yes. IIRC this pattern does occur in practice. |
|
This breaks the building of Or is it due to a bug in one of the earlier cross compiling commits? |
|
The cause of this is that |
|
#27831 fixes this. |
|
Currently a lot of Qt apolications/libraries aren't building because of linker errors that as far as I can tell are a result of this PR: https://hydra.nixos.org/build/58122426 If I run the build with |
|
Hmm, but I get that error on master and that PR is currently only in staging. EDIT: Ah, you've just merged that commit, but I was experiencing the error before that. |
|
@globin I can not reproduce this error on master, |
|
It could be a bug in #27831 |
|
Definitely occuring for me on master.. |
|
And here the failing build of kidletime on master on hydra: |
|
If there's one thing I've learned lately, to play with |
|
@globin This is a bug in Should it go to |
|
@orivej staging |
|
I'll test and fix this on staging |
This is used by cmake to edit rpath: foreach(file
"$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/lib/libKF5IdleTime.so.5.36.0"
"$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/lib/libKF5IdleTime.so.5"
"$ENV{DESTDIR}${CMAKE_INSTALL_PREFIX}/lib/libKF5IdleTime.so"
)
if(EXISTS "${file}" AND
NOT IS_SYMLINK "${file}")
file(RPATH_CHANGE
FILE "${file}"
OLD_RPATH "::::::::::::::::::::"
NEW_RPATH "/var/empty/local/lib")
if(CMAKE_INSTALL_DO_STRIP)
execute_process(COMMAND "/nix/store/d8dsyi10qz3dg7vlnrf2k44bs2yk0945-binutils-2.29/bin/strip" "${file}")
endif()
endif()
endforeach()https://gitlab.kitware.com/cmake/cmake/blob/b17b6dbb/Source/cmInstallTargetGenerator.cxx#L745 |
| -l?*) | ||
| libs["lib${p:2}.so"]=1 | ||
| ;; | ||
| "$NIX_STORE"/*.so | "$NIX_STORE"/*.so.*) |
There was a problem hiding this comment.
@orivej It just occurred to me, say somebody does $LD ... -o $out/lib/asdf.so? Should we check that [[ $prev != -* ]]?
There was a problem hiding this comment.
Some options do not require an argument, so this check would be incorrect. The idea behind the current option parser is that the path after -o and possibly other options, except -dynamic-linker and -plugin, can not start with $NIX_STORE and will not be added to rpath. Only when ld is invoked outside nix-build environment where $NIX_STORE is unset ld may add extra rpaths. This is worth fixing by adding -o alongside -plugin above.
There was a problem hiding this comment.
But I'm wrong in thinking that $out does not start with $NIX_STORE. I'll ignore the argument after -o in the next pull request for #28021 (comment)
|
I have now found how I can backport this fix to First define customLdWrapper = pkgs.stdenv.mkDerivation {
name = "custom-ld-wrapper";
# The wrapper scripts use 'cat' and 'grep', so we may need coreutils.
# The variable being set here exposes it as a bash variable
# in `buildCommand`, where `substituteAll` will pick it up.
coreutils_bin = pkgs.lib.getBin pkgs.coreutils;
buildCommand = let ldWrapperPath = ./fast-ld-wrapper.sh; in ''
mkdir -p $out/bin $out/nix-support
wrap() {
local dst="$1"
local wrapper="$2"
export prog="$3"
substituteAll "$wrapper" "$out/bin/$dst"
chmod +x "$out/bin/$dst"
}
wrap ld.gold ${ldWrapperPath} ${pkgs.lib.getBin pkgs.binutils}/bin/ld.gold
'' +
# We need all the files available that the real wrapper has, so we copy them over.
''
cp -r ${pkgs.gcc}/nix-support $out
'';
};where But it turns out it is not enough to just put this package into I had to pass it as preConfigure = ''
export FIXED_LD_WRAPPER_PATH=${pkgs.customLdWrapper}/bin
'';
# So that it also works in a cabal/Setup.hs build in nix-shell
shellHook = ''
export FIXED_LD_WRAPPER_PATH=${pkgs.customLdWrapper}/bin
'';and then define a custom import Distribution.Simple
import Distribution.Simple.Setup
import Distribution.Simple.LocalBuildInfo (extraConfigArgs, configFlags)
import System.Environment (lookupEnv)
main = defaultMainWithHooks simpleUserHooks{
confHook = \(genericPackageDescription, hookedBuildInfo) hookConfigFlags -> do
-- TODO Remove this, the customLdWrapper nix derivation and
-- all occurrences of FIXED_LD_WRAPPER_PATH once we are
-- on nixpkgs 17.09.
maybeLdPath <- lookupEnv "FIXED_LD_WRAPPER_PATH"
let patchedConfigFlags = case maybeLdPath of
Nothing -> hookConfigFlags
Just path -> hookConfigFlags{ configProgramArgs = prependIfNotPresent ("ghc", ["-optl-B" ++ path]) (configProgramArgs hookConfigFlags) }
where
prependIfNotPresent :: (Eq a) => a -> [a] -> [a]
prependIfNotPresent x xs = if x `elem` xs then xs else x:xs
localBuildInfo <- (confHook simpleUserHooks) (genericPackageDescription, hookedBuildInfo) patchedConfigFlags
putStrLn $
"Setup.hs: Adding flag to tell GHC to use fixed nix gold ld-wrapper; configure arguments are: "
++ show (configProgramArgs $ configFlags localBuildInfo)
return localBuildInfo
}and set That sped up my |
|
You could also override the whole cc-wrapper part only for the packages that you care for, i.e. build only those with a stdenv containing the modified wrapper. |
Motivation for this change
The time to expand rpath was proportional to the number of -L flags times the number of -l flags. This change makes it proportional to their sum (assuming constant number of files in each directory in an -L flag).
Issue reported by @nh2 at #27609 (comment)
I'm going to test this soon.
Things done
Please check what applies. Note that these are not hard requirements but mereley serve as information for reviewers.
(nix.useSandbox on NixOS,
or option
build-use-sandboxinnix.confon non-NixOS)
nix-shell -p nox --run "nox-review wip"./result/bin/)