From fa7b041eca021cecfd883cc5796150dae5ad4c5f Mon Sep 17 00:00:00 2001 From: Brad King Date: Thu, 28 May 2020 17:29:31 -0400 Subject: 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 --- Source/cmGeneratorTarget.cxx | 2 -- Source/cmGlobalGenerator.cxx | 18 +++++++++++++++++- Tests/RunCMake/PrecompileHeaders/PchReuseFrom.cmake | 7 +++++-- 3 files changed, 22 insertions(+), 5 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. diff --git a/Tests/RunCMake/PrecompileHeaders/PchReuseFrom.cmake b/Tests/RunCMake/PrecompileHeaders/PchReuseFrom.cmake index 03a97ed..f8fba44 100644 --- a/Tests/RunCMake/PrecompileHeaders/PchReuseFrom.cmake +++ b/Tests/RunCMake/PrecompileHeaders/PchReuseFrom.cmake @@ -5,6 +5,11 @@ if(CMAKE_C_COMPILE_OPTIONS_USE_PCH) add_definitions(-DHAVE_PCH_SUPPORT) endif() +# Add this before the target from which we will reuse the PCH +# to test that generators can handle reversed ordering. +add_library(foo foo.c) +target_include_directories(foo PUBLIC include) + add_library(empty empty.c) target_precompile_headers(empty PRIVATE @@ -12,8 +17,6 @@ target_precompile_headers(empty PRIVATE ) target_include_directories(empty PUBLIC include) -add_library(foo foo.c) -target_include_directories(foo PUBLIC include) target_precompile_headers(foo REUSE_FROM empty) # should not cause problems if configured multiple times -- cgit v0.12