Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

PCH not detected using Clang and CMake 3.16 #539

Closed
cristianadam opened this issue Feb 12, 2020 · 18 comments · Fixed by #624
Closed

PCH not detected using Clang and CMake 3.16 #539

cristianadam opened this issue Feb 12, 2020 · 18 comments · Fixed by #624
Labels
bug Does not work as intended/documented new compiler behavior New compiler behavior not handled correctly
Milestone

Comments

@cristianadam
Copy link
Contributor

How to reproduce

CMake 3.16 has gotten support for PCHs. I've build a "Hello World" application and build it with MinGW-GCC 8.1 (gcc) and LLVM-MinGW 9.0 (clang).

CMakeLists.txt

cmake_minimum_required(VERSION 3.16)

project(main)

add_executable(main main.cpp)
target_precompile_headers(main PRIVATE <iostream>)

install(TARGETS main)

enable_testing()
add_test(NAME main COMMAND main)

main.cpp

#include <iostream>

int main()
{
  std::cout << "Hello world\n";
}

test.cmake

set(ENV{CCACHE_DEBUG} true)
set(ENV{CCACHE_LOGFILE} ${CMAKE_CURRENT_LIST_DIR}/ccache.log)
set(ENV{CCACHE_DIR} ${CMAKE_CURRENT_LIST_DIR}/.ccache)
set(ENV{CCACHE_SLOPPINESS} pch_defines,time_macros)

foreach(step 1 2)
    execute_process(COMMAND cmake -E remove_directory build)

    execute_process(COMMAND cmake
    -G Ninja
    -D CMAKE_BUILD_TYPE=Release
    -D CMAKE_CXX_COMPILER_LAUNCHER=ccache
    -S .
    -B build)

    execute_process(COMMAND ccache -z)

    execute_process(COMMAND cmake --build build)
endforeach()

execute_process(COMMAND ccache -s) 

I've build the application running cmake -P test.cmake.

Actual behavior

For gcc at the end I've got in the statistics:

cache hit (direct)                     0
cache hit (preprocessed)               1
cache miss                             1
cache hit rate                     50.00 %
cleanups performed                     0
files in cache                         2

In the ccache.log file I could see entries like:

Detected use of precompiled header:

For clang I've got:

cache hit (direct)                     2
cache hit (preprocessed)               0
cache miss                             0
cache hit rate                    100.00 %
cleanups performed                     0
files in cache                         4

In the ccache.log I couldn't see any pch log entries.

Expected behavior

I expected ccache to detect and handle pch file for Clang.

Environment

ccache version 3.7.7

CMake PCH options

For Clang CMake uses the following format for pch file creation and usage:

  set(CMAKE_${lang}_COMPILE_OPTIONS_USE_PCH -Xclang -include-pch -Xclang <PCH_FILE>)
  set(CMAKE_${lang}_COMPILE_OPTIONS_CREATE_PCH -Xclang -emit-pch -Xclang -include -Xclang <PCH_HEADER>)
@cristianadam cristianadam added the bug Does not work as intended/documented label Feb 12, 2020
@cristianadam
Copy link
Contributor Author

I tried to overcome the -Xclang arguments with this patch:

--- src/ccache.cpp
+++ src/ccache.cpp
@@ -2521,6 +2521,13 @@ cc_process_args(ArgsInfo& args_info,
       continue;
     }
 
+    // Allow parameters to compiler passed via -Xclang to be analyzed by
+    // ccache's logic like -Xclang -emit-pch
+    if (str_eq(argv[i], "-Xclang") && i + 2 < argc) {
+      args_add(common_args, argv[i]);
+      ++i;
+    }
+
     if (str_eq(argv[i], "-fpch-preprocess") || str_eq(argv[i], "-emit-pch")
         || str_eq(argv[i], "-emit-pth")) {
       found_fpch_preprocess = true;
@@ -2971,11 +2978,17 @@ cc_process_args(ArgsInfo& args_info,
         return false;
       }
 
-      if (!detect_pch(argv[i], argv[i + 1], &found_pch)) {
+      // Skip the -Xclang -include -Xclang path/to/file cases
+      int next = 1;
+      if (str_eq(argv[i + 1], "-Xclang") && i + 3 < argc) {
+        next = 2;
+      }
+
+      if (!detect_pch(argv[i], argv[i + next], &found_pch)) {
         return false;
       }
 
-      std::string relpath = make_relative_path(argv[i + 1]);
+      std::string relpath = make_relative_path(argv[i + next]);
       if (compopt_affects_cpp(argv[i])) {
         args_add(cpp_args, argv[i]);
         args_add(cpp_args, relpath.c_str());

Unfortunately, as seen in ccache.log I've hit Multiple input files error:

[.349169 524  ] Command line: ccache C:\llvm-mingw\bin\c++.exe -O3 -DNDEBUG -Xclang -emit-pch -Xclang -include -Xclang C:/Projects/github/ccache-test/build/CMakeFiles/main.dir/cmake_pch.hxx -MD -MT CMakeFiles/main.dir/cmake_pch.hxx.pch -MF CMakeFiles\main.dir\cmake_pch.hxx.pch.d -o CMakeFiles/main.dir/cmake_pch.hxx.pch -c CMakeFiles/main.dir/cmake_pch.hxx.cxx
[.364794 524  ] Hostname: unknown
[.364794 524  ] Working directory: C:\Projects\github\ccache-test\build
[.364794 524  ] Multiple input files: C:/Projects/github/ccache-test/build/CMakeFiles/main.dir/cmake_pch.hxx and CMakeFiles/main.dir/cmake_pch.hxx.cxx
[.139761 8792 ] Command line: ccache C:\llvm-mingw\bin\c++.exe -O3 -DNDEBUG -Xclang -include-pch -Xclang C:/Projects/github/ccache-test/build/CMakeFiles/main.dir/cmake_pch.hxx.pch -MD -MT CMakeFiles/main.dir/main.cpp.obj -MF CMakeFiles\main.dir\main.cpp.obj.d -o CMakeFiles/main.dir/main.cpp.obj -c ../main.cpp
[.139761 8792 ] Hostname: unknown
[.139761 8792 ] Working directory: C:\Projects\github\ccache-test\build
[.139761 8792 ] Detected use of precompiled header: C:/Projects/github/ccache-test/build/CMakeFiles/main.dir/cmake_pch.hxx.pch
[.139761 8792 ] Multiple input files: C:/Projects/github/ccache-test/build/CMakeFiles/main.dir/cmake_pch.hxx.pch and ../main.cpp

In the first case when the pch file is created, the header cmake_pch.hxx is force included, and cmake_pch.hxx.cxx is empty because CMake needs a source file in order to compile the pch file.

In the second case, cmake_pch.hxx.pch is used via include-pch.

I'll have to dig further in oder to make ccache happy. This bug affects all Clang compilers (default on macOS) when used with CMake.

@jrosdahl
Copy link
Member

jrosdahl commented Feb 14, 2020

Thanks for the bug report. This would be good to have fixed.

For my own understanding: Do you happen to know why CMake uses -Xclang? Wouldn't it work just as well to just use -include-pch <PCH_FILE>, etc.?

@cristianadam
Copy link
Contributor Author

Clang doesn't seem to have emit-pch as a front end parameter in the compiler, just as a backend one.

clang++ -emit-pch -include iostream -c iostream.cpp -o iostream.pch

Produces a 396 bytes file with Clang9 on Windows, whilst:

clang++ -Xclang -emit-pch -Xclang -include -Xclang iostream -c iostream.cpp -o iostream.pch

Produces a 7.138.760 bytes file, and it works no matter how many times you run the command.

The -Xclang -include is needed for subsequent runs. Without it the second run of:

clang -Xclang -emit-pch -include iostream -c iostream.cpp -o iostream.pch

will generate the second time a 13.228 bytes file, and the 3rd time will issue an error:

fatal error: PCH file 'C:\Temp\iostream.pch' is out of date and needs to be rebuilt: module file out of date
note: imported by 'iostream.pch'

This is due to Clang doing chained pchs.

ccache would have to deal with -Xclang parameters for -include or -include-pch.

@annulen
Copy link

annulen commented Mar 11, 2020

Clang doesn't seem to have emit-pch as a front end parameter in the compiler, just as a backend one.

clang++ -emit-pch -include iostream -c iostream.cpp -o iostream.pch

Why not just use normal way of pch generation, namely

clang++ -x c++-header iostream -o iostream.pch

?

@cristianadam
Copy link
Contributor Author

Why not just use normal way of pch generation, namely

clang++ -x c++-header iostream -o iostream.pch

?

Clang has a bug with the GCC mode, see https://gitlab.kitware.com/cmake/cmake/issues/19786

@annulen
Copy link

annulen commented Mar 11, 2020

Thanks. Was that bug reported to bugs.llvm.org?

@cristianadam
Copy link
Contributor Author

I don't think we reported to llvm, since we found a workaround quickly, which works with all existing Clang versions.

@annulen
Copy link

annulen commented Mar 11, 2020

IMHO it would be better to get Clang bug fixed instead of introducing unnecessary complexity in every project around it. More so as that bug affect only one platform.

@annulen
Copy link

annulen commented Mar 17, 2020

Seems like you've used the same -Xclang -include-pch way on all platforms, so ccache+PCH is now broken for clang everywhere just because of Windows-specific bug.

@cristianadam
Copy link
Contributor Author

This is not a Windows specific bug. Clang has the same behavior on all platforms.

https://gitlab.kitware.com/cmake/cmake/issues/19786 was mostly tested with AppleClang on macOS.

@annulen
Copy link

annulen commented Mar 17, 2020

Sorry, I didn't read issue carefully.
Seems like it could be easier to work around on cmake side by adding rm to rule which compiles pch

@xkszltl
Copy link

xkszltl commented Jun 2, 2020

I recently hit an error of reusing PCH with different arg in Halide. Wondering if it's related to this issue.
halide/Halide#4985

@ahasselbring
Copy link
Contributor

ahasselbring commented Jul 2, 2020

Hi,
I am part of a project that is affected by this bug and have tried to investigate it. I think the shortcomings in @cristianadam's approach are:

  • not setting output_is_precompiled_header which needs special treatment because the input file suffix when building the PCH is .cxx and the language is not overridden explicitly (could be done by either checking for -emit-pch or checking the output file suffix for .pch)
  • in the end of the compopt_takes_path branch, i should have been incremented by next instead of 1. Not doing this leads to the "multiple input files" error because this means that in the next iteration, the path to the PCH is the argument and that is recognized as input file because it is not an option

PS: I don't know what the distinction in common_args and cpp_args is for, but could it be bad that in the -Xclang -include(-pch) -Xclang <path> the result would be that -Xclang is added to common_args and -include(-pch) and <path> are added to cpp_args?

@ahasselbring
Copy link
Contributor

ahasselbring commented Jul 2, 2020

Another problem will then be that with CMake 3.17.3, the command line looks like -Xclang -include-pch -Xclang <PCH_FILE> -Xclang -include -Xclang <PCH_HEADER> (in order to work around some other issue). Since detect_pch recognizes both, it will trigger the error "Multiple precompiled headers used". But since they are in fact the same file, this should be resolvable.

jrosdahl pushed a commit that referenced this issue Jul 25, 2020
Co-authored-by: Cristian Adam <cristian.adam@gmail.com>
jrosdahl pushed a commit that referenced this issue Jul 25, 2020
Co-authored-by: Cristian Adam <cristian.adam@gmail.com>
@cristianadam
Copy link
Contributor Author

@ahasselbring thank you for the work on this issue!

I took the latest master, build it with MinGW 8.1 (cmake -GNinja -DCMAKE_BUILD_TYPE=Release -DZSTD_FROM_INTERNET=ON -DCMAKE_C_FLAGS_RELEASE_INIT=-fno-asynchronous-unwind-tables ) and gave the setup that I presented in this bug report a test.

I used CMake 3.18.0.

And to my surprise...:

cache hit (direct)                     0
cache hit (preprocessed)               0
cache miss                             2
cache hit rate                      0.00 %
cleanups performed                     0
files in cache                         5

I expected 100% hit rate.

For sloppiness I tried set(ENV{CCACHE_SLOPPINESS} pch_defines,time_macros) and also set(ENV{CCACHE_SLOPPINESS} pch_defines,time_macros,include_file_mtime) (learned from #549).

ccache-master-clang9.log and ccache-master-clang9-include_mtime.log.

The weird part is that the same happend with MinGW GCC 8.1. I suspect there is a problem with how precompiled headers are handled in master.

If I disable the precompiled headers in CMake, I get 100% hit rate for both compilers.

@ahasselbring
Copy link
Contributor

ahasselbring commented Jul 26, 2020

I think I also encountered this when I wrote the tests. I first wrote the PCH generation test exactly like CMake does it, i.e. compiling an empty file and passing the header pch.h via -include, noticed that it wouldn't hit and found the line
Include file pch.h too new
or as in your logfiles
Include file CMakeFiles/main.dir/cmake_pch.hxx.pch too new
Without deeper knowledge, I assumed that this is because in the automated test scenario the timestamp of the included header file and the cached result are the same and verified this by inserting a sleep 1, which then resulted in a cache hit in the test. However, I didn't want to make the testsuite unnecessarily slower, so I changed the test from the exact CMake command line to directly passing pch.h as input file (in which case the timestamp doesn't seem to be a problem).

I just did the experiment again and the message in the log is actually
Precompiled header includes pch.h, which has a new mtime
so it might be unrelated. However, I could swear that I also saw Include file pch.h too new at some point.

Sorry, I hadn't read #549 when writing this. This post is probably pointless.

@jrosdahl
Copy link
Member

For sloppiness I tried set(ENV{CCACHE_SLOPPINESS} pch_defines,time_macros) and also set(ENV{CCACHE_SLOPPINESS} pch_defines,time_macros,include_file_mtime) (learned from #549).

Add include_file_ctime as well? If that does not work, would you mind creating a new bug report?

@ahasselbring
Copy link
Contributor

By the way, I just saw in the original bug report that with gcc there is also only a preprocessed hit and not a direct hit. You can get preprocessed PCH hits with clang now if you add -Xclang -fno-pch-timestamp to the PCH generation command line. Then the stats should at least be the same for gcc and clang.

@jrosdahl jrosdahl added the new compiler behavior New compiler behavior not handled correctly label Jan 12, 2021
@jrosdahl jrosdahl added this to the 4.0 milestone Jan 12, 2021
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
bug Does not work as intended/documented new compiler behavior New compiler behavior not handled correctly
Projects
None yet
Development

Successfully merging a pull request may close this issue.

5 participants