summaryrefslogtreecommitdiff
diff options
context:
space:
mode:
authorAndrey Parfenov <andrey.parfenov@intel.com>2023-06-18 23:07:08 +0200
committerGitHub <noreply@github.com>2023-06-18 14:07:08 -0700
commit7dc485d2887c70e3e1ebdc6f680cda926444da72 (patch)
tree192308fa3285ff4ce2aa5fcefbfe0fc8a3b191af
parent5c6c3b403bc4a8cd08b4e9303748933f3f79f5f7 (diff)
downloadspack-7dc485d2887c70e3e1ebdc6f680cda926444da72.tar.gz
spack-7dc485d2887c70e3e1ebdc6f680cda926444da72.tar.bz2
spack-7dc485d2887c70e3e1ebdc6f680cda926444da72.tar.xz
spack-7dc485d2887c70e3e1ebdc6f680cda926444da72.zip
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 <andrey.parfenov@intel.com> Co-authored-by: iermolae <igor.ermolaev@intel.com>
-rwxr-xr-xlib/spack/env/cc418
-rw-r--r--lib/spack/spack/test/cc.py33
2 files changed, 256 insertions, 195 deletions
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 <other args> -Xlinker <path>
- 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 <other args> -Xlinker <path>
+ 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,
)