fix a comment

Thanks!

rename toolchain to "unix"

address comments

Thanks for the thorough review! I replied "not needed yet" to several items, I hope that's fine.

address comments

Nice!

variables too

zero symlinks, in exchange for longer gen command

fix bad exec_script_whitelist default

just one symlink

Comment from the peanut gallery:

• I like the whitelist model for gcc-style flags. It allows us to curate them (and /? output) since many don't make much sense in the cl world.
• I like the idea behind this patch (and /clang: seems like a good spelling)

In D52411#1260822, @aganea wrote:

This patch doesn't link on Windows/VS2017 as main() is not in the same library as clangDriver

Thanks for the fix! Do you happen to know what had regressed this?

Thanks!

add requested comment

update comments

Thanks for the fast review! All done.

address comments

It looks like you ran clang-format on all of lib/Sema/SemaExpr.cpp and changed many lines that are irrelevant to your patch. Can you undo that, please?

Thanks!

forgot to regen diff after latest changes, sorry.

Thanks! Will land with tweaked comment.

krasimir, do you do clang-format reviews these days? If not, do you know who does?

In D52773#1255093, @zturner wrote:

I agree magic environment variables are bad, but without it we don’t
address the only current actual use we have for this, which is making the
vs integration print filenames. Detecting compiler version from inside the
integration is hard

add warning test

Looks good. If this is passed and we invoke lld-link, should we give that a similar flag and pass that to lld-link as well? I think link.exe also prints its outputs.

Sounds great, thanks!

Thanks! lgtm.

Actually, trying this out with MSVC, I don't see any __chkstk calls with /O1, or with eg. /Gs1 for that matter:

Maybe file a bug on figuring out the /Gs story and add a FIXME linking to it. Weird.

...and to reword this a bit: Clang taking a long time to start up in some configurations is a bug we should profile and fix :-)

In D52193#1241067, @aganea wrote:

@thakis > clang-cl isn't supposed to do (explicit) registry accesses when you hold it right (pass in -fms-compatibility-version etc). Have you seen registry access costs, or is that speculation?

Please see this log:

clang-cl-log.zip5 KB

Download

- the child clang-cl -cc1 takes about ~117ms until it gets into the global initializers. This is on my Haswell PC. On the Skylake, this takes "only" ~60ms.
This probably explains why Ninja is slower on the Skylake when using clang-cl as a compiler. There should be a shorter codepath maybe when only a single .cpp is being compiled, and avoid running the child process.

In D52266#1240304, @hans wrote:

Sorry, I didn't realize we both set off to do this in parallel. I've incorporated your changes into my patch.

Since it helps existing msbuild configs, adding this seems like a good thing to me.

FWIW the recommendation against /Ox in my version is because of https://github.com/ulfjack/ryu/pull/70#issuecomment-412168459

In D52143#1236151, @zturner wrote:

Not formally in any kind of mailing list discussion

Thanks! Changed the comment; landing.

Thanks!

fix case on variable

The revert helped, see e.g. http://lab.llvm.org:8011/builders/lld-x86_64-darwin13/builds/25686 and http://lab.llvm.org:8011/builders/lld-x86_64-freebsd/builds/23378 (compare to previous build)

I reverted this (and follow-on fix attempts r342154 r342180 r342182 r342193) in r342336 in an attempt to heal the lld bots.

Ah, looks like all changes are mentioned at the top, just not as comments further down.

https://reviews.llvm.org/rLLD342334 too. Thanks!

Also https://reviews.llvm.org/rL342331 and https://reviews.llvm.org/rLLD342332

Thanks! Submitting...

In D51956#1235313, @llvm-commits wrote:

The symptoms of a collision are just going to be you can’t debug the
program, so not very severe imo, especially since it would almost certainly
be resolved on the next incremental build

Thanks!

ruiu comments

improve comment

r342248, thanks!

rebase, minor cleanups

In D51561#1234862, @zturner wrote:

All of the lit tests have this restriction, is this something you would do
with unit tests but not lit tests?

Unrelatedly, another problem with the current design is that llvm.srcdir.txt must contain an absolute path, which makes it impossible to build on one machine and then copy artifacts and test inputs to another machine and run tests on another machine. DebugInfoPDBTests is the only test in all llvm, lld, clang tests that has this restriction. Requiring some defined fixed pwd for unit tests with inputs would address this issue as well. (As would finding a way to not require reading a file off disk for this test.)

In D51561#1227163, @thakis wrote:

In D51561#1226583, @zturner wrote:

These aren't demangle tests fwiw. But that just complicates the cmake
logic, and I don't see the benefit of doing so. Writing 3k of data to the
disk is hardly going to be a noticeable part of the configure step.

The benefits are:

1. Unit tests reading inputs are bad form; doing that should require an explicit opt-in.
2. While writing 3k of data to disk isn't causing measurable perf issues, writing a text file next to every test binary just because one test binary needs it is still plain ugly.
   
   And not doing it is super easy to do. I don't understand the pushback here.

Actually, this replaces https://reviews.llvm.org/D51887

This replaces https://reviews.llvm.org/D51957.