Improve handling of tool build flags (#6475)

rauhul · web-flow · commit 151343e96705 · 2023-05-03T12:41:00.000+01:00
Refactors the various compilerArguments and linkerArgument functions slightly to ensure flags from a user's sdk and cli properly propagate to the expected subcommand invocations. Adds a fairly comprehensive test to cover the toolset and cli flag passing end result. --- Fixes a bug where clang targets didn't respect the `isCXX` override. Fixes a bug where cxxCompiler extraCLIFlags where not passed to clang when building cxx targets. Future work: #6491. cxxCompiler extraCLIFlags should also be passed to `swiftc` when compiling swift modules to ensure the clang importer has the same interpretation of the module as the initial compile. Fixes a bug where linker extraCLIFlags were not passed to `swiftc` prefixed by `-Xlinker`. The previous workaround to pass these flags in swiftCompiler extraCLIFlags is no longer necessary. Fixes a bug where a user could pass `-whole-module-optimization` or `-wmo` to swiftc when linking causing the driver to either attempt to compile the object files when using the legacy driver: rdar://27578292 or crash when using the new driver: rdar://108057797. Fixes a bug where cCompiler extraCLIFlags were not passed to `swiftc` prefixed by `-Xcc`. The previous workaround to pass these flags in swiftCompiler extraCLIFlags is no longer necessary. Fixes a bug where extraCLIFlags from an SDKs toolset.json files would be repeated multiple times in child process invocations. The repeated flags could cause issues with some downstream tools, such as repeated `-segaddr` flags to ld64 for the same segment. Fixes a bug where extra toolchain flags would be added to an xcode build parameters file without escaping. Fixes a bug where extra toolchain linker flags would not be added to an xcode build parameters file.
diff --git a/Sources/Build/BuildDescription/ClangTargetBuildDescription.swift b/Sources/Build/BuildDescription/ClangTargetBuildDescription.swift
@@ -235,12 +235,14 @@ public final class ClangTargetBuildDescription {
             args += ["-include", resourceAccessorHeaderFile.pathString]
         }
 
-        args += buildParameters.toolchain.extraFlags.cCompilerFlags
-        // User arguments (from -Xcc and -Xcxx below) should follow generated arguments to allow user overrides
-        args += buildParameters.flags.cCompilerFlags
+        args += self.buildParameters.toolchain.extraFlags.cCompilerFlags
+        // User arguments (from -Xcc) should follow generated arguments to allow user overrides
+        args += self.buildParameters.flags.cCompilerFlags
 
         // Add extra C++ flags if this target contains C++ files.
-        if clangTarget.isCXX {
+        if isCXX {
+            args += self.buildParameters.toolchain.extraFlags.cxxCompilerFlags
+            // User arguments (from -Xcxx) should follow generated arguments to allow user overrides
             args += self.buildParameters.flags.cxxCompilerFlags
         }
         return args
diff --git a/Sources/Build/BuildDescription/ProductBuildDescription.swift b/Sources/Build/BuildDescription/ProductBuildDescription.swift
@@ -151,6 +151,11 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription
         args += self.buildParameters.sanitizers.linkSwiftFlags()
         args += self.additionalFlags
 
+        // pass `-v` during verbose builds.
+        if self.buildParameters.verboseOutput {
+            args += ["-v"]
+        }
+
         // Pass `-g` during a *release* build so the Swift driver emits a dSYM file for the binary.
         if self.buildParameters.configuration == .release {
             if self.buildParameters.triple.isWindows() {
@@ -304,9 +309,12 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription
         args += self.swiftASTs.flatMap { ["-Xlinker", "-add_ast_path", "-Xlinker", $0.pathString] }
 
         args += self.buildParameters.toolchain.extraFlags.swiftCompilerFlags
-        // User arguments (from -Xlinker and -Xswiftc) should follow generated arguments to allow user overrides
-        args += self.buildParameters.linkerFlags
-        args += self.stripInvalidArguments(self.buildParameters.swiftCompilerFlags)
+        // User arguments (from -Xswiftc) should follow generated arguments to allow user overrides
+        args += self.buildParameters.flags.swiftCompilerFlags
+
+        args += self.buildParameters.toolchain.extraFlags.linkerFlags.asSwiftcLinkerFlags()
+        // User arguments (from -Xlinker) should follow generated arguments to allow user overrides
+        args += self.buildParameters.flags.linkerFlags.asSwiftcLinkerFlags()
 
         // Add toolchain's libdir at the very end (even after the user -Xlinker arguments).
         //
@@ -323,7 +331,7 @@ public final class ProductBuildDescription: SPMBuildCore.ProductBuildDescription
         }
         #endif
 
-        return args
+        return self.stripInvalidArguments(args)
     }
 
     /// Writes link filelist to the filesystem.
diff --git a/Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift b/Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift
@@ -422,6 +422,11 @@ public final class SwiftTargetBuildDescription {
         args += try self.buildParameters.targetTripleArgs(for: self.target)
         args += ["-swift-version", self.swiftVersion.rawValue]
 
+        // pass `-v` during verbose builds.
+        if self.buildParameters.verboseOutput {
+            args += ["-v"]
+        }
+
         // Enable batch mode in debug mode.
         //
         // Technically, it should be enabled whenever WMO is off but we
@@ -504,7 +509,17 @@ public final class SwiftTargetBuildDescription {
 
         args += self.buildParameters.toolchain.extraFlags.swiftCompilerFlags
         // User arguments (from -Xswiftc) should follow generated arguments to allow user overrides
-        args += self.buildParameters.swiftCompilerFlags
+        args += self.buildParameters.flags.swiftCompilerFlags
+
+        args += self.buildParameters.toolchain.extraFlags.cCompilerFlags.asSwiftcCCompilerFlags()
+        // User arguments (from -Xcc) should follow generated arguments to allow user overrides
+        args += self.buildParameters.flags.cCompilerFlags.asSwiftcCCompilerFlags()
+
+        // TODO: Pass -Xcxx flags to swiftc (#6491)
+        // Uncomment when downstream support arrives.
+        // args += self.buildParameters.toolchain.extraFlags.cxxCompilerFlags.asSwiftcCXXCompilerFlags()
+        // // User arguments (from -Xcxx) should follow generated arguments to allow user overrides
+        // args += self.buildParameters.flags.cxxCompilerFlags.asSwiftcCXXCompilerFlags()
 
         // suppress warnings if the package is remote
         if self.package.isRemote {
@@ -619,6 +634,11 @@ public final class SwiftTargetBuildDescription {
         var result: [String] = []
         result.append(self.buildParameters.toolchain.swiftCompilerPath.pathString)
 
+        // pass `-v` during verbose builds.
+        if self.buildParameters.verboseOutput {
+            result += ["-v"]
+        }
+
         result.append("-module-name")
         result.append(self.target.c99name)
         result.append(contentsOf: packageNameArgumentIfSupported(with: self.package, packageAccess: self.target.packageAccess))
@@ -654,8 +674,21 @@ public final class SwiftTargetBuildDescription {
         result += self.buildParameters.sanitizers.compileSwiftFlags()
         result += ["-parseable-output"]
         result += try self.buildSettingsFlags()
+
         result += self.buildParameters.toolchain.extraFlags.swiftCompilerFlags
-        result += self.buildParameters.swiftCompilerFlags
+        // User arguments (from -Xswiftc) should follow generated arguments to allow user overrides
+        result += self.buildParameters.flags.swiftCompilerFlags
+
+        result += self.buildParameters.toolchain.extraFlags.cCompilerFlags.asSwiftcCCompilerFlags()
+        // User arguments (from -Xcc) should follow generated arguments to allow user overrides
+        result += self.buildParameters.flags.cCompilerFlags.asSwiftcCCompilerFlags()
+
+        // TODO: Pass -Xcxx flags to swiftc (#6491)
+        // Uncomment when downstream support arrives.
+        // result += self.buildParameters.toolchain.extraFlags.cxxCompilerFlags.asSwiftcCXXCompilerFlags()
+        // // User arguments (from -Xcxx) should follow generated arguments to allow user overrides
+        // result += self.buildParameters.flags.cxxCompilerFlags.asSwiftcCXXCompilerFlags()
+
         result += try self.macroArguments()
         return result
     }
diff --git a/Sources/Build/BuildPlan.swift b/Sources/Build/BuildPlan.swift
@@ -30,31 +30,30 @@ extension String {
     }
 }
 
-extension BuildParameters {
-    /// Returns the directory to be used for module cache.
-    public var moduleCache: AbsolutePath {
-        get throws {
-            // FIXME: We use this hack to let swiftpm's functional test use shared
-            // cache so it doesn't become painfully slow.
-            if let path = ProcessEnv.vars["SWIFTPM_TESTS_MODULECACHE"] {
-                return try AbsolutePath(validating: path)
-            }
-            return buildPath.appending("ModuleCache")
-        }
+extension Array where Element == String {
+    /// Converts a set of C compiler flags into an equivalent set to be
+    /// indirected through the Swift compiler instead.
+    func asSwiftcCCompilerFlags() -> Self {
+        self.flatMap { ["-Xcc", $0] }
     }
 
-    /// Extra flags to pass to Swift compiler.
-    public var swiftCompilerFlags: [String] {
-        var flags = self.flags.cCompilerFlags.flatMap({ ["-Xcc", $0] })
-        flags += self.flags.swiftCompilerFlags
-        if self.verboseOutput {
-            flags.append("-v")
-        }
-        return flags
+    /// Converts a set of C++ compiler flags into an equivalent set to be
+    /// indirected through the Swift compiler instead.
+    func asSwiftcCXXCompilerFlags() -> Self {
+        _ = self.flatMap { ["-Xcxx", $0] }
+        // TODO: Pass -Xcxx flags to swiftc (#6491)
+        // Remove fatal error when downstream support arrives.
+        fatalError("swiftc does support -Xcxx flags yet.")
     }
 
-    /// Extra flags to pass to linker.
-    public var linkerFlags: [String] {
+    /// Converts a set of linker flags into an equivalent set to be indirected
+    /// through the Swift compiler instead.
+    ///
+    /// Some arguments can be passed directly to the Swift compiler. We omit
+    /// prefixing these arguments (in both the "-option value" and
+    /// "-option[=]value" forms) with "-Xlinker". All other arguments are
+    /// prefixed with "-Xlinker".
+    func asSwiftcLinkerFlags() -> Self {
         // Arguments that can be passed directly to the Swift compiler and
         // doesn't require -Xlinker prefix.
         //
@@ -63,25 +62,39 @@ extension BuildParameters {
         let directSwiftLinkerArgs = ["-L"]
 
         var flags: [String] = []
-        var it = self.flags.linkerFlags.makeIterator()
+        var it = self.makeIterator()
         while let flag = it.next() {
             if directSwiftLinkerArgs.contains(flag) {
-                // `-L <value>` variant.
+                // `<option> <value>` variant.
                 flags.append(flag)
                 guard let nextFlag = it.next() else {
                     // We expected a flag but don't have one.
                     continue
                 }
                 flags.append(nextFlag)
             } else if directSwiftLinkerArgs.contains(where: { flag.hasPrefix($0) }) {
-                // `-L<value>` variant.
+                // `<option>[=]<value>` variant.
                 flags.append(flag)
             } else {
                 flags += ["-Xlinker", flag]
             }
         }
         return flags
     }
+}
+
+extension BuildParameters {
+    /// Returns the directory to be used for module cache.
+    public var moduleCache: AbsolutePath {
+        get throws {
+            // FIXME: We use this hack to let swiftpm's functional test use shared
+            // cache so it doesn't become painfully slow.
+            if let path = ProcessEnv.vars["SWIFTPM_TESTS_MODULECACHE"] {
+                return try AbsolutePath(validating: path)
+            }
+            return buildPath.appending("ModuleCache")
+        }
+    }
 
     /// Returns the compiler arguments for the index store, if enabled.
     func indexStoreArguments(for target: ResolvedTarget) -> [String] {
@@ -135,7 +148,7 @@ extension BuildParameters {
         else {
             return nil
         }
-        return args.flatMap { ["-Xlinker", $0] }
+        return args.asSwiftcLinkerFlags()
     }
 
     /// Returns the scoped view of build settings for a given target.
diff --git a/Sources/CoreCommands/SwiftTool.swift b/Sources/CoreCommands/SwiftTool.swift
@@ -680,15 +680,13 @@ public final class SwiftTool {
             let dataPath = self.scratchDirectory.appending(
                 component: destinationTriple.platformBuildPathComponent(buildSystem: options.build.buildSystem)
             )
-            var buildFlags = options.build.buildFlags
-            buildFlags.append(destinationToolchain.extraFlags)
 
             return try BuildParameters(
                 dataPath: dataPath,
                 configuration: options.build.configuration,
                 toolchain: destinationToolchain,
                 destinationTriple: destinationTriple,
-                flags: buildFlags,
+                flags: options.build.buildFlags,
                 pkgConfigDirectories: options.locations.pkgConfigDirectories,
                 architectures: options.build.architectures,
                 workers: options.build.jobs ?? UInt32(ProcessInfo.processInfo.activeProcessorCount),
diff --git a/Sources/PackageModel/BuildFlags.swift b/Sources/PackageModel/BuildFlags.swift
@@ -40,20 +40,4 @@ public struct BuildFlags: Equatable, Encodable {
         self.linkerFlags = linkerFlags
         self.xcbuildFlags = xcbuildFlags
     }
-    
-    /// Appends corresponding properties of a different `BuildFlags` value into `self`.
-    /// - Parameter buildFlags: a `BuildFlags` value to merge flags from.
-    public mutating func append(_ buildFlags: BuildFlags) {
-        cCompilerFlags += buildFlags.cCompilerFlags
-        cxxCompilerFlags += buildFlags.cxxCompilerFlags
-        swiftCompilerFlags += buildFlags.swiftCompilerFlags
-        linkerFlags += buildFlags.linkerFlags
-
-        if var xcbuildFlags, let newXcbuildFlags = buildFlags.xcbuildFlags {
-            xcbuildFlags += newXcbuildFlags
-            self.xcbuildFlags = xcbuildFlags
-        } else if let xcbuildFlags = buildFlags.xcbuildFlags {
-            self.xcbuildFlags = xcbuildFlags
-        }
-    }
 }
diff --git a/Sources/PackageModel/UserToolchain.swift b/Sources/PackageModel/UserToolchain.swift
@@ -463,13 +463,15 @@ public final class UserToolchain: Toolchain {
         }
 
         self.triple = triple
-        self.extraFlags = BuildFlags()
-
-        self.extraFlags.swiftCompilerFlags = try Self.deriveSwiftCFlags(
-            triple: triple,
-            destination: destination,
-            environment: environment
-        )
+        self.extraFlags = BuildFlags(
+            cCompilerFlags: destination.toolset.knownTools[.cCompiler]?.extraCLIOptions ?? [],
+            cxxCompilerFlags: destination.toolset.knownTools[.cxxCompiler]?.extraCLIOptions ?? [],
+            swiftCompilerFlags: try Self.deriveSwiftCFlags(
+                triple: triple,
+                destination: destination,
+                environment: environment),
+            linkerFlags: destination.toolset.knownTools[.linker]?.extraCLIOptions ?? [],
+            xcbuildFlags: destination.toolset.knownTools[.xcbuild]?.extraCLIOptions ?? [])
 
         self.librarianPath = try UserToolchain.determineLibrarian(
             triple: triple,
@@ -481,14 +483,8 @@ public final class UserToolchain: Toolchain {
         )
 
         if let sdkDir = destination.pathsConfiguration.sdkRootPath {
-            self.extraFlags.cCompilerFlags = [
-                triple.isDarwin() ? "-isysroot" : "--sysroot", sdkDir.pathString,
-            ] + (destination.toolset.knownTools[.cCompiler]?.extraCLIOptions ?? [])
-
-            self.extraFlags.cxxCompilerFlags = destination.toolset.knownTools[.cxxCompiler]?.extraCLIOptions ?? []
-        } else {
-            self.extraFlags.cCompilerFlags = (destination.toolset.knownTools[.cCompiler]?.extraCLIOptions ?? [])
-            self.extraFlags.cxxCompilerFlags = (destination.toolset.knownTools[.cxxCompiler]?.extraCLIOptions ?? [])
+            let sysrootFlags = [triple.isDarwin() ? "-isysroot" : "--sysroot", sdkDir.pathString]
+            self.extraFlags.cCompilerFlags.insert(contentsOf: sysrootFlags, at: 0)
         }
 
         if triple.isWindows() {
diff --git a/Sources/XCBuildSupport/XcodeBuildSystem.swift b/Sources/XCBuildSupport/XcodeBuildSystem.swift
@@ -193,21 +193,22 @@ public final class XcodeBuildSystem: SPMBuildCore.BuildSystem {
         settings["LIBRARY_SEARCH_PATHS"] = "$(inherited) \(try buildParameters.toolchain.toolchainLibDir.pathString)"
         settings["OTHER_CFLAGS"] = (
             ["$(inherited)"]
-            + buildParameters.toolchain.extraFlags.cCompilerFlags
+            + buildParameters.toolchain.extraFlags.cCompilerFlags.map { $0.spm_shellEscaped() }
             + buildParameters.flags.cCompilerFlags.map { $0.spm_shellEscaped() }
         ).joined(separator: " ")
         settings["OTHER_CPLUSPLUSFLAGS"] = (
             ["$(inherited)"]
-            + buildParameters.toolchain.extraFlags.cxxCompilerFlags
+            + buildParameters.toolchain.extraFlags.cxxCompilerFlags.map { $0.spm_shellEscaped() }
             + buildParameters.flags.cxxCompilerFlags.map { $0.spm_shellEscaped() }
         ).joined(separator: " ")
         settings["OTHER_SWIFT_FLAGS"] = (
             ["$(inherited)"]
-            + buildParameters.toolchain.extraFlags.swiftCompilerFlags
+            + buildParameters.toolchain.extraFlags.swiftCompilerFlags.map { $0.spm_shellEscaped() }
             + buildParameters.flags.swiftCompilerFlags.map { $0.spm_shellEscaped() }
         ).joined(separator: " ")
         settings["OTHER_LDFLAGS"] = (
             ["$(inherited)"]
+            + buildParameters.toolchain.extraFlags.linkerFlags.map { $0.spm_shellEscaped() }
             + buildParameters.flags.linkerFlags.map { $0.spm_shellEscaped() }
         ).joined(separator: " ")
 
diff --git a/Tests/BuildTests/BuildPlanTests.swift b/Tests/BuildTests/BuildPlanTests.swift
diff --git a/Tests/BuildTests/MockBuildTestHelper.swift b/Tests/BuildTests/MockBuildTestHelper.swift
