LGTM

lgtm.

lgtm.

lgtm

lgtm

lgtm

lgtm

lgtm

LGTM. We can look at splitting it up again once we know what the dependencies are.

I have a slight preference for -arcmt-action=, but up to you if you want to change it. Otherwise LGTM.

lgtm with the fix I mentioned.

LGTM with KeyPathPrefix moved to the patch that actually uses it.

The reason KillTheDoctor exists is because of test-suite. It compiles and runs programs that do not contain calls to that API, and can do so with non-llvm compilers. I personally don't use it anymore as I don't run test-suite on Windows anymore, but I'm still not aware of any way short of changing registry settings to run test-suite on Windows without hanging other than this.

lgtm.

In D79998#2038430, @dang wrote:

Should I add the test here or in the clang-scan-deps patch?

In D80172#2042946, @rnk wrote:

I'd be fine going back to the behavior from before.

Without this behavior there is no intrinsic for 32 bit rotates on these platforms.

Strictly speaking, there seems to be __builtin_rotate(left|right)32, which is portable to all clang platforms.

I mostly just need to rebase this patch now. I'll try to get to that soon.

LGTM

Added a test.

Not really. It's a static function in MemoryBuffer.cpp, and the MemoryBuffer class doesn't have a Kind member so we can't check for MemoryBufferMMapFile.

In D77697#1970586, @dexonsmith wrote:

In D77697#1969998, @compnerd wrote:

@dexonsmith - yeah, sadly I dont think that there is a good way to audit that - any change to the public headers can cause issues. Furthermore, the libc headers themselves also influence this.

For auditing, can you use llvm-bcanalyze to see which headers are claimed by which PCM? If not, we should probably add a clang-pcm tool or something to help inspect module contents. @Bigcheese, thoughts?

I guess the obvious concern about this is that this is a game of whack-a-mole. If the headers change, you may need to shuffle module order again, at which point, which version of libc should we make libc++ work against? And different libc implementations could need different orders. But I'm just pointing it out; I don't have a problem with this patch landing.

LGTM for llvm/{lib,include/llvm}/Object/* and llvm/tools/llvm-readobj/*.

committed as a20862307fff9385bbf6562de3154d05138f8bec.

Sorry for the delay. LGTM. I'll commit with you as the author.

In D69575#1906078, @Ladd wrote:

@Bigcheese -- any more thoughts on this one? It would be helpful to have some version of this fix in the release compiler.

Cleaned up the test to not reference unused paths.

In D75395#1901778, @dexonsmith wrote:

This is used when
converting an implicit build to an explicit build to match the
systemness the implicit build would have had for a given module.

I had another thought. What if for the explicitly built "system" modules:

• If -Wsystem-headers is on, leave them as user modules in the explicit build.
• If -Wsystem-headers is off, turn off all diagnostics in the explicit build.

Does that give the right semantics, or is there something subtly different?

I pushed a commit fixing this in rG0b6abe428164 by just excluding the header.

I'm pretty sure this commit broke the modules build on non-Windows. WindowsSupport.h is now part of the LLVM_Util module which gets built on all platforms. You need to update the modulemap to put it in its own module, or wrap it in #ifdef.

lgtm

The aforementioned review is at https://reviews.llvm.org/D74892. I'm currently verifying it fixes everything for us. I'll update you when you're good to recommit this, which should happen this week.

Confirmed that CMake has a bug here: https://github.com/Kitware/CMake/blob/master/Source/cmLocalGenerator.cxx#L1780

This change also breaks MacOS, as -arch doesn't get passed to these invocations. Possibly CMAKE_OSX_ARCHITECTURES not being handled correctly within CMake.