[Toolchains] Cygwin toolchain inherits from Unix

[modocache]

What's in this pull request?

There is currently a great deal of duplication across the GenericUnix and Windows toolchains. The Android port in #1442 will add an Android toolchain with even more duplication.

To mitigate this, have Windows inherit from GenericUnix, and have them share most of their implementation.

In addition, rename Windows to Cygwin (it would be pretty strange to have a Windows toolchain inherit from something named *Unix).

Resolved bug number: None

---

This is meant to address @jrose-apple's comments in #1442 (comment). This is probably the most C++ I've ever written in my life, so apologies in advance if it isn't any good! 😅 Virtual methods that take mutable references to ArgStringList seem like a bad idea, for example.

I'm also not a fan of how Cygwin overrides GenericUnix in order to prevent begin/end objects from being added to the linker inputs. Perhaps a better option would be to define a new base class:

toolchains::GenericUnix  # => Links the Swift runtime
    toolchains::Linux    # => Adds begin/end objects, conditionally uses gold linker, sets rpath
    toolchains::Cygwin   # => Sets rpath
    toolchains::Android  # => Adds begin/end objects, conditionally uses gold linker, links libgcc/libc++

Still, this is different from what @jrose-apple and others discussed in #1442 (comment), so I thought I'd submit this for feedback.

---

Before merging this pull request to apple/swift repository:

• Test pull request on Swift continuous integration.

Triggering Swift CI

The swift-ci is triggered by writing a comment on this PR addressed to the GitHub user @swift-ci. Different tests will run depending on the specific comment that you use. The currently available comments are:

Smoke Testing

Platform	Comment
All supported platforms	@swift-ci Please smoke test
OS X platform	@swift-ci Please smoke test OS X platform
Linux platform	@swift-ci Please smoke test Linux platform

Validation Testing

Platform	Comment
All supported platforms	@swift-ci Please test
OS X platform	@swift-ci Please test OS X platform
Linux platform	@swift-ci Please test Linux platform

Note: Only members of the Apple organization can trigger swift-ci.

[jrose-apple]

How does the Cygwin port handle protocol conformances anyway? Or do they just not care yet?

[modocache]

I'm honestly not sure. I'm submitting this change mainly as part of #1442 for Android. Perhaps @tinysun212 knows?

[tinysun212]

Cygwin does not need the begin/end object because it fetch the section size directly from COFF header without the exported variable. The gold linker can't be used in Cygwin. Your patch will be work.

[tkremenek]

@swift-ci test

[modocache]

Thanks, @tinysun212! @jrose-apple, any thoughts here? Is this acceptable or should I go with the broader class hierarchy I describe above?

[modocache]

@jrose-apple Friendly ping! I'd like to try to get #1442 merged (constantly rebasing it is a pain), and this seems like one of the last major issues involved. Is there anything I can improve upon here?

[jrose-apple]

Sorry, been swamped with #1878-related work. If I don't get to this today I'll be sure to do it Monday morning.

[modocache]

Thanks!! Sorry to pester you. 🙇

[jrose-apple]

Hm. I'm a little concerned about having too many axes of flexibility here; if I want to make a change, which of the several extension points do I override?

What do you think about having flags that control behavior instead?

virtual bool shouldProvideRPath() const { return true; }
virtual StringRef getPreInputObjectPath(…) const { return ""; }
// …

The obvious downside is that we could end up with a zillion of these, especially for things that are only relevant to one platform. The Android "links libgcc and libc++" seems like a pretty standard thing to do, but the most natural thing I can think of there would be something like getExtraLinkLibraries, which is only a step below getExtraLinkArguments.

(I do think whichever solution we go with, the names of the methods need to make it clear that these are about the link arguments. The base constructInvocation name is generic because it's overloaded, but these don't have any way to distinguish.)

---

I also (again) don't want to be politically incorrect by lumping FreeBSD under Linux; I suspect those two will diverge but meanwhile they have a lot in common. So that's a point against your "flatter" hierarchy. (Originally that was the motivation behind "GenericUnix" but…)

[jrose-apple]

I can go either way on this, so if you'd rather go with this form then I think it's probably good enough to just rename the methods. I thought about asking to merge the "arguments" and "arguments before inputs" cases, but I guess it makes organizational sense the way things are now.

[modocache]

Thanks for the feedback!

Yes, I also considered more granular controls such as shouldProvideRPath, but was concerned since I wasn't sure how generic some of these methods were. getPreInputObjectPath(), for example: Is adding an object to the beginning of the list of inputs something drivers normally do? As a newcomer to this sort of thing I wasn't sure.

Now that I see it isn't such a crazy idea, I think I'll implement this in that way. As you say, the current set of changes make it unclear which extension point is best for some cases.

I'll work on this now, expect an update sometime in the next few hours!

[jrose-apple]

I'm less worried about what other drivers do—Clang's driver is a mess; Swift copied over some of that mess—and would rather focus on what's going to be more maintainable for Swift going forward. The "arbitrary argument" extension points might be easier for out-of-tree platforms, but I'm worried it'll lead to arguments being added in arbitrary places, which means we'll at some point add them in the wrong place (cf. addCommonFrontendArgs).

Hopefully we won't hit the other extreme with a rats' nest of conditionals in the GenericUnix toolchain.

[modocache]

@jrose-apple Updated! Thanks for the advice, I think this revision is much nicer. I'm running the tests locally on my Ubuntu 15.10 machine, so far so good. :)

[jrose-apple]

C++ note: please use StringRef here (no const needed), so that we reference the data in the existing SmallString. This is an implicit conversion in LLVM, so no need to change the call site.

[jrose-apple]

Yeah, I'm liking this a lot better. Thanks, Brian!

[modocache]

@jrose-apple OK, I've hopefully addressed all of your comments. One thing to note: to use llvm::sys::path::append(), I needed to cast the StringRef arguments to SmallString<128>:

/home/modocache/GitHub/apple/swift/lib/Driver/ToolChains.cpp:1232:3: error: no matching function for call to 'append'
  llvm::sys::path::append(PostInputObjectPath, "swift_end.o");
  ^~~~~~~~~~~~~~~~~~~~~~~
/home/modocache/GitHub/apple/llvm/include/llvm/Support/Path.h:153:6: note: candidate function not viable: no known conversion from 'llvm::StringRef' to 'SmallVectorImpl<char> &' for 1st argument
void append(SmallVectorImpl<char> &path, const Twine &a,
     ^

I'm still getting familiar with llvm/ADT, so I'm not sure if this was something you anticipated. Anyway just a heads up!

[jrose-apple]

Oh, that's not correct. The way llvm::sys::path::append works is it modifies its first argument, so either you should use the argument to the function as an out parameter (and not return anything, or return a bool), or you should make a local SmallString as a buffer and then return it as a std::string.

[jrose-apple]

*actually reads code* Ah, what you did is the second option. I suppose this is a "cast" in C++ terminology; "conversion" would have scared me less.

[jrose-apple]

Nitpicky style thing: line-wrap indents should be two levels (four spaces) when not aligning with an "open" character on the previous line.

And,

@swift-ci Please test

[modocache]

line-wrap indents should be two levels (four spaces) when not aligning with an "open" character on the previous line.

Ah, you're referring to code like the following, right?

std::string getPreInputObjectPath(
       StringRef RuntimeLibraryPath) const override;

I'll update these indentations to 4 spaces once CI completes (I think updating in the middle of a CI job will cancel it, not sure).

[jrose-apple]

Yep. CI checks passed, so ready for updates. (This also applies to wrapped code lines within functions according to the LLVM style guide.)

[modocache]

@jrose-apple Amended the commit with the whitespace fixes 👍

[modocache]

Thanks! I'll apply this to #1442. Getting closer!