diff --git a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java index 9e8c958ee61bc9..c671bf495df292 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java +++ b/src/main/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionBuilder.java @@ -532,10 +532,6 @@ boolean canSplitCommandLine() { case PIC_STATIC_LIBRARY: case ALWAYS_LINK_STATIC_LIBRARY: case ALWAYS_LINK_PIC_STATIC_LIBRARY: - case OBJC_ARCHIVE: - case OBJC_FULLY_LINKED_ARCHIVE: - case OBJC_EXECUTABLE: - case OBJCPP_EXECUTABLE: return true; default: diff --git a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java index 6cc0b8c9b63bec..2e90615456719f 100644 --- a/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java +++ b/src/main/java/com/google/devtools/build/lib/rules/objc/CompilationSupport.java @@ -1183,7 +1183,7 @@ CompilationSupport registerLinkActions( ruleContext, ruleContext.getLabel(), binaryToLink, - buildConfiguration, + ruleContext.getConfiguration(), toolchain, toolchain.getFdoContext(), getFeatureConfiguration(ruleContext, toolchain, buildConfiguration), @@ -1358,7 +1358,7 @@ CompilationSupport registerFullyLinkAction(ObjcProvider objcProvider, Artifact o ruleContext, ruleContext.getLabel(), outputArchive, - buildConfiguration, + ruleContext.getConfiguration(), toolchain, toolchain.getFdoContext(), getFeatureConfiguration(ruleContext, toolchain, buildConfiguration), diff --git a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java index e36068e95f22c5..445f33591898fa 100644 --- a/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java +++ b/src/test/java/com/google/devtools/build/lib/rules/cpp/CppLinkActionTest.java @@ -604,18 +604,6 @@ public void testCommandLineSplitting() throws Exception { builder.setLinkType(LinkTargetType.STATIC_LIBRARY); assertThat(builder.canSplitCommandLine()).isTrue(); - builder.setLinkType(LinkTargetType.OBJC_ARCHIVE); - assertThat(builder.canSplitCommandLine()).isTrue(); - - builder.setLinkType(LinkTargetType.OBJC_EXECUTABLE); - assertThat(builder.canSplitCommandLine()).isTrue(); - - builder.setLinkType(LinkTargetType.OBJCPP_EXECUTABLE); - assertThat(builder.canSplitCommandLine()).isTrue(); - - builder.setLinkType(LinkTargetType.OBJC_FULLY_LINKED_ARCHIVE); - assertThat(builder.canSplitCommandLine()).isTrue(); - builder.setLinkType(LinkTargetType.NODEPS_DYNAMIC_LIBRARY); assertThat(builder.canSplitCommandLine()).isTrue(); diff --git a/src/test/shell/integration/aquery_test.sh b/src/test/shell/integration/aquery_test.sh index 7819ba3bd64e15..b237b3e1e9c45d 100755 --- a/src/test/shell/integration/aquery_test.sh +++ b/src/test/shell/integration/aquery_test.sh @@ -711,6 +711,10 @@ EOF } function test_aquery_include_param_file_cc_binary() { + if is_darwin; then + return 0 + fi + local pkg="${FUNCNAME[0]}" mkdir -p "$pkg" || fail "mkdir -p $pkg" cat > "$pkg/BUILD" <<'EOF' @@ -735,6 +739,10 @@ EOF } function test_aquery_include_param_file_starlark_rule() { + if is_darwin; then + return 0 + fi + local pkg="${FUNCNAME[0]}" mkdir -p "$pkg" || fail "mkdir -p $pkg" cat > "$pkg/test_rule.bzl" <<'EOF' @@ -785,6 +793,10 @@ EOF } function test_aquery_include_param_file_not_enabled_by_default() { + if is_darwin; then + return 0 + fi + local pkg="${FUNCNAME[0]}" mkdir -p "$pkg" || fail "mkdir -p $pkg" cat > "$pkg/BUILD" <<'EOF' diff --git a/tools/cpp/BUILD.tools b/tools/cpp/BUILD.tools index db27b19ed59899..dc72e233a7b0a6 100644 --- a/tools/cpp/BUILD.tools +++ b/tools/cpp/BUILD.tools @@ -277,7 +277,7 @@ cc_toolchain( linker_files = ":osx_wrapper", objcopy_files = ":empty", strip_files = ":empty", - supports_param_files = 1, + supports_param_files = 0, toolchain_config = ":local_darwin", toolchain_identifier = "local_darwin", ) diff --git a/tools/cpp/BUILD.tpl b/tools/cpp/BUILD.tpl index d3514d12fb92ba..37986a9047fff6 100644 --- a/tools/cpp/BUILD.tpl +++ b/tools/cpp/BUILD.tpl @@ -66,7 +66,7 @@ cc_toolchain( linker_files = ":compiler_deps", objcopy_files = ":empty", strip_files = ":empty", - supports_param_files = 1, + supports_param_files = %{supports_param_files}, module_map = %{modulemap}, ) diff --git a/tools/cpp/osx_cc_wrapper.sh b/tools/cpp/osx_cc_wrapper.sh index 8c9c111a750587..bbb5d7e5390e81 100755 --- a/tools/cpp/osx_cc_wrapper.sh +++ b/tools/cpp/osx_cc_wrapper.sh @@ -34,33 +34,20 @@ LIBS= LIB_DIRS= RPATHS= OUTPUT= - -function parse_option() { - local -r opt="$1" +# let parse the option list +for i in "$@"; do if [[ "${OUTPUT}" = "1" ]]; then - OUTPUT=$opt - elif [[ "$opt" =~ ^-l(.*)$ ]]; then + OUTPUT=$i + elif [[ "$i" =~ ^-l(.*)$ ]]; then LIBS="${BASH_REMATCH[1]} $LIBS" - elif [[ "$opt" =~ ^-L(.*)$ ]]; then + elif [[ "$i" =~ ^-L(.*)$ ]]; then LIB_DIRS="${BASH_REMATCH[1]} $LIB_DIRS" - elif [[ "$opt" =~ ^-Wl,-rpath,\@loader_path/(.*)$ ]]; then + elif [[ "$i" =~ ^-Wl,-rpath,\@loader_path/(.*)$ ]]; then RPATHS="${BASH_REMATCH[1]} ${RPATHS}" - elif [[ "$opt" = "-o" ]]; then + elif [[ "$i" = "-o" ]]; then # output is coming OUTPUT=1 fi -} - -# let parse the option list -for i in "$@"; do - if [[ "$i" = @* ]]; then - while IFS= read -r opt - do - parse_option "$opt" - done < "${i:1}" || exit 1 - else - parse_option "$i" - fi done # Call gcc diff --git a/tools/cpp/osx_cc_wrapper.sh.tpl b/tools/cpp/osx_cc_wrapper.sh.tpl index 28bd47ba1a4a97..4c85cd9b6bb39f 100644 --- a/tools/cpp/osx_cc_wrapper.sh.tpl +++ b/tools/cpp/osx_cc_wrapper.sh.tpl @@ -33,33 +33,20 @@ LIBS= LIB_DIRS= RPATHS= OUTPUT= - -function parse_option() { - local -r opt="$1" +# let parse the option list +for i in "$@"; do if [[ "${OUTPUT}" = "1" ]]; then - OUTPUT=$opt - elif [[ "$opt" =~ ^-l(.*)$ ]]; then + OUTPUT=$i + elif [[ "$i" =~ ^-l(.*)$ ]]; then LIBS="${BASH_REMATCH[1]} $LIBS" - elif [[ "$opt" =~ ^-L(.*)$ ]]; then + elif [[ "$i" =~ ^-L(.*)$ ]]; then LIB_DIRS="${BASH_REMATCH[1]} $LIB_DIRS" - elif [[ "$opt" =~ ^-Wl,-rpath,\@loader_path/(.*)$ ]]; then + elif [[ "$i" =~ ^-Wl,-rpath,\@loader_path/(.*)$ ]]; then RPATHS="${BASH_REMATCH[1]} ${RPATHS}" - elif [[ "$opt" = "-o" ]]; then + elif [[ "$i" = "-o" ]]; then # output is coming OUTPUT=1 fi -} - -# let parse the option list -for i in "$@"; do - if [[ "$i" = @* ]]; then - while IFS= read -r opt - do - parse_option "$opt" - done < "${i:1}" || exit 1 - else - parse_option "$i" - fi done # Set-up the environment diff --git a/tools/cpp/unix_cc_configure.bzl b/tools/cpp/unix_cc_configure.bzl index 5cb1a9b21cb141..5270e9ee668a9a 100644 --- a/tools/cpp/unix_cc_configure.bzl +++ b/tools/cpp/unix_cc_configure.bzl @@ -447,6 +447,7 @@ def configure_unix_toolchain(repository_ctx, cpu_value, overriden_tools): "%{cc_toolchain_identifier}": cc_toolchain_identifier, "%{name}": cpu_value, "%{modulemap}": ("\":module.modulemap\"" if is_clang else "None"), + "%{supports_param_files}": "0" if darwin else "1", "%{cc_compiler_deps}": get_starlark_list([":builtin_include_directory_paths"] + ( [":cc_wrapper"] if darwin else [] )), diff --git a/tools/osx/crosstool/BUILD.tpl b/tools/osx/crosstool/BUILD.tpl index ce8a0add5b82bc..a2e6e92d8aca7a 100644 --- a/tools/osx/crosstool/BUILD.tpl +++ b/tools/osx/crosstool/BUILD.tpl @@ -70,7 +70,7 @@ cc_toolchain_suite( linker_files = ":osx_tools_" + arch, objcopy_files = ":empty", strip_files = ":osx_tools_" + arch, - supports_param_files = 1, + supports_param_files = 0, toolchain_config = ":" + ( arch if arch != "armeabi-v7a" else "stub_armeabi-v7a" ), diff --git a/tools/osx/crosstool/cc_toolchain_config.bzl b/tools/osx/crosstool/cc_toolchain_config.bzl index 267b856c67e014..d8c16d9937b60a 100644 --- a/tools/osx/crosstool/cc_toolchain_config.bzl +++ b/tools/osx/crosstool/cc_toolchain_config.bzl @@ -5198,13 +5198,16 @@ def _impl(ctx): name = "linker_param_file", flag_sets = [ flag_set( - actions = all_link_actions + [ - ACTION_NAMES.cpp_link_static_library, - ACTION_NAMES.objc_archive, - ACTION_NAMES.objc_fully_link, - ACTION_NAMES.objc_executable, - ACTION_NAMES.objcpp_executable, + actions = all_link_actions, + flag_groups = [ + flag_group( + flags = ["-Wl,@%{linker_param_file}"], + expand_if_available = "linker_param_file", + ), ], + ), + flag_set( + actions = [ACTION_NAMES.cpp_link_static_library], flag_groups = [ flag_group( flags = ["@%{linker_param_file}"], diff --git a/tools/osx/crosstool/wrapped_clang.cc b/tools/osx/crosstool/wrapped_clang.cc index febf3cb246cbec..886cc12a3853fd 100644 --- a/tools/osx/crosstool/wrapped_clang.cc +++ b/tools/osx/crosstool/wrapped_clang.cc @@ -55,45 +55,6 @@ const char *Basename(const char *filepath) { return base ? (base + 1) : filepath; } -// Unescape and unquote an argument read from a line of a response file. -static std::string Unescape(const std::string &arg) { - std::string result; - auto length = arg.size(); - for (size_t i = 0; i < length; ++i) { - auto ch = arg[i]; - - // If it's a backslash, consume it and append the character that follows. - if (ch == '\\' && i + 1 < length) { - ++i; - result.push_back(arg[i]); - continue; - } - - // If it's a quote, process everything up to the matching quote, unescaping - // backslashed characters as needed. - if (ch == '"' || ch == '\'') { - auto quote = ch; - ++i; - while (i != length && arg[i] != quote) { - if (arg[i] == '\\' && i + 1 < length) { - ++i; - } - result.push_back(arg[i]); - ++i; - } - if (i == length) { - break; - } - continue; - } - - // It's a regular character. - result.push_back(ch); - } - - return result; -} - // Converts an array of string arguments to char *arguments. // The first arg is reduced to its basename as per execve conventions. // Note that the lifetime of the char* arguments in the returned array @@ -113,7 +74,7 @@ std::vector ConvertToCArgs(const std::vector &args) { void ExecProcess(const std::vector &args) { std::vector exec_argv = ConvertToCArgs(args); execv(args[0].c_str(), const_cast(exec_argv.data())); - std::cerr << "Error executing child process.'" << args[0] << "'. " + std::cerr << "Error executing child process.'" << args[0] << "'. " << strerror(errno) << "\n"; abort(); } @@ -131,17 +92,17 @@ void RunSubProcess(const std::vector &args) { wait_status = waitpid(pid, &status, 0); } while ((wait_status == -1) && (errno == EINTR)); if (wait_status < 0) { - std::cerr << "Error waiting on child process '" << args[0] << "'. " + std::cerr << "Error waiting on child process '" << args[0] << "'. " << strerror(errno) << "\n"; abort(); } if (WEXITSTATUS(status) != 0) { - std::cerr << "Error in child process '" << args[0] << "'. " + std::cerr << "Error in child process '" << args[0] << "'. " << WEXITSTATUS(status) << "\n"; abort(); } } else { - std::cerr << "Error forking process '" << args[0] << "'. " + std::cerr << "Error forking process '" << args[0] << "'. " << strerror(status) << "\n"; abort(); } @@ -196,151 +157,6 @@ bool StripPrefixStringIfPresent(std::string *str, const std::string &prefix) { return false; } -// An RAII temporary file. -class TempFile { - public: - // Create a new temporary file using the given path template string (the same - // form used by `mkstemp`). The file will automatically be deleted when the - // object goes out of scope. - static std::unique_ptr Create(const std::string &path_template) { - const char *tmpDir = getenv("TMPDIR"); - if (!tmpDir) { - tmpDir = "/tmp"; - } - size_t size = strlen(tmpDir) + path_template.size() + 2; - std::unique_ptr path(new char[size]); - snprintf(path.get(), size, "%s/%s", tmpDir, path_template.c_str()); - - if (mkstemp(path.get()) == -1) { - std::cerr << "Failed to create temporary file '" << path.get() - << "': " << strerror(errno) << "\n"; - return nullptr; - } - return std::unique_ptr(new TempFile(path.get())); - } - - // Explicitly make TempFile non-copyable and movable. - TempFile(const TempFile &) = delete; - TempFile &operator=(const TempFile &) = delete; - TempFile(TempFile &&) = default; - TempFile &operator=(TempFile &&) = default; - - ~TempFile() { remove(path_.c_str()); } - - // Gets the path to the temporary file. - std::string GetPath() const { return path_; } - - private: - explicit TempFile(const std::string &path) : path_(path) {} - - std::string path_; -}; - -static std::unique_ptr WriteResponseFile( - const std::vector &args) { - auto response_file = TempFile::Create("wrapped_clang_params.XXXXXX"); - std::ofstream response_file_stream(response_file->GetPath()); - - for (const auto &arg : args) { - // When Clang/Swift write out a response file to communicate from driver to - // frontend, they just quote every argument to be safe; we duplicate that - // instead of trying to be "smarter" and only quoting when necessary. - response_file_stream << '"'; - for (auto ch : arg) { - if (ch == '"' || ch == '\\') { - response_file_stream << '\\'; - } - response_file_stream << ch; - } - response_file_stream << "\"\n"; - } - - response_file_stream.close(); - return response_file; -} - -void ProcessArgument(const std::string arg, const std::string developer_dir, - const std::string sdk_root, const std::string cwd, - bool relative_ast_path, std::string &linked_binary, - std::string &dsym_path, - std::function consumer); - -bool ProcessResponseFile(const std::string arg, const std::string developer_dir, - const std::string sdk_root, const std::string cwd, - bool relative_ast_path, std::string &linked_binary, - std::string &dsym_path, - std::function consumer) { - auto path = arg.substr(1); - std::ifstream original_file(path); - // Ignore non-file args such as '@loader_path/...' - if (!original_file.good()) { - return false; - } - - std::string arg_from_file; - while (std::getline(original_file, arg_from_file)) { - // Arguments in response files might be quoted/escaped, so we need to - // unescape them ourselves. - ProcessArgument(Unescape(arg_from_file), developer_dir, sdk_root, cwd, - relative_ast_path, linked_binary, dsym_path, consumer); - } - - return true; -} - -std::string GetCurrentDirectory() { - // Passing null,0 causes getcwd to allocate the buffer of the correct size. - char *buffer = getcwd(nullptr, 0); - std::string cwd(buffer); - free(buffer); - return cwd; -} - -void ProcessArgument(const std::string arg, const std::string developer_dir, - const std::string sdk_root, const std::string cwd, - bool relative_ast_path, std::string &linked_binary, - std::string &dsym_path, - std::function consumer) { - auto new_arg = arg; - if (arg[0] == '@') { - if (ProcessResponseFile(arg, developer_dir, sdk_root, cwd, - relative_ast_path, linked_binary, dsym_path, - consumer)) { - return; - } - } - - if (SetArgIfFlagPresent(arg, "DSYM_HINT_LINKED_BINARY", &linked_binary)) { - return; - } - if (SetArgIfFlagPresent(arg, "DSYM_HINT_DSYM_PATH", &dsym_path)) { - return; - } - - std::string dest_dir, bitcode_symbol_map; - if (arg.compare("OSO_PREFIX_MAP_PWD") == 0) { - new_arg = "-Wl,-oso_prefix," + cwd + "/"; - } - - FindAndReplace("__BAZEL_XCODE_DEVELOPER_DIR__", developer_dir, &new_arg); - FindAndReplace("__BAZEL_XCODE_SDKROOT__", sdk_root, &new_arg); - - // Make the `add_ast_path` options used to embed Swift module references - // absolute to enable Swift debugging without dSYMs: see - // https://forums.swift.org/t/improving-swift-lldb-support-for-path-remappings/22694 - if (!relative_ast_path && - StripPrefixStringIfPresent(&new_arg, kAddASTPathPrefix)) { - // Only modify relative paths. - if (!StartsWith(arg, "/")) { - new_arg = std::string(kAddASTPathPrefix) + cwd + "/" + new_arg; - } else { - new_arg = std::string(kAddASTPathPrefix) + new_arg; - } - } - - consumer(new_arg); -} - } // namespace int main(int argc, char *argv[]) { @@ -360,34 +176,60 @@ int main(int argc, char *argv[]) { std::string developer_dir = GetMandatoryEnvVar("DEVELOPER_DIR"); std::string sdk_root = GetMandatoryEnvVar("SDKROOT"); + + std::vector processed_args = {"/usr/bin/xcrun", tool_name}; + std::string linked_binary, dsym_path; + std::string dest_dir; - const std::string cwd = GetCurrentDirectory(); - std::vector invocation_args = {"/usr/bin/xcrun", tool_name}; - std::vector processed_args = {}; + std::unique_ptr cwd{getcwd(nullptr, 0), + std::free}; + if (cwd == nullptr) { + std::cerr << "Error determining current working directory\n"; + abort(); + } bool relative_ast_path = getenv("RELATIVE_AST_PATH") != nullptr; - auto consumer = [&](const std::string &arg) { - processed_args.push_back(arg); - }; for (int i = 1; i < argc; i++) { std::string arg(argv[i]); - ProcessArgument(arg, developer_dir, sdk_root, cwd, relative_ast_path, - linked_binary, dsym_path, consumer); + if (SetArgIfFlagPresent(arg, "DSYM_HINT_LINKED_BINARY", &linked_binary)) { + continue; + } + if (SetArgIfFlagPresent(arg, "DSYM_HINT_DSYM_PATH", &dsym_path)) { + continue; + } + if (arg.compare("OSO_PREFIX_MAP_PWD") == 0) { + arg = "-Wl,-oso_prefix," + std::string(cwd.get()) + "/"; + } + FindAndReplace("__BAZEL_XCODE_DEVELOPER_DIR__", developer_dir, &arg); + FindAndReplace("__BAZEL_XCODE_SDKROOT__", sdk_root, &arg); + + // Make the `add_ast_path` options used to embed Swift module references + // absolute to enable Swift debugging without dSYMs: see + // https://forums.swift.org/t/improving-swift-lldb-support-for-path-remappings/22694 + if (!relative_ast_path && + StripPrefixStringIfPresent(&arg, kAddASTPathPrefix)) { + // Only modify relative paths. + if (!StartsWith(arg, "/")) { + arg = std::string(kAddASTPathPrefix) + + std::string(cwd.get()) + "/" + arg; + } else { + arg = std::string(kAddASTPathPrefix) + arg; + } + } + + processed_args.push_back(arg); } // Special mode that only prints the command. Used for testing. if (getenv("__WRAPPED_CLANG_LOG_ONLY")) { - for (const std::string &arg : invocation_args) std::cout << arg << ' '; - for (const std::string &arg : processed_args) std::cout << arg << ' '; + for (const std::string &arg : processed_args) + std::cout << arg << ' '; std::cout << "\n"; return 0; } - auto response_file = WriteResponseFile(processed_args); - invocation_args.push_back("@" + response_file->GetPath()); - // Check to see if we should postprocess with dsymutil. bool postprocess = false; if ((!linked_binary.empty()) || (!dsym_path.empty())) { @@ -399,8 +241,8 @@ int main(int argc, char *argv[]) { missing_dsym_flag = "DSYM_HINT_DSYM_PATH"; } std::cerr << "Error in clang wrapper: If any dsym " - "hint is defined, then " - << missing_dsym_flag << " must be defined\n"; + "hint is defined, then " + << missing_dsym_flag << " must be defined\n"; abort(); } else { postprocess = true; @@ -408,15 +250,16 @@ int main(int argc, char *argv[]) { } if (!postprocess) { - ExecProcess(invocation_args); + ExecProcess(processed_args); std::cerr << "ExecProcess should not return. Please fix!\n"; abort(); } - RunSubProcess(invocation_args); + RunSubProcess(processed_args); - std::vector dsymutil_args = { - "/usr/bin/xcrun", "dsymutil", linked_binary, "-o", dsym_path, "--flat"}; + std::vector dsymutil_args = {"/usr/bin/xcrun", "dsymutil", + linked_binary, "-o", dsym_path, + "--flat"}; ExecProcess(dsymutil_args); std::cerr << "ExecProcess should not return. Please fix!\n"; abort(); diff --git a/tools/osx/crosstool/wrapped_clang_test.sh b/tools/osx/crosstool/wrapped_clang_test.sh index 484b2dc2d41e26..921b108bc92109 100755 --- a/tools/osx/crosstool/wrapped_clang_test.sh +++ b/tools/osx/crosstool/wrapped_clang_test.sh @@ -80,20 +80,4 @@ function test_sdkroot_remapping() { expect_log "sdkroot=mysdkroot" "Expected sdkroot to be remapped." } -function test_params_expansion() { - params=$(mktemp) - { - echo "first" - echo "-rpath" - echo "@loader_path" - echo "sdkroot=__BAZEL_XCODE_SDKROOT__" - echo "developer_dir=__BAZEL_XCODE_DEVELOPER_DIR__" - } > "$params" - - env DEVELOPER_DIR=dummy SDKROOT=mysdkroot \ - "${WRAPPED_CLANG}" "@$params" \ - >"$TEST_log" || fail "wrapped_clang failed"; - expect_log "/usr/bin/xcrun clang first -rpath @loader_path sdkroot=mysdkroot developer_dir=dummy" -} - run_suite "Wrapped clang tests"