[Driver] Always link compiler_rt on Darwin #17843

jrose-apple · 2018-07-09T20:44:33Z

Turns out it's needed for normal builtins that can appear in inlinable functions, including Objective-C's @available. Clang always links it unconditionally, so so should Swift.

~~Note that this does mean you have to build compiler_rt to get a successful test run on Apple platforms. That was always true if you wanted the sanitizer tests to work, though.~~

rdar://problem/41911599

@available

Turns out it's needed for normal builtins that can appear in inlinable functions, including Objective-C's @available. Clang always links it unconditionally, so so should Swift. Note that this does mean you have to build compiler_rt to get a successful test run on Apple platforms. That was always true if you wanted the sanitizer tests to work, though. rdar://problem/41911599

jrose-apple · 2018-07-09T20:44:41Z

@swift-ci Please test

swift-ci · 2018-07-09T21:50:41Z

Build failed
Swift Test OS X Platform
Git Sha - 64b3d88

Tweak the tests to check this correctly.

jrose-apple · 2018-07-10T19:46:20Z

@swift-ci Please test

swift-ci · 2018-07-10T19:48:07Z

Build failed
Swift Test OS X Platform
Git Sha - 64b3d88

swift-ci · 2018-07-10T19:48:31Z

Build failed
Swift Test Linux Platform
Git Sha - 64b3d88

jrose-apple · 2018-07-11T02:12:42Z

swiftlang/swift-source-compat-suite#213
@swift-ci Please test source compatibility

davidungar

I know this isn't a huge change, but that constructInvocation function really needs to be broken up. All my other comments are optional. I realize this change is merely adding to the long function, but I would still feel so much better if you could take this opportunity to improve it.

davidungar · 2018-07-11T21:07:21Z

lib/Driver/DarwinToolChains.cpp

+  case DarwinPlatformKind::IPhoneOSSimulator:
+    if (distinguishSimulator)
+      return "iossim";
+    LLVM_FALLTHROUGH;
  case DarwinPlatformKind::IPhoneOS:
    return "ios";


What you really want here IMO is Swift's switch on tuple, so you could switch on both.
Failing that, it would be nice to make the symmetry wrt distinguishSimulator more apparent.
E.g.:

DarwinPlatformKind mapSimulatorToOS(DarwinPlatformKind kind) { switch kind { case DarwinPlatformKind::MacOS: return DarwinPlatformKind::MacOS case IPhoneOSSimulator: return IPhoneOS case IPhoneOS: return IPhoneOS ... } static StringRef getDarwinLibraryNameSuffixForTriple(const llvm::Triple &triple, bool distinguishSimulator = true) { const DarwinPlatformKind kind = getDarwinPlatformKind(triple) const DarwinPlatformKind effectiveKind = distinguishSimulator ? kind : mapSimulatorToOS(kind) switch (effectiveKind) { case DarwinPlatformKind::MacOS: return "osx"; case DarwinPlatformKind::IPhoneOSSimulator: return "iossim"; case DarwinPlatformKind::IPhoneOS: return "ios"; ... }

I like this because IMO it reduces the possibility for future errors.

davidungar · 2018-07-11T21:12:15Z

lib/Driver/DarwinToolChains.cpp

+        ".a");
+  if (llvm::sys::fs::exists(CompilerRTPath))
+    Arguments.push_back(context.Args.MakeArgString(CompilerRTPath));
+


The routine this is in, constructInvocation is already too long for me to understand without breaking it down. At the least, it would be great to factor this paragraph out into a separate function. At best, in order to ease the way for others to work on this code, it would help a lot to break this constructInvocation function down into smaller pieces.

I definitely don't want to do that in a cherry-pick to 4.2, and I don't want to do it without a guiding principle for grouping arguments, which I don't think we have today.

Ahh, yes, I understand about cherry-picking. Sigh. Will check the other box.

davidungar · 2018-07-11T21:13:31Z

test/Driver/linker-clang_rt.swift

@@ -0,0 +1,36 @@
+// REQUIRES: OS=macosx


What is this test testing? What is essential vs incidental? Comments to that effect would be helpful.

davidungar · 2018-07-11T21:14:27Z

test/lit.cfg

@@ -945,8 +945,9 @@ def source_compiler_rt_libs(path):
                                        if lib.startswith('libclang_rt.')
                                        and config.compiler_rt_platform in lib])

-source_compiler_rt_libs(make_path(test_resource_dir, 'clang', 'lib',
-                        platform.system().lower()))
+compiler_rt_dir = make_path(test_resource_dir, 'clang', 'lib',


Would it be possible to add a comment to explain the why here?

Explain what why?

LOL! I need to add a comment to explain my request for a comment!
My thought was to add comments to this file to explain what is going on w.r.t. the new feature being added. (I know nothing about lit.cfg files and thought maybe there were breadcrumbs you could add.)

This feels like another case where there's not a reasonable comment. This part didn't change, and the part below builds a path and tests if it exists.

davidungar · 2018-07-11T21:15:59Z

validation-test/Driver/Inputs/clang_rt-helper.h

@@ -0,0 +1,9 @@
+#include <stdbool.h>
+
+static inline bool testNewOS() {


Is this testing the OS, or testing the availability of the OS? If the latter, the current name could be improved.

davidungar · 2018-07-11T21:16:44Z

validation-test/Driver/clang_rt.swift

+
+// Just make sure we can build and link successfully.
+
+if testNewOS() {


Or maybe the name should be isTestingNewOS?

It's a dummy function and we don't run the code, so it doesn't actually matter, but sure.

davidungar

Too bad this will be cherry-picked and so too risky to bust up constructInvocation. Thanks for adding the other clarifications (i.e. comments, and refactoring the switch).

And use it in getDarwinLibraryNameSuffixForTriple, making the logic there clearer. Suggested by David U!

Per feedback from David U.

jrose-apple · 2018-07-11T22:12:56Z

@swift-ci Please test

jrose-apple · 2018-07-11T22:13:02Z

@swift-ci Please test source compatibility

swift-ci · 2018-07-11T22:14:41Z

Build failed
Swift Test OS X Platform
Git Sha - d466883

swift-ci · 2018-07-11T22:14:42Z

Build failed
Swift Test Linux Platform
Git Sha - d466883

[Driver] Always link compiler_rt on Darwin (when available) rdar://problem/41911599 (cherry picked from commit b02d554)

jrose-apple requested review from davidungar, hyp and ahatanaka July 9, 2018 20:44

[Driver] Only link to compiler_rt if present for the target platform

d466883

Tweak the tests to check this correctly.

davidungar suggested changes Jul 11, 2018

View reviewed changes

davidungar reviewed Jul 11, 2018

View reviewed changes

davidungar approved these changes Jul 11, 2018

View reviewed changes

jrose-apple added 2 commits July 11, 2018 15:09

Add swift::getNonSimulatorPlatform for DarwinPlatformKind

d11704b

And use it in getDarwinLibraryNameSuffixForTriple, making the logic there clearer. Suggested by David U!

[test] Tidy up newly-added clang_rt Driver tests to be a bit clearer

51f6e9a

Per feedback from David U.

jrose-apple merged commit b02d554 into swiftlang:master Jul 12, 2018

jrose-apple deleted the batteries-included branch July 12, 2018 00:35

jrose-apple added a commit that referenced this pull request Jul 12, 2018

Merge pull request #17843 from jrose-apple/batteries-included (#17896)

abad917

[Driver] Always link compiler_rt on Darwin (when available) rdar://problem/41911599 (cherry picked from commit b02d554)

		@@ -0,0 +1,9 @@
		#include <stdbool.h>

		static inline bool testNewOS() {


		// Just make sure we can build and link successfully.

		if testNewOS() {

[Driver] Always link compiler_rt on Darwin #17843

[Driver] Always link compiler_rt on Darwin #17843

Uh oh!

Conversation

jrose-apple commented Jul 9, 2018 • edited Loading Uh oh! There was an error while loading. Please reload this page.

Uh oh!

Uh oh!

jrose-apple commented Jul 9, 2018

Uh oh!

swift-ci commented Jul 9, 2018

Uh oh!

jrose-apple commented Jul 10, 2018

Uh oh!

swift-ci commented Jul 10, 2018

Uh oh!

swift-ci commented Jul 10, 2018

Uh oh!

jrose-apple commented Jul 11, 2018

Uh oh!

davidungar left a comment

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

Choose a reason for hiding this comment

Uh oh!

davidungar left a comment

Choose a reason for hiding this comment

Uh oh!

jrose-apple commented Jul 11, 2018

Uh oh!

jrose-apple commented Jul 11, 2018

Uh oh!

swift-ci commented Jul 11, 2018

Uh oh!

swift-ci commented Jul 11, 2018

Uh oh!

Uh oh!

jrose-apple commented Jul 9, 2018 •

edited

Loading