Fix duplicated swiftc flags when using custom sdks

rauhul · rauhul · commit 5ad71d52f746 · 2023-04-27T00:49:20.000-07:00
- Fixes a bug which caused extraCLIFlags from an SDK's toolset.json
  files to be repeated multiple times in child process invocations. The
  repeated flags can cause issues with some downstream tools, such as
  repeated -segaddr flags to ld64 for the same segment.
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
@@ -424,6 +424,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
@@ -506,7 +511,15 @@ 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()
+
+        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 {
@@ -621,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))
@@ -656,8 +674,19 @@ 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()
+
+        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
@@ -42,58 +42,70 @@ extension AbsolutePath {
     }
 }
 
-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 { ["-Xcc", $0] }
     }
 
-    /// 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.
         //
         // We do this to avoid sending flags like linker search path at the end
         // of the search list.
-        let directSwiftLinkerArgs = ["-L"]
+        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] {
@@ -147,7 +159,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
