[yugabyte#19378] Fix duplicate RPATH arguments in linker command lines

Summary: Fixing duplicate `-Wl,-rpath,...` arguments provided to the linker. It became a warning with the new linker on macOS 13.6 (`ld: warning: duplicate -rpath` messages). Adding deduplication in compiler-wrapper.sh that should make sure that the eventual command line does not have any duplicate RPATHs, even if the build scripts are modified so that the duplicates reappear. Also, by setting the YB_FAIL_ON_DUPLICATE_RPATH environment variable, we can cause a compilation failure if duplicate RPATH options are ever given to the linker. Also various other cleanup in the build system: - Remove static analyzer support from compiler wrapper. - Better naming for third-party directory variables in CMakeLists.txt. - Actually use the ENFORCE_OUT_OF_SOURCE_BUILD function in CMakeLists.txt. This function was defined but not used. - Removing compiler wrapper code for making RPATHs relative, because it was incomplete anyway (did not work for Postgres), and we don't really ever move code to a different directory just to run tests. For release packaging, we use a different approach for making RPATHs relative. - Remove CMake*.log files before running CMake so that if we have to show their contents, we don't show stale information. Also updating third-party dependencies: https://github.com/yugabyte/yugabyte-db-thirdparty/compare/057e0a1188230bc58a4d71394a92bec157dce13f..09031a51d0fd907fa79420954f46da9e1ede2829 Test Plan: Jenkins Manual test on Linux and macOS: YB_FAIL_ON_DUPLICATE_RPATH=1 ./yb_build.sh Reviewers: asrivastava, hsunder Reviewed By: hsunder Subscribers: ybase, yql Differential Revision: https://phorge.dev.yugabyte.com/D28966
norachoukri · Oct 3, 2023 · 21b30b2 · 21b30b2
1 parent 90cc6dc
commit 21b30b2
Show file tree

Hide file tree

Showing 12 changed files with 227 additions and 296 deletions.
diff --git a/CMakeLists.txt b/CMakeLists.txt
@@ -53,6 +53,7 @@ message("-- PRECOMPILED HEADERS:    ${YB_PCH_ON}")
 set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_SOURCE_DIR}/cmake_modules")
 set(CMAKE_MODULE_PATH ${CMAKE_MODULE_PATH} "${CMAKE_SOURCE_DIR}/cmake_standard_modules")
 include(YugabyteFunctions)
+ENFORCE_OUT_OF_SOURCE_BUILD()
 yb_initialize_constants()
 include(YugabyteTesting)
 
@@ -232,9 +233,7 @@ include(CompilerInfo)
 # This helps find the right third-party build directory.
 if ("${YB_BUILD_TYPE}" MATCHES "^(asan|tsan)$")
   set(THIRDPARTY_INSTRUMENTATION_TYPE "${YB_BUILD_TYPE}")
-elseif (IS_CLANG)
-  set(THIRDPARTY_INSTRUMENTATION_TYPE "uninstrumented")
-elseif (IS_GCC)
+elseif (IS_CLANG OR IS_GCC)
   set(THIRDPARTY_INSTRUMENTATION_TYPE "uninstrumented")
 else()
   message(FATAL_ERROR "Unknown compiler family: '${COMPILER_FAMILY}'.")
@@ -364,21 +363,6 @@ if(IS_CLANG)
   ADD_CXX_FLAGS("-Werror=implicit-fallthrough")
   ADD_CXX_FLAGS("-D_LIBCPP_ENABLE_THREAD_SAFETY_ANNOTATIONS")
   ADD_CXX_FLAGS("-Wthread-safety-analysis")
-
-  if("$ENV{YB_ENABLE_STATIC_ANALYZER}" STREQUAL "1")
-    if(APPLE)
-      message("YB_ENABLE_STATIC_ANALYZER is set to 1, but we are not running the static analyzer"
-              " on macOS yet")
-    else()
-      message("YB_ENABLE_STATIC_ANALYZER is set to 1, enabling Clang static analyzer")
-      include(yb_clang_analyzer)
-      message("clang_analyzer_flags=${clang_analyzer_flags}")
-      ADD_CXX_FLAGS("${clang_analyzer_flags}")
-    endif()
-  else()
-    message("YB_ENABLE_STATIC_ANALYZER is not set to 1, not enabling Clang static analyzer")
-  endif()
-
   ADD_CXX_FLAGS("-Wshorten-64-to-32")
 endif()
 
@@ -536,8 +520,6 @@ endif()
 
 include(YugabyteFindThirdParty)
 yb_find_third_party_installed_dir()
-set(YB_PREFIX_COMMON "${YB_THIRDPARTY_INSTALLED_DIR}/common")
-yb_put_var_into_cache(YB_PREFIX_COMMON STRING)
 
 if(IS_CLANG)
   YB_SETUP_CLANG()
@@ -680,20 +662,41 @@ function(ADD_THIRDPARTY_LIB LIB_NAME)
 endfunction()
 
 # Look in thirdparty prefix paths before anywhere else for system dependencies.
-set(CMAKE_PREFIX_PATH ${YB_PREFIX_COMMON} ${CMAKE_PREFIX_PATH})
-ADD_LINKER_FLAGS("-L${YB_PREFIX_COMMON}/lib")
+set(YB_THIRDPARTY_COMMON_DIR "${YB_THIRDPARTY_INSTALLED_DIR}/common")
 
-set(YB_THIRDPARTY_INSTALLED_DEPS_DIR
-  "${YB_THIRDPARTY_INSTALLED_DIR}/${THIRDPARTY_INSTRUMENTATION_TYPE}")
-set(CMAKE_PREFIX_PATH ${YB_THIRDPARTY_INSTALLED_DEPS_DIR} ${CMAKE_PREFIX_PATH})
-ADD_LINKER_FLAGS("-L${YB_THIRDPARTY_INSTALLED_DEPS_DIR}/lib")
+# The instrumentation type could be "uninstrumented", "asan", "tsan".
+set(YB_THIRDPARTY_MAYBE_INSTRUMENTED_DIR
+    "${YB_THIRDPARTY_INSTALLED_DIR}/${THIRDPARTY_INSTRUMENTATION_TYPE}")
 
-set(YB_THIRDPARTY_INSTALLED_DIR ${YB_PREFIX_COMMON})
+# PKG_CONFIG_PATH is a colon-separated list of directories.
+concat_thirdparty_prefix_dirs_with_suffix(YB_PKG_CONFIG_PATH ":" "/lib/pkgconfig")
+yb_put_var_into_cache(YB_PKG_CONFIG_PATH STRING)
 
-# See thirdparty/yb_build_thirdparty_main.py for an explanation of differences between installed and
-# installed-deps.
-ADD_GLOBAL_RPATH_ENTRY("${YB_THIRDPARTY_INSTALLED_DIR}/lib")
-ADD_GLOBAL_RPATH_ENTRY("${YB_THIRDPARTY_INSTALLED_DEPS_DIR}/lib")
+set(CMAKE_PREFIX_PATH
+    "${YB_THIRDPARTY_COMMON_DIR}"
+    "${YB_THIRDPARTY_MAYBE_INSTRUMENTED_DIR}"
+    ${CMAKE_PREFIX_PATH})
+
+# Paths added to CMAKE_PREFIX_PATH might end up as RPATHs on the linker command line. One possible
+# explanation for this is that when CMake finds dependency libraries in some of the "lib"
+# subdirectories of directories listed by CMAKE_PREFIX_PATHS, it will make sure the compiled
+# executables can find those libraries. Therefore, in order to avoid duplicate RPATHs in the
+# command line, we have to be cautions when adding these same directories to RPATH.
+message("CMAKE_PREFIX_PATH: ${CMAKE_PREFIX_PATH}")
+
+if(APPLE AND IS_CLANG AND "${COMPILER_VERSION}" MATCHES "^1[567][.].*$")
+  # To avoid "warning: duplicate LC_RPATH are deprecated" messages.
+  # Suggestion from https://github.com/igraph/igraph/issues/2394#issuecomment-1732292483
+  ADD_LINKER_FLAGS("-ld_classic")
+endif()
+
+ADD_GLOBAL_RPATH_ENTRY_AND_LIB_DIR("${YB_THIRDPARTY_COMMON_DIR}/lib")
+
+# Not adding RPATH here, because in practice it already gets added via the CMAKE_PREFIX_PATH
+# mechanism.
+YB_ADD_LIB_DIR("${YB_THIRDPARTY_MAYBE_INSTRUMENTED_DIR}/lib")
+SET(YB_THIRDPARTY_MAYBE_INSTRUMENTED_RPATH_ARG
+    "-Wl,-rpath,${YB_THIRDPARTY_MAYBE_INSTRUMENTED_DIR}/lib")
 
 # If we are using a non-default gcc or clang compiler, we need to add its library directory to
 # rpath.
@@ -738,7 +741,7 @@ if ("${YB_TCMALLOC_ENABLED}" STREQUAL "1" OR
   else()
     # Allow undefined symbols when linking libs, so not resolved references to tcmalloc symbols
     # does not break libs linking.
-    set(CMAKE_SHARED_LINKER_FLAGS "${CMAKE_SHARED_LINKER_FLAGS} -Wl,--allow-shlib-undefined")
+    string(APPEND CMAKE_SHARED_LINKER_FLAGS " -Wl,--allow-shlib-undefined")
     # Don't allow undefined symbols when linking executables, so we can catch undefined symbols
     # during build phase rather than later in runtime.
     ADD_EXE_LINKER_FLAGS("-Wl,--no-undefined -Wl,--no-allow-shlib-undefined")
@@ -831,6 +834,9 @@ set(CMAKE_CXX_STANDARD_REQUIRED ON)
 yb_deduplicate_arguments(CMAKE_EXE_LINKER_FLAGS)
 message("Linker flags for executables: ${CMAKE_EXE_LINKER_FLAGS}")
 
+yb_deduplicate_arguments(CMAKE_SHARED_LINKER_FLAGS)
+message("Linker flags for shared libraries: ${CMAKE_SHARED_LINKER_FLAGS}")
+
 # Define full paths to libpq and libyb_pgbackend shared libraries. These libraries are built as part
 # of building PostgreSQL. We need to declare these as by-products of running the PostgreSQL build
 # command, as well as wrap them into CMake targets so that other libraries and executables can

diff --git a/build-support/common-build-env.sh b/build-support/common-build-env.sh
@@ -63,7 +63,6 @@ $YB_SRC_ROOT/python/yugabyte/gen_initial_sys_catalog_snapshot.py
   export YB_SCRIPT_PATH_IS_SAME_PATH=$YB_SRC_ROOT/python/yugabyte/is_same_path.py
   export YB_SCRIPT_PATH_KILL_LONG_RUNNING_MINICLUSTER_DAEMONS=\
 $YB_SRC_ROOT/python/yugabyte/kill_long_running_minicluster_daemons.py
-  export YB_SCRIPT_PATH_MAKE_RPATH_RELATIVE=$YB_SRC_ROOT/python/yugabyte/make_rpath_relative.py
   export YB_SCRIPT_PATH_PARSE_TEST_FAILURE=$YB_SRC_ROOT/python/yugabyte/parse_test_failure.py
   export YB_SCRIPT_PATH_POSTPROCESS_TEST_RESULT=\
 $YB_SRC_ROOT/python/yugabyte/postprocess_test_result.py

diff --git a/build-support/compiler-wrappers/compiler-wrapper.sh b/build-support/compiler-wrappers/compiler-wrapper.sh
@@ -234,6 +234,7 @@ check_compiler_exit_code() {
     if grep -Eq 'error: linker command failed with exit code [0-9]+ \(use -v to see invocation\)' \
          "$stderr_path" || \
        grep -Eq 'error: ld returned' "$stderr_path"; then
+      local determine_compiler_cmdline_rv
       determine_compiler_cmdline
       generate_build_debug_script rerun_failed_link_step "$determine_compiler_cmdline_rv -v"
     fi
@@ -248,9 +249,39 @@ check_compiler_exit_code() {
   fi
 }
 
+remove_duplicate_rpath_args() {
+  local filtered_args=()
+  local rpath_args=()
+  local skip_arg
+  for arg in "${compiler_args[@]}"; do
+    if [[ $arg == -Wl,-rpath,* ]]; then
+      skip_arg=false
+      if [[ ${#rpath_args[@]} -gt 0 ]]; then
+        for existing_rpath_arg in "${rpath_args[@]}"; do
+          if [[ $existing_rpath_arg == "$arg" ]]; then
+            if [[ ${YB_FAIL_ON_DUPLICATE_RPATH:-0} == "1" ]]; then
+              fatal "Duplicate RPATH argument: $arg. Compiler arguments: ${compiler_args_str}"
+            fi
+            skip_arg=true
+            break
+          fi
+        done
+      fi
+      if [[ $skip_arg == "false" ]]; then
+        filtered_args+=( "$arg" )
+        rpath_args+=( "$arg" )
+      fi
+    else
+      filtered_args+=( "$arg" )
+    fi
+  done
+  compiler_args=( "${filtered_args[@]}" )
+}
+
 # -------------------------------------------------------------------------------------------------
 # Common setup for remote and local build.
 # We parse command-line arguments in both cases.
+# -------------------------------------------------------------------------------------------------
 
 cc_or_cxx=${0##*/}
 
@@ -259,7 +290,9 @@ stderr_path=/tmp/yb-$cc_or_cxx.$RANDOM-$RANDOM-$RANDOM.$$.stderr
 compiler_args=( "$@" )
 set +u
 # The same as one string. We allow undefined variables for this line because an empty array is
-# treated as such.
+# treated as such. When performing checks against compiler_args_str, note that that compiler_args
+# can be modified after this line, and compiler_args_str might not be an exact match compared to
+# the compiler command actually being executed.
 compiler_args_str="${compiler_args[*]}"
 set -u
 
@@ -380,12 +413,16 @@ if [[ $YB_COMPILER_TYPE == clang* && $output_file == "jsonpath_gram.o" ]]; then
   compiler_args+=( -Wno-error=implicit-fallthrough )
 fi
 
+remove_duplicate_rpath_args
+
 if [[ ${num_times_tcmalloc_linked} -gt 1 ]]; then
   fatal "libtcmalloc.a static library linked multiple times (${num_times_tcmalloc_linked} times)." \
         "This could lead to subtle bugs. Command line: ${compiler_args[*]}."
 fi
+
 # -------------------------------------------------------------------------------------------------
 # Remote build
+# -------------------------------------------------------------------------------------------------
 
 local_build_only=false
 for arg in "$@"; do
@@ -504,6 +541,7 @@ fi
 
 # -------------------------------------------------------------------------------------------------
 # Functions for local build
+# -------------------------------------------------------------------------------------------------
 
 local_build_exit_handler() {
   local exit_code=$?
@@ -592,6 +630,7 @@ run_compiler_and_save_stderr() {
 
 # -------------------------------------------------------------------------------------------------
 # Local build
+# -------------------------------------------------------------------------------------------------
 
 if [[ $output_file == libyb_pgbackend* || $output_file == postgres ]]; then
   # We record the linker command used for the libyb_pgbackend library so we can use it when
@@ -717,40 +756,6 @@ if [[ $has_yb_c_files == "true" && $PWD == $BUILD_ROOT/postgres_build/* ]]; then
 fi
 add_brew_bin_to_path
 
-# Make RPATHs relative whenever possible. This is an effort towards being able to relocate the
-# entire build root to a different path and still be able to run tests. Currently disabled for
-# PostgreSQL code because to do it correctly we need to know the eventual "installation" locations
-# of PostgreSQL binaries and libraries, which requires a bit more work.
-if [[ ${YB_DISABLE_RELATIVE_RPATH:-0} == "0" ]] &&
-   is_linux &&
-   "$rpath_found" &&
-   # In case BUILD_ROOT is defined (should be in all cases except for when we're determining the
-   # compilier type), and we know we are building inside of the PostgreSQL dir, we don't want to
-   # make RPATHs relative.
-   [[ -z $BUILD_ROOT || $PWD != $BUILD_ROOT/postgres_build/* ]]; then
-  if [[ $num_output_files_found -ne 1 ]]; then
-    # Ideally this will only happen as part of running PostgreSQL's configure and will be hidden.
-    log "RPATH options found on the command line, but could not find exactly one output file " \
-        "to make RPATHs relative to. Found $num_output_files_found output files. Command args: " \
-        "$compiler_args_str"
-  else
-    new_cmd=()
-    for arg in "${cmd[@]}"; do
-      case $arg in
-        -Wl,-rpath,*)
-          new_rpath_arg=$(
-          "$YB_SCRIPT_PATH_MAKE_RPATH_RELATIVE" "$output_file" "$arg"
-          )
-          new_cmd+=( "$new_rpath_arg" )
-        ;;
-        *)
-          new_cmd+=( "$arg" )
-      esac
-    done
-    cmd=( "${new_cmd[@]}" )
-  fi
-fi
-
 if [[ $PWD == $BUILD_ROOT/postgres_build ||
       $PWD == $BUILD_ROOT/postgres_build/* ]]; then
   new_cmd=()
@@ -925,40 +930,3 @@ if grep -Eq 'ld: warning: directory not found for option' "$stderr_path"; then
   log "Linker failed to find a directory (probably a library directory) that should exist."
   exit 1
 fi
-
-if is_clang &&
-    [[ ${YB_ENABLE_STATIC_ANALYZER:-0} == "1" ]] &&
-    [[ ${YB_PG_BUILD_STEP:-} != "configure" ]] &&
-    ! is_configure_mode_invocation &&
-    [[ $output_file == *.o ]] &&
-    is_linux; then
-  if [[ $analyzer_checkers_specified == "false" ]]; then
-    log "No -analyzer-checker=... option found on compiler command line. It is possible that" \
-        "cmake was run without the YB_ENABLE_STATIC_ANALYZER environment variable set to 1. " \
-        "Command: $compiler_args_str"
-    echo "$compiler_args_str" >>/tmp/compiler_args.log
-  fi
-  compilation_step_name="STATIC ANALYSIS"
-  output_prefix=${output_file%.o}
-  stderr_path=$output_prefix.analyzer.err
-  html_output_dir="$output_prefix.html.d"
-  if [[ -e $html_output_dir ]]; then
-    rm -rf "$html_output_dir"
-  fi
-
-  # The error handler will delete the .o file on analyzer failure to keep the combined compilation
-  # and analysis process repeatable. Re-running the build should result in the same error if the
-  # analyzer fails.
-  delete_output_file_on_failure=true
-  delete_stderr_file=false
-
-  # TODO: the output redirection here does not quite work. clang might be doing something unusual
-  # with the tty. As of 01/2019 all stderr output might still go to the terminal/log.
-  set +e
-  "$compiler_executable" "${compiler_args_no_output[@]}" --analyze \
-    -Xanalyzer -analyzer-output=html -fno-color-diagnostics -o "$html_output_dir" \
-    2>"$stderr_path"
-
-  compiler_exit_code=$?
-  set -e
-fi
diff --git a/build-support/jenkins/build.sh b/build-support/jenkins/build.sh
@@ -325,28 +325,6 @@ if [[ $YB_RUN_AFFECTED_TESTS_ONLY == "1" ]]; then
   )
 fi
 
-if [[ ${YB_ENABLE_STATIC_ANALYZER:-auto} == "auto" ]]; then
-  if is_clang &&
-     is_linux &&
-     [[ $build_type =~ ^(debug|release)$ ]] &&
-     is_jenkins_master_build
-  then
-    if true; then
-      log "Not enabling Clang static analyzer. Will enable in clang/Linux builds in the future."
-    else
-      # TODO: re-enable this when we have time to sift through analyzer warnings.
-      export YB_ENABLE_STATIC_ANALYZER=1
-      log "Enabling Clang static analyzer (this is a clang Linux $build_type build)"
-    fi
-  else
-    log "Not enabling Clang static analyzer (this is not a clang Linux debug/release build):" \
-        "OSTYPE=$OSTYPE, YB_COMPILER_TYPE=$YB_COMPILER_TYPE, build_type=$build_type"
-  fi
-else
-  log "YB_ENABLE_STATIC_ANALYZER is already set to $YB_ENABLE_STATIC_ANALYZER," \
-      "not setting automatically"
-fi
-
 # We have a retry loop around CMake because it sometimes fails due to NFS unavailability.
 declare -i -r MAX_CMAKE_RETRIES=3
 declare -i cmake_attempt_index=1