Detect whether cc (the linker) is gcc or clang. #17231

pnkfelix · 2014-09-13T09:12:57Z

Detect whether cc (the linker) is gcc or clang.

Use this information to drive which options are added to the cc
command line arguments.

As a drive-by, fix an english grammar mistake in a comment.

This is meant to be, in part, a more robust version of PR #17192.

nrc · 2014-09-13T09:27:30Z

src/librustc/back/link.rs

+        ClangArguments
+    } else if output.contains("gcc") {
+        GccArguments
+    } else if output.contains("Free Software Foundation") {


I would || these two clauses rather than have separate else if branches

will do (decided its not worth debating).

nrc · 2014-09-13T09:31:48Z

r=me with the two nits addressed. Not sure whether the third question is valid or not, r+ either way.

alexcrichton · 2014-09-13T14:39:56Z

src/librustc/back/link.rs

+        fail!("unrecognized linker version string: {:s}", output)
+    };
+
+    (prog_string, args_fmt)


I'm a little uncomfortable with fail!() because compilers can do really weird things sometimes. Could we use a UnknownArguments enum instead?

Also, invoking a process can be pretty expensive if it happens many times for a very small compilation, is there any way that we could cache this output to make sure we only run it once? It looks like get_cc_prog is called in a loop from a couple of places.

I'm pretty sure we can cache the info (e.g lazily computing it via a method on the session). I didn't feel like taking the time to implement that since I just wanted builds to work again on my home machine, but I will go ahead and add the lazy computation.

(as for the fail!, I was following a philosophy of fail-fast, but a lowest-common-denominator UnknownCcArgumentFormat is reasonable too. I'll switch to that.)

alexcrichton · 2014-09-13T14:41:18Z

Why can't we just limit this flag to windows? The only reason we're passing this flag on windows is to not pick up more runtime dependencies than necessary. I'm worried about invoking lots of processes and attempting to parse output as I've seen it add significantly to compile-time in the past for other projects.

I also would feel pretty uncomfortable if we went really far down the path of specializing for either gcc or clang.

vadimcn · 2014-09-13T19:33:43Z

I agree with Alex, spawning an extra process per compilation seems way too heavy, and it will slow down Rust test suite even more. If this is the best we can do, let's just limit this switch to OsWindows.

pnkfelix · 2014-09-13T22:02:03Z

I figure Limiting the flag to windows (rather than putting in an option filter like this) strikes me as adding yet another spot where behavior on windows is different; i.e. a potential source for strange windows-only errors.

(Plus, what would that imply for clang atop windows? Is that a contradiction? )

Will a memoization slot not address your concerns?

vadimcn · 2014-09-13T22:34:40Z

compiletest spawns rustc per test, doesn't it? So memorization won't help. And yes, clang on windows will be broken in any case.
I think that suppressing clang error via ARCHFLAGS might be the best option for the moment. And who knows, maybe they'll change their mind about removing the override. Can someone please try it? (Sorry don't have clang installed on my box).

pnkfelix · 2014-09-14T09:23:29Z

@vadimcn hold on: @alexcrichton's original concern was "invoking a process can be pretty expensive if it happens many times for a very small compilation"; and that I can understand, e.g. if this PR were to add many process spawns for a single compilation, that would be bad. And it would also be bad if it were to add just one process spawn where there were zero before.

And yes, compiletest is indeed spawning off a rustc per-test.

But with the memoization strategy, we are talking about adding one process spawn of cc --version for the first time that you attempt to build up the command line for invoking cc for the link step, which I assume follows soon thereafter. Shouldn't the time added for invoking cc --version and parsing its output be dwarfed by the time taken by linking, even for the smallest of compilations (and even on platforms where process spawns are relatively expensive)?

Where is my reasoning going wrong here?

If compiletest really is a concern, then I guess we could further attempt to allow compiletest to do this check regarding the backend driver and then pass that info along to rustc. But that's more infrastructure than I really feel like putting in.

It seems like clang is the one that is forcing our hand here, in terms of stating up front that passing these options is going to become a hard error in the future, and I am trying to prepare us for that reality in a robust way.

The detected categorization is memoized in the Session object. The link.rs backend then uses this information when choosing options to add to the `cc` command line arguments (currently it just affects a single option that will cause a hard error in clang in the future). This is meant to be, in part, a more robust version of PR rust-lang#17192. As drive-bys: * fix an english grammar mistake in a comment. * replace the use of `String` with `&str` for the program names corresponding `cc` and `ar`, avoiding unnecessary string copies but more importantly making the code overall look nicer. :) Fix rust-lang#17214. (Addressed review nits from nrc and acrichto.)

pnkfelix · 2014-09-14T10:35:28Z

r? @alexcrichton (if you agree with @vadimcn that this will add too much overhead to compiletest, then I will stop this train of development and let others figure out the appropriate solution here).

alexcrichton · 2014-09-14T21:18:23Z

If we were to take this route, this looks good to me. I'd prefer to see some data about small compilations and see what sort of overhead this adds. In the past I seem to recall that a noop process invocation is on the order of 3ms, but that may have just been the program in question

thestinger · 2014-09-15T03:43:21Z

Spawning a process on Windows takes ~60ms, so it's a bigger problem there. Why do we need to pass this flag to GCC anyway? Clang / LLVM also has a linker plugin, and it apparently doesn't cause any issues because it's not being disabled. I don't think it makes sense to have different behaviour on different compilers without a clear rationale.

vadimcn · 2014-09-15T03:54:08Z

We are passing it because I didn't want to bundle gcc's LTO linker plugin (which Rust doesn't use anyway) into Windows installer. On other platforms it was a drive-by change,- to avoid yet another special case for Windows.

alexcrichton · 2014-09-15T05:19:53Z

I've approved #17192 for now as it's the more conservative of these two changes, and I'm not sure that we want to take a hit of just running --version before all compilations.

pnkfelix · 2014-09-15T06:39:44Z

A perhaps reasonable way to avoid the hit entirely on Windows would be to look for "gcc" or "clang" as a suffix in the cc_prog string, and only resorting to the --version invocation when neither suffix is found. And then clients such as compiletest could indicate gcc/clang as the linker when they invoke rustc (to avoid the extra invocation of the cc --version default on non Windows targets).

pnkfelix · 2014-09-15T14:02:16Z

(closing this since I don not feel like trying to work on it more, at least not until some other clang incompatibility crops up.)

nrc reviewed Sep 13, 2014
View reviewed changes

pnkfelix force-pushed the fsk-detect-cc-variant branch from e991d6b to b2cc887 Compare September 13, 2014 09:48

pnkfelix mentioned this pull request Sep 13, 2014

Don't pass -fno-use-linker-plugin on OS X #17192

Merged

alexcrichton reviewed Sep 13, 2014
View reviewed changes

pnkfelix force-pushed the fsk-detect-cc-variant branch from b2cc887 to 4b54066 Compare September 14, 2014 10:33

pnkfelix closed this Sep 15, 2014

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Detect whether cc (the linker) is gcc or clang. #17231

Detect whether cc (the linker) is gcc or clang. #17231

pnkfelix commented Sep 13, 2014

nrc Sep 13, 2014

pnkfelix Sep 13, 2014

nrc commented Sep 13, 2014

alexcrichton Sep 13, 2014

pnkfelix Sep 13, 2014

pnkfelix Sep 14, 2014

alexcrichton commented Sep 13, 2014

vadimcn commented Sep 13, 2014

pnkfelix commented Sep 13, 2014

vadimcn commented Sep 13, 2014

pnkfelix commented Sep 14, 2014

pnkfelix commented Sep 14, 2014

alexcrichton commented Sep 14, 2014

thestinger commented Sep 15, 2014

vadimcn commented Sep 15, 2014

alexcrichton commented Sep 15, 2014

pnkfelix commented Sep 15, 2014

pnkfelix commented Sep 15, 2014

Detect whether cc (the linker) is gcc or clang. #17231

Detect whether cc (the linker) is gcc or clang. #17231

Conversation

pnkfelix commented Sep 13, 2014

nrc Sep 13, 2014

Choose a reason for hiding this comment

pnkfelix Sep 13, 2014

Choose a reason for hiding this comment

nrc commented Sep 13, 2014

alexcrichton Sep 13, 2014

Choose a reason for hiding this comment

pnkfelix Sep 13, 2014

Choose a reason for hiding this comment

pnkfelix Sep 14, 2014

Choose a reason for hiding this comment

alexcrichton commented Sep 13, 2014

vadimcn commented Sep 13, 2014

pnkfelix commented Sep 13, 2014

vadimcn commented Sep 13, 2014

pnkfelix commented Sep 14, 2014

pnkfelix commented Sep 14, 2014

alexcrichton commented Sep 14, 2014

thestinger commented Sep 15, 2014

vadimcn commented Sep 15, 2014

alexcrichton commented Sep 15, 2014

pnkfelix commented Sep 15, 2014

pnkfelix commented Sep 15, 2014