I think for depfile generation, we generally try to be gcc-compatible (I can link to prior changes in this spirit), so I think this seems like a good thing to do to me. gcc only does this for system headers, yes?

r312594, thanks!

fix a comment typo

Actually, I talked myself out of this affecting %LINK% / %_LINK_% while writing the patch summary. I removed that bit.

312216, thanks!

Thanks, landed in r312167.

Don't warn in unevaluated contexts. Ready for a look now.

add test from rnk

Just use TryTerminatedBlock

Many driver tests check in a basic representative directory structure (e.g. test/Driver/Inputs/basic_freebsd_tree/ and its many siblings).

This approach looks good to me.

Oh, I guess this is superseded by https://reviews.llvm.org/D36860 ?

Thanks!

r310376, thanks!

Why should this be part of llvm? This seems to come with very heavy dependencies (protobuf), and LLVM has historically tried to minimize the number of things it depends on.

Is landing this blocked on internal testing?

Hm, that's unfortunate. I don't see a good way to rescue this patch.

309984.

I'm going to land this. It's early in the 6.0 cycle, so if this causes issues, we should have time to find them and then follow up in case we run into any.

309980, thanks!

I've reverted this in 309960, as discussed.

(Our workaround is to call __builtin_available() once before engaging the sandbox, which isn't so bad. Just thought I'd let you know about it; this isn't a serious bug for us.)

Looks like the email reply didn't make it to phab, so here it is again:

We just noticed that if you call __builtin_available() for the first time after activating your app's sandbox, the function will fail:

Awesome, thanks! Maybe mention the PR# for this in the commit message.

dim: Does putting the target listing behind a different flag work for you? Which problem are you trying to solve here?

Thanks! r309231

r308998, thanks!

r308997, thanks!

In D33900#819234, @dim wrote:

In D33900#818957, @thakis wrote:

Sorry, I just noticed this weeks later. Why are we adding this to --version instead of adding some new flag for printing this? When I pass --version, I'm usually interested in clang's version and don't need a screenful of other information below it (which makes the output I do care about scroll off the screen).

I added it there, for consistency with other LLVM tools, for example:

r308897.

Sorry, I just noticed this weeks later. Why are we adding this to --version instead of adding some new flag for printing this? When I pass --version, I'm usually interested in clang's version and don't need a screenful of other information below it (which makes the output I do care about scroll off the screen).

Most of the patch is unifying all the toolchains to call the newly-introduced ToolChain::ShouldLinkCXXStdlib() instead of all manually checking for D.CCIsCXX() && !getFlag(nostdlib, nodefaultlibs). The actual behavior change is to make that function check the new nostdlib++ flag too.

From what I understand, _MSC_VER changes with each 2017 update.

r308544

(landed in r307867)

I went ahead and landed this in r308044, given that I addressed the nits and removed the possibly contentious bits. Happy to address remaining nits in a follow-up.

Mostly done, thanks!

sdy comments

arphaman comments

Thanks, all done, much better!

Document *, add example with multiple platforms

comments

r306442, thanks!

http://lab.llvm.org:8011/builders/llvm-clang-x86_64-expensive-checks-win/builds/2604
http://lab.llvm.org:8011/builders/clang-with-thin-lto-ubuntu/builds/3280

Either this or the reland of making allow_user_segv_handler to true broke this test on Windows:

The new test fails for me like so, at r303736:

This looks good to me, thanks. Sorry about the slow turnaround. Do you have commit access? If not, I can land it for you – but it also looks like you've contributed several patches by now, so you could also ask for commit access if you don't have it yet.

r302255, thanks!

s/Add one/All done/

Thanks! Add one and landed in r302247.

r301039, thanks.

Not sure if this is useful, but this is where I always check first.

r301032, thanks.

Looks good to me.

I was just confused again by this not working, and didn't remember this review for a while. From a user's point of view it feels like this should work, so I've gone ahead and landed this in r300692.

I'm out today. Since lld will be a client of the library code of this tool, Rui should probably take a look if he's around this week. I'd also already add a test with just a RUN line for running the tool without parameters, just to have test coverage from day one (makes it easier to add tests in the future once the tool actually does something).

Landed in r299952: http://lists.llvm.org/pipermail/cfe-commits/Week-of-Mon-20170410/189882.html

Looks good. Do you have commit access?

This breaks bootstrap builds. I put the error message in the revert commit description: http://lists.llvm.org/pipermail/llvm-commits/Week-of-Mon-20170410/444101.html

We have a test in chromium that loads a small so file and calls some of the functions therein. There are no allocations in the so file. The test used to run fine under tsan, now it fails with the error message you added. I'll disable the test under tsan, but maybe others out there where also calling non-allocating so's without problems before this change.

I think this landed in r297900.

Ah ok, thanks for explaining. In that case, this sounds fine and I'll leave the review to zturner.