[Modules] Fix rebuilding an updated module for each of its consumers.

[vsapsai]

Summary

Marking a module for a rebuild when its signature differs from the
expected one causes redundant module rebuilds for incremental builds.
When a module is updated, its signature changes. But its consumers still
have the old signature and loading them will result in signature
mismatches. It will correctly cause the rebuilds for the consumers but
we don't need to rebuild the common module for each of them as it is
already up to date.

In practice this bug causes longer build times. We are doing more work
than required and only a single process can build a module, so parallel
builds degrade to a single-process mode where extra processes are just
waiting on a file lock.

Fix by not marking a module dependency for a rebuild on signature
mismatch. We'll check if it is up to date when we load it.

rdar://problem/50212358

[vsapsai]

Earlier tryToDropPCM was called tryToRemoveBuffer and it was added in r298278. According to my investigation and understanding it was used to remove tentatively loaded dependencies. Otherwise we would finalize them before validating if they are up to date. But now we are tracking a state of each module (Unknown, Tentative, ToBuild, Final) individually and don't need tryToDropPCM.

[dexonsmith]

This LGTM.

The key insight you're making is that when you're importing B into A and B's signature doesn't match what A expects, that does not mean that B is out-of-date. B may have just been built and be totally correct. It only means that A is out-of-date. I had considered removing this call, but was confused about exactly what this would mean.

clang/lib/Serialization/ModuleManager.cpp

211 ↗

(On Diff #217729)

This is the only caller of FileManager::invalidateCache. I suggest deleting it in a follow-up, in order to reduce complexity in the FileManager.

[bruno]

Thanks for fixing this, LGTM!

[vsapsai]

clang/lib/Serialization/ModuleManager.cpp

211 ↗

(On Diff #217729)

Good suggestion. Agree it will be better to keep the FileManager simpler.

cfe/trunk/lib/Serialization/ModuleManager.cpp

ModuleManager::addModule(StringRef FileName, ModuleKind Type,
   }
 
   // Initialize the stream.
   NewModule->Data = PCHContainerRdr.ExtractPCH(*NewModule->Buffer);
 
   // Read the signature eagerly now so that we can check it.  Avoid calling
   // ReadSignature unless there's something to check though.
   if (ExpectedSignature && checkSignature(ReadSignature(NewModule->Data),
-                                          ExpectedSignature, ErrorStr)) {
-    // Try to remove the buffer.  If it can't be removed, then it was already
-    // validated by this process.
-    if (!getModuleCache().tryToDropPCM(NewModule->FileName))
-      FileMgr.invalidateCache(NewModule->File);
+                                          ExpectedSignature, ErrorStr))
     return OutOfDate;
-  }
 
   // We're keeping this module.  Store it everywhere.
   Module = Modules[Entry] = NewModule.get();
 
   updateModuleImports(*NewModule, ImportedBy, ImportLoc);
 
   if (!NewModule->isModule())
     PCHChain.push_back(NewModule.get());

cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/A.h

+// A
+#include "Common.h"

cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/B.h

+// B
+#include "Common.h"

cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/Common.h

+// Common

cfe/trunk/test/Modules/Inputs/implicit-invalidate-common/module.modulemap

+module A { header "A.h" }
+module B { header "B.h" }
+module Common { header "Common.h" }

cfe/trunk/test/Modules/implicit-invalidate-common.c

+// REQUIRES: shell
+// RUN: rm -rf %t
+// RUN: mkdir -p %t/implicit-invalidate-common
+// RUN: cp -r %S/Inputs/implicit-invalidate-common %t/
+// RUN: echo '#include "A.h"' > %t/A.c
+// RUN: echo '#include "B.h"' > %t/B.c
+
+// Build with an empty module cache. Module 'Common' should be built only once.
+//
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/Cache \
+// RUN:     -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN:     %t/A.c 2> %t/initial_build.txt
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/Cache \
+// RUN:     -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN:     %t/B.c 2>> %t/initial_build.txt
+// RUN: FileCheck %s --implicit-check-not "remark:" --input-file %t/initial_build.txt
+
+// Update module 'Common' and build with the populated module cache. Module
+// 'Common' still should be built only once. Note that we are using the same
+// flags for A.c and B.c to avoid building Common.pcm at different paths.
+//
+// RUN: echo ' // ' >> %t/implicit-invalidate-common/Common.h
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/Cache \
+// RUN:     -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN:     %t/A.c 2> %t/incremental_build.txt
+// RUN: %clang_cc1 -fmodules -fimplicit-module-maps -fmodules-cache-path=%t/Cache \
+// RUN:     -fsyntax-only -I %t/implicit-invalidate-common -Rmodule-build \
+// RUN:     %t/B.c 2>> %t/incremental_build.txt
+// RUN: FileCheck %s --implicit-check-not "remark:" --input-file %t/incremental_build.txt
+
+// CHECK: remark: building module 'A'
+// CHECK: remark: building module 'Common'
+// CHECK: remark: finished building module 'Common'
+// CHECK: remark: finished building module 'A'
+// CHECK: remark: building module 'B'
+// CHECK: remark: finished building module 'B'