The main reservation I have about landing this is whether to make it more generic and abstracted. I kind of feel like the main SymbolSerializer class should be powerful enough to do all these optimizations for us, or maybe some replacement for it.

I collected statistics:
https://bugs.chromium.org/p/chromium/issues/detail?id=904324#c4
The summary is that links in Chromium get 32% faster, but object files grow by 7%. There is no compile time impact. Check it out for more details.

lgtm

In D54554#1299963, @aganea wrote:

Very nice! You're talking about /DEBUG:GHASH, but your optimisation is also beneficial when GHASH isn't used?

I ended up finding these instructions accidentally when searching for other monorepo related stuff, and I followed these instructions, and they worked. Thanks for writing them up!

I'd say feel free to commit typo fixes like this in the future. I'd avoid fixing typos where there is spelling disagreement, like "prolog vs prologue", so just use your best judgement as always.
lgtm

It went green with my change here:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/1463

I think this broke the windows selfhost:
http://lab.llvm.org:8011/builders/clang-x64-windows-msvc/builds/1457/steps/stage%202%20build/logs/stdio

In D54328#1297688, @joerg wrote:

As I said: because it makes it harder to rerun parts of the test interactively. Which is a good enough reason to forbid cd - as well as any directory stack operations, IMO.

In D54328#1294801, @joerg wrote:

I'm quite against this. "cd -" is a non-portable extension to shell and should never be used in first place.

Sorry, I didn't realize this was waiting on my review. It looks like this is mostly about localescape, so let's change the patch description and code to be about that.

+1 for writing it as you have it, it's what clang-format does, and so far as I know it's the prevailing style.

Adding -Wno-cast-function-type seems reasonable, but we should do it more carefully.

In D54370#1294648, @aganea wrote:

@rnk Could you possibly post build metrics for Chromium (for the whole build process, compilation and link) with and without ghash?

lgtm

Can you add a quick test to the lit test suite for this? Shouldn't be too hard, make some dirs, cd into and out, pwd, filecheck the output.

Sorry, I lost track of this.

I guess you should update the commit message to say "monorepo-root" instead of "monorepo-top". Otherwise, neat!

Threading a new options argument through mangleType that includes QualifierMangleMode as well as these obj-c options seems reasonable.

lgtm

In D54153#1291543, @aganea wrote:

I managed to reproduce the issue with Ninja as well. It happens rarely, 1 out 5 runs.

• Elaborate on comdats in a comment

I was going to consult @zturner about the state of Python 3 in LLDB, but that's not really related to your change. I think it's fine. lgtm

I hadn't realized that Dexter knew how to drive VS tools. I'll have to go read more and get back to you all.

Traditionally, LLDB has resisted efforts to more tightly couple it with LLVM. Many of the contributors at Apple seem to prefer to build LLVM separately and then build against that with the LLDB xcode project. You can see how that would disincentivize you from contributing to LLVM first. And in fairness, sometimes LLVM people object to adding extra complexity to Support when it's only used in LLDB. And, as with lib/DebugInfo/DWARF, sometimes code gets hoisted to LLVM without refactoring LLDB to use it. So, there's plenty of work to go around. :)

In D54187#1289607, @aprantl wrote:

It would be nice if instead of having a set of windows-only tests, we could wrap cdb similar to llgdb.py wraps LLDB, so these tests run on all platforms. If the Windows debugger is just too different for this to make sense, let me know.

• Fix thinko

In D54129#1289509, @efriedma wrote:

The case I'm trying to fix is functions/funclets which don't have an epilogue at all. As far as I can tell, the x86 only emits SEH_Epilogue before an epilogue; I don't see how it's related.

• simplify

lgtm with some minor suggestions

In D51524#1288018, @efriedma wrote:

It works for x64 and x86, so I guess it's arm-specific. The state table emission code is portable, so maybe the bug is in the arm ip2state mapping logic?

Looked a bit more; it looks like the problem is actually that the "throw" is the last instruction in the function, so unwinding gets confused. There's some code in X86TargetMachine::X86TargetMachine which turns on TrapUnreachable, which avoids the problem. It looks like MSVC does something similar (it inserts a nop instead of a trap, but same idea). I'll post a patch separately.

We came up with a different solution for this called SEH_Epilogue. It basically inserts a nop if the last real instruction was a call. It's slightly complicated because it looks backwards across empty basic blocks and things. You can see it here:
https://github.com/llvm-mirror/llvm/blob/master/lib/Target/X86/X86MCInstLower.cpp#L1800

In D54153#1288824, @aganea wrote:

The issue is that XXX-tablegen-host .vcxprojects are explicitly calling cmake --build, thus not going through MSBuild's dependency graph. It looks like the Ninja generator doesn't do that.

In D51524#1286346, @efriedma wrote:

The following testcase is also broken, for reasons I don't understand. I don't think it's a problem with any of the code in this patch, though; the throw out of the "catch" block somehow picks the wrong catch block, but this patch doesn't touch that code.

void a() { try { try { throw 1; } catch (...) { throw 1; } } catch (...) {}}

lgtm

Looks good, but when you commit it, you probably will want to keep an eye out for warnings or build breakages in other configurations. This code will be compiled with GCC, MSVC, and clang, and I can imagine a few ways the WindowsSupport.h code could raise warnings.

In D51524#1285854, @efriedma wrote:

Reid, do you have an opinion on what test coverage is necessary here?

In D53912#1284804, @azharudd wrote:

I agree. This currently resolves issues with building for ARM64 target using Visual Studio 2017. The missing intrinsics it complains about are already present in intrin.h. We could add those to intrin0.h too but that would duplicate. Please let me know what you think.

lgtm