Improve handling of tool build flags #6475

rauhul · 2023-04-24T23:33:37Z

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 SDK's 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.

MaxDesiatov

Can we add a test to verify that #6380 doesn't regress with this change?

MaxDesiatov · 2023-04-25T16:58:24Z

I admit, it was on me to add a test in #6380, I guess I can add it in a separate PR myself.

This one still could benefit from a test that verifies that duplicate flags bug is fixed.

rauhul · 2023-04-27T07:50:06Z

Do not test yet please, I need to update some unit tests still

rauhul · 2023-04-27T07:56:16Z

Sources/Build/BuildDescription/SwiftTargetBuildDescription.swift

+        // 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()


@compnerd @MaxDesiatov I'd really like to get some feedback here. I'm not sure if passing -Xcxx flags to swiftc is ok for all swift targets, but it seems maybe reasonable to do because the previous implementation passed the -Xcc flags (via buildParameters.swiftCompilerFlags).

The -Xcc flags can be needed when compiling a swift target to ensure additional args make it to the clang importer. For example I pass -D__none__ and -D__MACH__ to workaround some funkiness in my armv7em-apple-none-macho build. Similarly I figure -Xcxx flags maybe should be passed to the clang importer when importing c++ modules/headers.

AFAICT by default the only change is a new trailing arg to swiftc: swiftc <old args> -Xcc -lc++ when compiling a an object this shouldn't matter and when linking I assume the linker doesn't eager link libraries if no symbols from it are used.

After reading #4994 I think the cxxCompiler flags should go to swiftc prefixed by -Xcxx however this option does not exist in swift-driver yet.

I think in absence of -Xcxx we probably shouldn't forward the flags here, but if needed we could add another set of flags which are the CXX flags that are "safe" to pass to SwiftPM.

I talked to @hyp this morning and it seems like we will be adding an -Xcxx to swift-driver some time in the future. When that happens we can start propagating spm's cxx flags to swift-driver.

rauhul · 2023-04-27T07:56:45Z

Sources/PackageModel/UserToolchain.swift

+                triple: triple,
+                destination: destination,
+                environment: environment),
+            linkerFlags: destination.toolset.knownTools[.linker]?.extraCLIOptions ?? [],


@MaxDesiatov toolset linker flags were previously getting lost here

Thanks, great catch!

rauhul · 2023-04-27T08:02:01Z

I will update the commit message/PR title tomorrow as well with more detail about the changes

neonichu · 2023-05-01T20:49:38Z

All the duplication in target build description makes me a bit sad, but it's not an issue introduced by this PR. We should really refactor all of this...

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.

rauhul · 2023-05-01T23:32:46Z

@swift-ci please smoke test

rauhul · 2023-05-01T23:32:53Z

@swift-ci please smoke test windows

rauhul · 2023-05-01T23:46:02Z

@swift-ci please smoke test windows

rauhul · 2023-05-01T23:49:40Z

@swift-ci please smoke test windows

rauhul · 2023-05-02T06:02:05Z

@swift-ci please smoke test macOS

MaxDesiatov · 2023-05-02T07:19:40Z

@swift-ci smoke test macos

rauhul · 2023-05-02T21:51:14Z

Requires swiftlang/sourcekit-lsp#742 to pass macOS smoke test

rauhul · 2023-05-03T03:58:25Z

@swift-ci please test macOS

rauhul · 2023-05-03T03:59:12Z

@swift-ci please smoke test macOS

MaxDesiatov

Thanks!

Cherry-pick of #6475. 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. (cherry picked from commit 151343e) # Conflicts: # Sources/Build/BuildPlan.swift # Tests/BuildTests/MockBuildTestHelper.swift --------- Co-authored-by: Rauhul Varma <[email protected]>

rauhul requested review from abertelrud, neonichu, tomerd and MaxDesiatov as code owners April 24, 2023 23:33

neonichu approved these changes Apr 24, 2023

View reviewed changes

MaxDesiatov requested changes Apr 25, 2023

View reviewed changes

rauhul force-pushed the dont-duplicate-toolset-flags branch from a1fc0ce to 0cf62db Compare April 27, 2023 07:49

rauhul force-pushed the dont-duplicate-toolset-flags branch from 0cf62db to 5ad71d5 Compare April 27, 2023 07:50

rauhul commented Apr 27, 2023

View reviewed changes

rauhul force-pushed the dont-duplicate-toolset-flags branch from 5ad71d5 to e44c050 Compare April 27, 2023 22:59

rauhul changed the title ~~Fix duplicated swiftc flags when using custom sdks~~ Improve handling of tool build flags Apr 27, 2023

tomerd assigned neonichu May 1, 2023

rauhul force-pushed the dont-duplicate-toolset-flags branch from e44c050 to e7c1d3e Compare May 1, 2023 23:32

rauhul closed this May 1, 2023

rauhul reopened this May 1, 2023

rauhul enabled auto-merge (squash) May 2, 2023 05:30

MaxDesiatov approved these changes May 3, 2023

View reviewed changes

rauhul merged commit 151343e into main May 3, 2023

rauhul deleted the dont-duplicate-toolset-flags branch May 3, 2023 11:41

MaxDesiatov mentioned this pull request Jul 14, 2023

[5.9] Improve handling of tool build flags #6709

Merged

Improve handling of tool build flags #6475

Improve handling of tool build flags #6475

Uh oh!

Conversation

rauhul commented Apr 24, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

MaxDesiatov left a comment

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov commented Apr 25, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

rauhul commented Apr 27, 2023

Uh oh!

rauhul Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

rauhul Apr 27, 2023 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Choose a reason for hiding this comment

Uh oh!

neonichu May 1, 2023

Choose a reason for hiding this comment

Uh oh!

rauhul May 1, 2023

Choose a reason for hiding this comment

Uh oh!

rauhul Apr 27, 2023

Choose a reason for hiding this comment

Uh oh!

MaxDesiatov May 1, 2023

Choose a reason for hiding this comment

Uh oh!

rauhul commented Apr 27, 2023

Uh oh!

neonichu commented May 1, 2023

Uh oh!

rauhul commented May 1, 2023

Uh oh!

rauhul commented May 1, 2023

Uh oh!

rauhul commented May 1, 2023

Uh oh!

rauhul commented May 1, 2023

Uh oh!

rauhul commented May 2, 2023

Uh oh!

MaxDesiatov commented May 2, 2023

Uh oh!

rauhul commented May 2, 2023

Uh oh!

rauhul commented May 3, 2023

Uh oh!

rauhul commented May 3, 2023

Uh oh!

MaxDesiatov left a comment

Choose a reason for hiding this comment

Uh oh!

Uh oh!

rauhul commented Apr 24, 2023 •

edited

Loading

MaxDesiatov commented Apr 25, 2023 •

edited

Loading

rauhul Apr 27, 2023 •

edited

Loading

rauhul Apr 27, 2023 •

edited

Loading