From 7dc485d2887c70e3e1ebdc6f680cda926444da72 Mon Sep 17 00:00:00 2001 From: Andrey Parfenov Date: Sun, 18 Jun 2023 23:07:08 +0200 Subject: cc: Ensure that user-specified flags take precedence over others (#37376) Spack flags supplied by users should supersede flags from package build systems and other places in Spack. However, Spack currently adds user-supplied flags to the beginning of the compile line, which means that in some cases build system flags will supersede user-supplied ones. The right place to add a flag to ensure it has highest precedence for the compiler really depends on the type of flag. For example, search paths like `-L` and `-I` are examined in order, so adding them first is highest precedence. Compilers take the *last* occurrence of optimization flags like `-O2`, so those should be placed *after* other such flags. Shim libraries with `-l` should go *before* other libraries on the command line, so we want user-supplied libs to go first, etc. `lib/spack/env/cc` already knows how to split arguments into categories like `libs_list`, `rpath_dirs_list`, etc., so we can leverage that functionality to merge user flags into the arg list correctly. The general rules for injected flags are: 1. All `-L`, `-I`, `-isystem`, `-l`, and `*-rpath` flags from `spack_flags_*` to appear before their regular counterparts. 2. All other flags ordered with the ones from flags after their regular counterparts, i.e. `other_flags` before `spack_flags_other_flags` - [x] Generalize argument categorization into its own function in the `cc` shell script - [x] Apply the same splitting logic to injected flags and flags from the original compile line. - [x] Use the resulting flag lists to merge user- and build-system-supplied flags by category. - [x] Add tests. Signed-off-by: Andrey Parfenov Co-authored-by: iermolae --- lib/spack/env/cc | 418 +++++++++++++++++++++++++-------------------- lib/spack/spack/test/cc.py | 33 ++-- 2 files changed, 256 insertions(+), 195 deletions(-) (limited to 'lib') diff --git a/lib/spack/env/cc b/lib/spack/env/cc index a647602cad..360a0de8bc 100755 --- a/lib/spack/env/cc +++ b/lib/spack/env/cc @@ -416,30 +416,14 @@ input_command="$*" # The lists are all bell-separated to be as flexible as possible, as their # contents may come from the command line, from ' '-separated lists, # ':'-separated lists, etc. -include_dirs_list="" -lib_dirs_list="" -rpath_dirs_list="" -system_include_dirs_list="" -system_lib_dirs_list="" -system_rpath_dirs_list="" -isystem_system_include_dirs_list="" -isystem_include_dirs_list="" -libs_list="" -other_args_list="" - -# Global state for keeping track of -Wl,-rpath -Wl,/path -wl_expect_rpath=no - -# Same, but for -Xlinker -rpath -Xlinker /path -xlinker_expect_rpath=no parse_Wl() { while [ $# -ne 0 ]; do if [ "$wl_expect_rpath" = yes ]; then if system_dir "$1"; then - append system_rpath_dirs_list "$1" + append return_system_rpath_dirs_list "$1" else - append rpath_dirs_list "$1" + append return_rpath_dirs_list "$1" fi wl_expect_rpath=no else @@ -449,9 +433,9 @@ parse_Wl() { if [ -z "$arg" ]; then shift; continue elif system_dir "$arg"; then - append system_rpath_dirs_list "$arg" + append return_system_rpath_dirs_list "$arg" else - append rpath_dirs_list "$arg" + append return_rpath_dirs_list "$arg" fi ;; --rpath=*) @@ -459,9 +443,9 @@ parse_Wl() { if [ -z "$arg" ]; then shift; continue elif system_dir "$arg"; then - append system_rpath_dirs_list "$arg" + append return_system_rpath_dirs_list "$arg" else - append rpath_dirs_list "$arg" + append return_rpath_dirs_list "$arg" fi ;; -rpath|--rpath) @@ -475,7 +459,7 @@ parse_Wl() { return 1 ;; *) - append other_args_list "-Wl,$1" + append return_other_args_list "-Wl,$1" ;; esac fi @@ -483,177 +467,210 @@ parse_Wl() { done } +categorize_arguments() { -while [ $# -ne 0 ]; do + unset IFS - # an RPATH to be added after the case statement. - rp="" + return_other_args_list="" + return_isystem_was_used="" + return_isystem_system_include_dirs_list="" + return_isystem_include_dirs_list="" + return_system_include_dirs_list="" + return_include_dirs_list="" + return_system_lib_dirs_list="" + return_lib_dirs_list="" + return_system_rpath_dirs_list="" + return_rpath_dirs_list="" - # Multiple consecutive spaces in the command line can - # result in blank arguments - if [ -z "$1" ]; then - shift - continue - fi + # Global state for keeping track of -Wl,-rpath -Wl,/path + wl_expect_rpath=no - if [ -n "${SPACK_COMPILER_FLAGS_KEEP}" ] ; then - # NOTE: the eval is required to allow `|` alternatives inside the variable - eval "\ - case \"\$1\" in - $SPACK_COMPILER_FLAGS_KEEP) - append other_args_list \"\$1\" - shift - continue - ;; - esac - " - fi - # the replace list is a space-separated list of pipe-separated pairs, - # the first in each pair is the original prefix to be matched, the - # second is the replacement prefix - if [ -n "${SPACK_COMPILER_FLAGS_REPLACE}" ] ; then - for rep in ${SPACK_COMPILER_FLAGS_REPLACE} ; do - before=${rep%|*} - after=${rep#*|} + # Same, but for -Xlinker -rpath -Xlinker /path + xlinker_expect_rpath=no + + while [ $# -ne 0 ]; do + + # an RPATH to be added after the case statement. + rp="" + + # Multiple consecutive spaces in the command line can + # result in blank arguments + if [ -z "$1" ]; then + shift + continue + fi + + if [ -n "${SPACK_COMPILER_FLAGS_KEEP}" ] ; then + # NOTE: the eval is required to allow `|` alternatives inside the variable eval "\ - stripped=\"\${1##$before}\" + case \"\$1\" in + $SPACK_COMPILER_FLAGS_KEEP) + append return_other_args_list \"\$1\" + shift + continue + ;; + esac " - if [ "$stripped" = "$1" ] ; then - continue - fi + fi + # the replace list is a space-separated list of pipe-separated pairs, + # the first in each pair is the original prefix to be matched, the + # second is the replacement prefix + if [ -n "${SPACK_COMPILER_FLAGS_REPLACE}" ] ; then + for rep in ${SPACK_COMPILER_FLAGS_REPLACE} ; do + before=${rep%|*} + after=${rep#*|} + eval "\ + stripped=\"\${1##$before}\" + " + if [ "$stripped" = "$1" ] ; then + continue + fi - replaced="$after$stripped" + replaced="$after$stripped" - # it matched, remove it - shift + # it matched, remove it + shift - if [ -z "$replaced" ] ; then - # completely removed, continue OUTER loop - continue 2 - fi + if [ -z "$replaced" ] ; then + # completely removed, continue OUTER loop + continue 2 + fi - # re-build argument list with replacement - set -- "$replaced" "$@" - done - fi + # re-build argument list with replacement + set -- "$replaced" "$@" + done + fi - case "$1" in - -isystem*) - arg="${1#-isystem}" - isystem_was_used=true - if [ -z "$arg" ]; then shift; arg="$1"; fi - if system_dir "$arg"; then - append isystem_system_include_dirs_list "$arg" - else - append isystem_include_dirs_list "$arg" - fi - ;; - -I*) - arg="${1#-I}" - if [ -z "$arg" ]; then shift; arg="$1"; fi - if system_dir "$arg"; then - append system_include_dirs_list "$arg" - else - append include_dirs_list "$arg" - fi - ;; - -L*) - arg="${1#-L}" - if [ -z "$arg" ]; then shift; arg="$1"; fi - if system_dir "$arg"; then - append system_lib_dirs_list "$arg" - else - append lib_dirs_list "$arg" - fi - ;; - -l*) - # -loopopt=0 is generated erroneously in autoconf <= 2.69, - # and passed by ifx to the linker, which confuses it with a - # library. Filter it out. - # TODO: generalize filtering of args with an env var, so that - # TODO: we do not have to special case this here. - if { [ "$mode" = "ccld" ] || [ $mode = "ld" ]; } \ - && [ "$1" != "${1#-loopopt}" ]; then + case "$1" in + -isystem*) + arg="${1#-isystem}" + return_isystem_was_used=true + if [ -z "$arg" ]; then shift; arg="$1"; fi + if system_dir "$arg"; then + append return_isystem_system_include_dirs_list "$arg" + else + append return_isystem_include_dirs_list "$arg" + fi + ;; + -I*) + arg="${1#-I}" + if [ -z "$arg" ]; then shift; arg="$1"; fi + if system_dir "$arg"; then + append return_system_include_dirs_list "$arg" + else + append return_include_dirs_list "$arg" + fi + ;; + -L*) + arg="${1#-L}" + if [ -z "$arg" ]; then shift; arg="$1"; fi + if system_dir "$arg"; then + append return_system_lib_dirs_list "$arg" + else + append return_lib_dirs_list "$arg" + fi + ;; + -l*) + # -loopopt=0 is generated erroneously in autoconf <= 2.69, + # and passed by ifx to the linker, which confuses it with a + # library. Filter it out. + # TODO: generalize filtering of args with an env var, so that + # TODO: we do not have to special case this here. + if { [ "$mode" = "ccld" ] || [ $mode = "ld" ]; } \ + && [ "$1" != "${1#-loopopt}" ]; then + shift + continue + fi + arg="${1#-l}" + if [ -z "$arg" ]; then shift; arg="$1"; fi + append return_other_args_list "-l$arg" + ;; + -Wl,*) + IFS=, + if ! parse_Wl ${1#-Wl,}; then + append return_other_args_list "$1" + fi + unset IFS + ;; + -Xlinker) shift - continue - fi - arg="${1#-l}" - if [ -z "$arg" ]; then shift; arg="$1"; fi - append other_args_list "-l$arg" - ;; - -Wl,*) - IFS=, - if ! parse_Wl ${1#-Wl,}; then - append other_args_list "$1" - fi - unset IFS - ;; - -Xlinker) - shift - if [ $# -eq 0 ]; then - # -Xlinker without value: let the compiler error about it. - append other_args_list -Xlinker - xlinker_expect_rpath=no - break - elif [ "$xlinker_expect_rpath" = yes ]; then - # Register the path of -Xlinker -rpath -Xlinker - if system_dir "$1"; then - append system_rpath_dirs_list "$1" + if [ $# -eq 0 ]; then + # -Xlinker without value: let the compiler error about it. + append return_other_args_list -Xlinker + xlinker_expect_rpath=no + break + elif [ "$xlinker_expect_rpath" = yes ]; then + # Register the path of -Xlinker -rpath -Xlinker + if system_dir "$1"; then + append return_system_rpath_dirs_list "$1" + else + append return_rpath_dirs_list "$1" + fi + xlinker_expect_rpath=no else - append rpath_dirs_list "$1" + case "$1" in + -rpath=*) + arg="${1#-rpath=}" + if system_dir "$arg"; then + append return_system_rpath_dirs_list "$arg" + else + append return_rpath_dirs_list "$arg" + fi + ;; + --rpath=*) + arg="${1#--rpath=}" + if system_dir "$arg"; then + append return_system_rpath_dirs_list "$arg" + else + append return_rpath_dirs_list "$arg" + fi + ;; + -rpath|--rpath) + xlinker_expect_rpath=yes + ;; + "$dtags_to_strip") + ;; + *) + append return_other_args_list -Xlinker + append return_other_args_list "$1" + ;; + esac fi - xlinker_expect_rpath=no - else - case "$1" in - -rpath=*) - arg="${1#-rpath=}" - if system_dir "$arg"; then - append system_rpath_dirs_list "$arg" - else - append rpath_dirs_list "$arg" - fi - ;; - --rpath=*) - arg="${1#--rpath=}" - if system_dir "$arg"; then - append system_rpath_dirs_list "$arg" - else - append rpath_dirs_list "$arg" - fi - ;; - -rpath|--rpath) - xlinker_expect_rpath=yes - ;; - "$dtags_to_strip") - ;; - *) - append other_args_list -Xlinker - append other_args_list "$1" - ;; - esac - fi - ;; - "$dtags_to_strip") - ;; - *) - append other_args_list "$1" - ;; - esac - shift -done + ;; + "$dtags_to_strip") + ;; + *) + append return_other_args_list "$1" + ;; + esac + shift + done -# We found `-Xlinker -rpath` but no matching value `-Xlinker /path`. Just append -# `-Xlinker -rpath` again and let the compiler or linker handle the error during arg -# parsing. -if [ "$xlinker_expect_rpath" = yes ]; then - append other_args_list -Xlinker - append other_args_list -rpath -fi + # We found `-Xlinker -rpath` but no matching value `-Xlinker /path`. Just append + # `-Xlinker -rpath` again and let the compiler or linker handle the error during arg + # parsing. + if [ "$xlinker_expect_rpath" = yes ]; then + append return_other_args_list -Xlinker + append return_other_args_list -rpath + fi -# Same, but for -Wl flags. -if [ "$wl_expect_rpath" = yes ]; then - append other_args_list -Wl,-rpath -fi + # Same, but for -Wl flags. + if [ "$wl_expect_rpath" = yes ]; then + append return_other_args_list -Wl,-rpath + fi +} + +categorize_arguments "$@" + include_dirs_list="$return_include_dirs_list" + lib_dirs_list="$return_lib_dirs_list" + rpath_dirs_list="$return_rpath_dirs_list" + system_include_dirs_list="$return_system_include_dirs_list" + system_lib_dirs_list="$return_system_lib_dirs_list" + system_rpath_dirs_list="$return_system_rpath_dirs_list" + isystem_was_used="$return_isystem_was_used" + isystem_system_include_dirs_list="$return_isystem_system_include_dirs_list" + isystem_include_dirs_list="$return_isystem_include_dirs_list" + other_args_list="$return_other_args_list" # # Add flags from Spack's cppflags, cflags, cxxflags, fcflags, fflags, and @@ -673,12 +690,14 @@ elif [ "$SPACK_ADD_DEBUG_FLAGS" = "custom" ]; then extend flags_list SPACK_DEBUG_FLAGS fi +spack_flags_list="" + # Fortran flags come before CPPFLAGS case "$mode" in cc|ccld) case $lang_flags in F) - extend flags_list SPACK_FFLAGS + extend spack_flags_list SPACK_FFLAGS ;; esac ;; @@ -687,7 +706,7 @@ esac # C preprocessor flags come before any C/CXX flags case "$mode" in cpp|as|cc|ccld) - extend flags_list SPACK_CPPFLAGS + extend spack_flags_list SPACK_CPPFLAGS ;; esac @@ -697,10 +716,10 @@ case "$mode" in cc|ccld) case $lang_flags in C) - extend flags_list SPACK_CFLAGS + extend spack_flags_list SPACK_CFLAGS ;; CXX) - extend flags_list SPACK_CXXFLAGS + extend spack_flags_list SPACK_CXXFLAGS ;; esac @@ -712,10 +731,25 @@ esac # Linker flags case "$mode" in ld|ccld) - extend flags_list SPACK_LDFLAGS + extend spack_flags_list SPACK_LDFLAGS ;; esac +IFS="$lsep" + categorize_arguments $spack_flags_list +unset IFS + spack_flags_include_dirs_list="$return_include_dirs_list" + spack_flags_lib_dirs_list="$return_lib_dirs_list" + spack_flags_rpath_dirs_list="$return_rpath_dirs_list" + spack_flags_system_include_dirs_list="$return_system_include_dirs_list" + spack_flags_system_lib_dirs_list="$return_system_lib_dirs_list" + spack_flags_system_rpath_dirs_list="$return_system_rpath_dirs_list" + spack_flags_isystem_was_used="$return_isystem_was_used" + spack_flags_isystem_system_include_dirs_list="$return_isystem_system_include_dirs_list" + spack_flags_isystem_include_dirs_list="$return_isystem_include_dirs_list" + spack_flags_other_args_list="$return_other_args_list" + + # On macOS insert headerpad_max_install_names linker flag if [ "$mode" = ld ] || [ "$mode" = ccld ]; then if [ "${SPACK_SHORT_SPEC#*darwin}" != "${SPACK_SHORT_SPEC}" ]; then @@ -741,6 +775,8 @@ if [ "$mode" = ccld ] || [ "$mode" = ld ]; then extend lib_dirs_list SPACK_LINK_DIRS fi +libs_list="" + # add RPATHs if we're in in any linking mode case "$mode" in ld|ccld) @@ -769,12 +805,16 @@ args_list="$flags_list" # Insert include directories just prior to any system include directories # NOTE: adding ${lsep} to the prefix here turns every added element into two +extend args_list spack_flags_include_dirs_list "-I" extend args_list include_dirs_list "-I" +extend args_list spack_flags_isystem_include_dirs_list "-isystem${lsep}" extend args_list isystem_include_dirs_list "-isystem${lsep}" case "$mode" in cpp|cc|as|ccld) - if [ "$isystem_was_used" = "true" ]; then + if [ "$spack_flags_isystem_was_used" = "true" ]; then + extend args_list SPACK_INCLUDE_DIRS "-isystem${lsep}" + elif [ "$isystem_was_used" = "true" ]; then extend args_list SPACK_INCLUDE_DIRS "-isystem${lsep}" else extend args_list SPACK_INCLUDE_DIRS "-I" @@ -782,11 +822,15 @@ case "$mode" in ;; esac +extend args_list spack_flags_system_include_dirs_list -I extend args_list system_include_dirs_list -I +extend args_list spack_flags_isystem_system_include_dirs_list "-isystem${lsep}" extend args_list isystem_system_include_dirs_list "-isystem${lsep}" # Library search paths +extend args_list spack_flags_lib_dirs_list "-L" extend args_list lib_dirs_list "-L" +extend args_list spack_flags_system_lib_dirs_list "-L" extend args_list system_lib_dirs_list "-L" # RPATHs arguments @@ -795,20 +839,25 @@ case "$mode" in if [ -n "$dtags_to_add" ] ; then append args_list "$linker_arg$dtags_to_add" fi + extend args_list spack_flags_rpath_dirs_list "$rpath" extend args_list rpath_dirs_list "$rpath" + extend args_list spack_flags_system_rpath_dirs_list "$rpath" extend args_list system_rpath_dirs_list "$rpath" ;; ld) if [ -n "$dtags_to_add" ] ; then append args_list "$dtags_to_add" fi + extend args_list spack_flags_rpath_dirs_list "-rpath${lsep}" extend args_list rpath_dirs_list "-rpath${lsep}" + extend args_list spack_flags_system_rpath_dirs_list "-rpath${lsep}" extend args_list system_rpath_dirs_list "-rpath${lsep}" ;; esac # Other arguments from the input command extend args_list other_args_list +extend args_list spack_flags_other_args_list # Inject SPACK_LDLIBS, if supplied extend args_list libs_list "-l" @@ -864,3 +913,4 @@ fi # Execute the full command, preserving spaces with IFS set # to the alarm bell separator. IFS="$lsep"; exec $full_command_list + diff --git a/lib/spack/spack/test/cc.py b/lib/spack/spack/test/cc.py index 7dc02d01f9..9c153c2d06 100644 --- a/lib/spack/spack/test/cc.py +++ b/lib/spack/spack/test/cc.py @@ -173,7 +173,7 @@ def wrapper_environment(working_env): SPACK_DTAGS_TO_ADD="--disable-new-dtags", SPACK_DTAGS_TO_STRIP="--enable-new-dtags", SPACK_COMPILER_FLAGS_KEEP="", - SPACK_COMPILER_FLAGS_REPLACE="-Werror*", + SPACK_COMPILER_FLAGS_REPLACE="-Werror*|", ): yield @@ -278,8 +278,8 @@ def test_ld_flags(wrapper_environment, wrapper_flags): ld, test_args, ["ld"] - + spack_ldflags + test_include_paths + + [spack_ldflags[i] + spack_ldflags[i + 1] for i in range(0, len(spack_ldflags), 2)] + test_library_paths + ["--disable-new-dtags"] + test_rpaths @@ -293,10 +293,10 @@ def test_cpp_flags(wrapper_environment, wrapper_flags): cpp, test_args, ["cpp"] - + spack_cppflags + test_include_paths + test_library_paths - + test_args_without_paths, + + test_args_without_paths + + spack_cppflags, ) @@ -306,10 +306,14 @@ def test_cc_flags(wrapper_environment, wrapper_flags): test_args, [real_cc] + target_args + + test_include_paths + + [spack_ldflags[i] + spack_ldflags[i + 1] for i in range(0, len(spack_ldflags), 2)] + + test_library_paths + + ["-Wl,--disable-new-dtags"] + + test_wl_rpaths + + test_args_without_paths + spack_cppflags + spack_cflags - + spack_ldflags - + common_compile_args + spack_ldlibs, ) @@ -320,10 +324,13 @@ def test_cxx_flags(wrapper_environment, wrapper_flags): test_args, [real_cc] + target_args + + test_include_paths + + [spack_ldflags[i] + spack_ldflags[i + 1] for i in range(0, len(spack_ldflags), 2)] + + test_library_paths + + ["-Wl,--disable-new-dtags"] + + test_wl_rpaths + + test_args_without_paths + spack_cppflags - + spack_cxxflags - + spack_ldflags - + common_compile_args + spack_ldlibs, ) @@ -334,10 +341,14 @@ def test_fc_flags(wrapper_environment, wrapper_flags): test_args, [real_cc] + target_args + + test_include_paths + + [spack_ldflags[i] + spack_ldflags[i + 1] for i in range(0, len(spack_ldflags), 2)] + + test_library_paths + + ["-Wl,--disable-new-dtags"] + + test_wl_rpaths + + test_args_without_paths + spack_fflags + spack_cppflags - + spack_ldflags - + common_compile_args + spack_ldlibs, ) -- cgit v1.2.3-60-g2f50