PCH: Fix logic error that incorrectly clears sources during VS generation

Since commit 729d997f10 (Precompile Headers: Add REUSE_FROM signature,
2019-08-30, v3.16.0-rc1~101^2), `GetPchFileObject` handles the case that
it is called first for another target's `REUSE_FROM` by calling
`AddSource` to make sure `GetObjectName` can produce the correct object
name.  However, `AddSource` causes `ClearSourcesCache` to be called,
which since commit a9f4f58f0c (cmGeneratorTarget: Clear AllConfigSources
in ClearSourcesCache, 2020-05-15, v3.16.7~2^2) now correctly erases the
`AllConfigSources` structure.  This is okay during `AddPchDependencies`,
but there is another code path in which it is problematic.

When the Visual Studio generator's `WriteAllSources` method is looping
over the sources, the `cmake_pch.cxx` source is encountered first.  This
causes `OutputSourceSpecificFlags` to call `GetPchCreateCompileOptions`,
which calls `GetPchFile`, which under MSVC with `CMAKE_LINK_PCH` calls
`GetPchFileObject`.  That leads to `ClearSourcesCache` erasing the
structure over which `WriteAllSources` is iterating!

This bug is caught by our `RunCMake.PrecompileHeaders` test when run
with the VS generator as of the commit that exposed it by fixing
`ClearSourcesCache`.  However, that change was backported to the CMake
3.16 series after testing only with later versions versions that contain
commit a55df20499 (Multi-Ninja: Add precompile headers support,
2020-01-10, v3.17.0-rc1~136^2).  By adding proper multi-config support
for PCH, that commit taught `cmLocalGenerator::AddPchDependencies` to
call `GetPchFile` with the real set of configurations instead of just
the empty string.  This allows the `GetPchFile` cache of PCH sources to
be populated up front so that the later calls to it in the
`WriteAllSources` loop as described above do not actually call
`GetPchFileObject` or `ClearSourcesCache`.  That hid the problem.

Fix this by re-ordering calls to `AddPchDependencies` to handle
`REUSE_FROM` targets only after the targets whose PCH they re-use.
Remove the now-unnecessary call to `AddSource` from `GetPchFileObject`
so that `ClearSourcesCache` is never called during `WriteAllSources`.
Update the PchReuseFrom test case to cover an ordering of targets that
causes generators to encounter a `REUSE_FROM` target before the target
whose PCH it re-uses.

Fixes: #20770

2 files changed, 17 insertions, 3 deletions

diff --git a/Source/cmGeneratorTarget.cxx b/Source/cmGeneratorTarget.cxx
index cde91c9..4cf076e 100644
--- a/Source/cmGeneratorTarget.cxx
+++ b/Source/cmGeneratorTarget.cxx
@@ -3496,8 +3496,6 @@ std::string cmGeneratorTarget::GetPchFileObject(const std::string& config,
     }
     std::string& filename = inserted.first->second;
 
-    this->AddSource(pchSource, true);
-
     auto pchSf = this->Makefile->GetOrCreateSource(
       pchSource, false, cmSourceFileLocationKind::Known);
 
diff --git a/Source/cmGlobalGenerator.cxx b/Source/cmGlobalGenerator.cxx
index 09fb87d..74bcf7b 100644
--- a/Source/cmGlobalGenerator.cxx
+++ b/Source/cmGlobalGenerator.cxx
@@ -1564,7 +1564,23 @@ bool cmGlobalGenerator::AddAutomaticSources()
         continue;
       }
       lg->AddUnityBuild(gt);
-      lg->AddPchDependencies(gt);
+      // Targets that re-use a PCH are handled below.
+      if (!gt->GetProperty("PRECOMPILE_HEADERS_REUSE_FROM")) {
+        lg->AddPchDependencies(gt);
+      }
+    }
+  }
+  for (cmLocalGenerator* lg : this->LocalGenerators) {
+    for (cmGeneratorTarget* gt : lg->GetGeneratorTargets()) {
+      if (gt->GetType() == cmStateEnums::INTERFACE_LIBRARY ||
+          gt->GetType() == cmStateEnums::UTILITY ||
+          gt->GetType() == cmStateEnums::GLOBAL_TARGET) {
+        continue;
+      }
+      // Handle targets that re-use a PCH from an above-handled target.
+      if (gt->GetProperty("PRECOMPILE_HEADERS_REUSE_FROM")) {
+        lg->AddPchDependencies(gt);
+      }
     }
   }
   // The above transformations may have changed the classification of sources.