diff options
author | Todd Gamblin <tgamblin@llnl.gov> | 2024-09-21 17:25:16 -0700 |
---|---|---|
committer | Todd Gamblin <tgamblin@llnl.gov> | 2024-09-27 05:27:26 -0700 |
commit | 2613a14c43b11ab92181bdd7ba57cd5b99f128a8 (patch) | |
tree | 89b0c34094cdebf382d321376391240342c922d7 | |
parent | a76a48c42e8d6657f9dcb0500bff574651f317d9 (diff) | |
download | spack-2613a14c43b11ab92181bdd7ba57cd5b99f128a8.tar.gz spack-2613a14c43b11ab92181bdd7ba57cd5b99f128a8.tar.bz2 spack-2613a14c43b11ab92181bdd7ba57cd5b99f128a8.tar.xz spack-2613a14c43b11ab92181bdd7ba57cd5b99f128a8.zip |
`cc`: ensure that RPATHs passed to linker are unique
macOS Sequoia's linker will complain if RPATHs on the CLI are specified more than once.
To avoid errors due to this, make `cc` only append unique RPATHs to the final args list.
This required a few improvements to the logic in `cc`:
1. List functions in `cc` didn't have any way to append unique elements to a list. Add a
`contains()` shell function that works like our other list functions. Use it to implement
an optional `"unique"` argument to `append()` and an `extend_unique()`. Use that to add
RPATHs to the `args_list`.
2. In the pure `ld` case, we weren't actually parsing `RPATH` arguments separately as we
do for `ccld`. Fix this by adding *another* nested case statement for raw `RPATH`
parsing. There are now 3 places where we deal with `-rpath` and friends, but I don't
see a great way to unify them, as `-Wl,`, `-Xlinker`, and raw `-rpath` arguments are
all ever so slightly different.
3. Fix ordering of assertions to make `pytest` diffs more intelligible. The meaning of
`+` and `-` in diffs changed in `pytest` 6.0 and the "preferred" order for assertions
became `assert actual == expected` instead of the other way around.
Signed-off-by: Todd Gamblin <tgamblin@llnl.gov>
-rwxr-xr-x | lib/spack/env/cc | 105 | ||||
-rw-r--r-- | lib/spack/spack/test/cc.py | 35 |
2 files changed, 114 insertions, 26 deletions
diff --git a/lib/spack/env/cc b/lib/spack/env/cc index 44f8b9316a..45e5e26269 100755 --- a/lib/spack/env/cc +++ b/lib/spack/env/cc @@ -101,10 +101,9 @@ setsep() { esac } -# prepend LISTNAME ELEMENT [SEP] +# prepend LISTNAME ELEMENT # -# Prepend ELEMENT to the list stored in the variable LISTNAME, -# assuming the list is separated by SEP. +# Prepend ELEMENT to the list stored in the variable LISTNAME. # Handles empty lists and single-element lists. prepend() { varname="$1" @@ -119,18 +118,39 @@ prepend() { fi } -# append LISTNAME ELEMENT [SEP] +# contains LISTNAME ELEMENT # -# Append ELEMENT to the list stored in the variable LISTNAME, -# assuming the list is separated by SEP. +# Test whether LISTNAME contains ELEMENT. +# Set $? to 1 if LISTNAME does not contain ELEMENT. +# Set $? to 0 if LISTNAME does not contain ELEMENT. +contains() { + varname="$1" + elt="$2" + + setsep "$varname" + + # the list may: 1) only contain the element, 2) start with the element, + # 3) contain the element in the middle, or 4) end wtih the element. + eval "[ \"\${$varname}\" = \"$elt\" ]" \ + || eval "[ \"\${$varname#${elt}${sep}}\" != \"\${$varname}\" ]" \ + || eval "[ \"\${$varname#*${sep}${elt}${sep}}\" != \"\${$varname}\" ]" \ + || eval "[ \"\${$varname%${sep}${elt}}\" != \"\${$varname}\" ]" +} + +# append LISTNAME ELEMENT [unique] +# +# Append ELEMENT to the list stored in the variable LISTNAME. # Handles empty lists and single-element lists. +# +# If the third argument is provided and if it is the string 'unique', +# this will not append if ELEMENT is already in the list LISTNAME. append() { varname="$1" elt="$2" if empty "$varname"; then eval "$varname=\"\${elt}\"" - else + elif [ "$3" != "unique" ] || ! contains "$varname" "$elt" ; then # Get the appropriate separator for the list we're appending to. setsep "$varname" eval "$varname=\"\${$varname}${sep}\${elt}\"" @@ -148,10 +168,21 @@ extend() { if [ "$sep" != " " ]; then IFS="$sep" fi - eval "for elt in \${$2}; do append $1 \"$3\${elt}\"; done" + eval "for elt in \${$2}; do append $1 \"$3\${elt}\" ${_append_args}; done" unset IFS } +# extend_unique LISTNAME1 LISTNAME2 [PREFIX] +# +# Append the elements stored in the variable LISTNAME2 to the list +# stored in LISTNAME1, if they are not already present. +# If PREFIX is provided, prepend it to each element. +extend_unique() { + _append_args="unique" + extend "$@" + unset _append_args +} + # preextend LISTNAME1 LISTNAME2 [PREFIX] # # Prepend the elements stored in the list at LISTNAME2 @@ -682,7 +713,32 @@ categorize_arguments() { "$dtags_to_strip") ;; *) - append return_other_args_list "$1" + # if mode is not ld, we can just add to other args + if [ "$mode" != "ld" ]; then + append return_other_args_list "$1" + shift + continue + fi + + # if we're in linker mode, we need to parse raw RPATH args + case "$1" in + -rpath=*) + arg="${1#-rpath=}" + append_path_lists return_rpath_dirs_list "$arg" + ;; + --rpath=*) + arg="${1#--rpath=}" + append_path_lists return_rpath_dirs_list "$arg" + ;; + -rpath|--rpath) + shift + [ $# -eq 0 ] && break # ignore -rpath without value + append_path_lists return_rpath_dirs_list "$1" + ;; + *) + append return_other_args_list "$1" + ;; + esac ;; esac shift @@ -890,35 +946,34 @@ extend args_list system_spack_flags_lib_dirs_list "-L" extend args_list system_lib_dirs_list "-L" # RPATHs arguments +rpath_prefix="" case "$mode" in ccld) if [ -n "$dtags_to_add" ] ; then append args_list "$linker_arg$dtags_to_add" fi - extend args_list spack_store_spack_flags_rpath_dirs_list "$rpath" - extend args_list spack_store_rpath_dirs_list "$rpath" - - extend args_list spack_flags_rpath_dirs_list "$rpath" - extend args_list rpath_dirs_list "$rpath" - - extend args_list system_spack_flags_rpath_dirs_list "$rpath" - extend args_list system_rpath_dirs_list "$rpath" + rpath_prefix="$rpath" ;; ld) if [ -n "$dtags_to_add" ] ; then append args_list "$dtags_to_add" fi - extend args_list spack_store_spack_flags_rpath_dirs_list "-rpath${lsep}" - extend args_list spack_store_rpath_dirs_list "-rpath${lsep}" - - extend args_list spack_flags_rpath_dirs_list "-rpath${lsep}" - extend args_list rpath_dirs_list "-rpath${lsep}" - - extend args_list system_spack_flags_rpath_dirs_list "-rpath${lsep}" - extend args_list system_rpath_dirs_list "-rpath${lsep}" + rpath_prefix="-rpath${lsep}" ;; esac +# if mode is ccld or ld, extend RPATH lists with the prefix determined above +if [ -n "$rpath_prefix" ]; then + extend_unique args_list spack_store_spack_flags_rpath_dirs_list "$rpath_prefix" + extend_unique args_list spack_store_rpath_dirs_list "$rpath_prefix" + + extend_unique args_list spack_flags_rpath_dirs_list "$rpath_prefix" + extend_unique args_list rpath_dirs_list "$rpath_prefix" + + extend_unique args_list system_spack_flags_rpath_dirs_list "$rpath_prefix" + extend_unique args_list system_rpath_dirs_list "$rpath_prefix" +fi + # Other arguments from the input command extend args_list other_args_list extend args_list spack_flags_other_args_list diff --git a/lib/spack/spack/test/cc.py b/lib/spack/spack/test/cc.py index 4a394680f4..dc08fdfb04 100644 --- a/lib/spack/spack/test/cc.py +++ b/lib/spack/spack/test/cc.py @@ -199,7 +199,7 @@ def check_args(cc, args, expected): """ with set_env(SPACK_TEST_COMMAND="dump-args"): cc_modified_args = cc(*args, output=str).strip().split("\n") - assert expected == cc_modified_args + assert cc_modified_args == expected def check_args_contents(cc, args, must_contain, must_not_contain): @@ -354,6 +354,39 @@ def test_fc_flags(wrapper_environment, wrapper_flags): ) +def test_ld_flags_with_redundant_rpaths(wrapper_environment, wrapper_flags): + check_args( + ld, + test_args + test_rpaths, # ensure thesee are made unique + ["ld"] + + test_include_paths + + test_library_paths + + ["--disable-new-dtags"] + + test_rpaths + + test_args_without_paths + + spack_ldlibs, + ) + + +def test_cc_flags_with_redundant_rpaths(wrapper_environment, wrapper_flags): + check_args( + cc, + test_args + test_wl_rpaths + test_wl_rpaths, # ensure thesee are made unique + [real_cc] + + target_args + + test_include_paths + + ["-Lfoo"] + + test_library_paths + + ["-Wl,--disable-new-dtags"] + + test_wl_rpaths + + test_args_without_paths + + spack_cppflags + + spack_cflags + + ["-Wl,--gc-sections"] + + spack_ldlibs, + ) + + def test_always_cflags(wrapper_environment, wrapper_flags): with set_env(SPACK_ALWAYS_CFLAGS="-always1 -always2"): check_args( |