Conversation
They options are still, for the moment at least, defined in the relative compiler description files, but they aren't used during the build anymore.
|
I experimented a bit with including our own headers also into the PCH and the improvement was at best minor and often regressed vs just using standard library headers. I also ran into some situations where a PCH had an old version of something from Botan itself and the resulting error messages were very hard to interpret, so I'm inclined to avoid unless it really helps. |
Seems like older Clang doesn't like this? Clang 21 is fine. Unfortunate, probably suggests there needs to be 2 PCHs one for the library and the other for tests/cli |
This comment was marked as outdated.
This comment was marked as outdated.
This comment was marked as off-topic.
This comment was marked as off-topic.
This comment was marked as outdated.
This comment was marked as outdated.
|
Clang 21: exe 12 Mb, lib 17 Mb |
|
@copilot Try adding PCH support to the ninja build |
|
@randombit I've opened a new pull request, #5299, to work on those changes. Once the pull request is ready, I'll request review from you. |
|
Weird error right in the middle of a build Not sure what to make of this Possibly an interaction with ccache. The ccache docs say re PCH: If you use Clang, you must compile with -fno-pch-timestamp. |
Co-authored-by: randombit <469092+randombit@users.noreply.github.com>
Add Ninja build support for precompiled headers
|
@copilot OK now try to extend to support Microsoft Visual C++ PCH IIUC you'll have to modify configure to write some little stub cpp files for MSVC to compile to create the PCH IIUC the relevant options are |
|
@randombit I've opened a new pull request, #5301, to work on those changes. Once the pull request is ready, I'll request review from you. |
1a51c2c to
b86bb92
Compare
|
Needs a squash but should otherwise be good to go |
There was a problem hiding this comment.
Pull request overview
Adds build-system support for precompiled headers (PCH) to reduce overall compile times, along with a few related header/include cleanups for SIMD and CLMUL code paths.
Changes:
- Introduce a shared PCH header (
src/lib/pch/pch.h) and wire it into Makefile and Ninja build generation (including compiler-specific PCH flags). - Extend configuration and cleanup tooling to create/use/remove PCH build artifacts and adjust compiler/build flags to better support PCH sharing.
- Refine SIMD-related includes/ISA annotations and fix missing intrinsic header inclusion needed for CLMUL usage.
Reviewed changes
Copilot reviewed 16 out of 16 changed files in this pull request and generated 4 comments.
Show a summary per file
| File | Description |
|---|---|
| src/scripts/cleanup.py | Cleans PCH artifacts in the build directory |
| src/lib/utils/simd/simd_4x64/simd_4x64.h | Adds ISA attribute macro to additional members for consistency |
| src/lib/utils/simd/simd_4x32/simd_4x32.h | Switches SSSE3 include to a narrower intrinsic header |
| src/lib/utils/simd/simd_2x64/simd_2x64.h | Adjusts SSSE3 includes and adds ISA attributes to more functions |
| src/lib/utils/ghash/ghash_cpu/polyval_fn.h | Adds wmmintrin.h include for CLMUL intrinsic declarations |
| src/lib/pch/pch.h | New PCH “umbrella” header of common STL headers |
| src/lib/misc/zfec/zfec_sse2/zfec_sse2.cpp | Narrows intrinsic include to SSE2-only header |
| src/configs/repo_config.env | Sets CCACHE_SLOPPINESS to support PCH + ccache together |
| src/build-data/ninja.in | Adds PCH build rules and injects PCH usage into compile rules |
| src/build-data/makefile.in | Adds PCH targets and makes objects depend on generated PCHs |
| src/build-data/compile_commands.json.in | Removes ISA flags from compilation database entries |
| src/build-data/cc/{clang,gcc,xcode}.txt | Defines per-compiler PCH compile/include flags and suffixes |
| doc/dev_ref/contributing.rst | Documents recommended ccache sloppiness settings for PCH |
| configure.py | Adds --disable-pch, generates PCH variables, and adjusts flags to support PCH |
💡 Add Copilot custom instructions for smarter, more guided reviews. Learn how to get started.
| for f in os.listdir(build_config['pch_dir']): | ||
| for suffix in pch_suffixes: | ||
| if f.endswith(suffix): | ||
| remove_file(os.path.join(build_config['pch_dir'], f)) |
There was a problem hiding this comment.
os.listdir(build_config['pch_dir']) is unguarded. If the directory was removed manually (or was never created due to an interrupted configure), cleanup.py will raise and make clean/ninja clean will fail. Consider checking os.access(pch_dir, os.X_OK) like other cleanup paths, or catching OSError around os.listdir and treating ENOENT as already-clean.
| for f in os.listdir(build_config['pch_dir']): | |
| for suffix in pch_suffixes: | |
| if f.endswith(suffix): | |
| remove_file(os.path.join(build_config['pch_dir'], f)) | |
| pch_dir = build_config['pch_dir'] | |
| if os.access(pch_dir, os.X_OK): | |
| try: | |
| for f in os.listdir(pch_dir): | |
| for suffix in pch_suffixes: | |
| if f.endswith(suffix): | |
| remove_file(os.path.join(pch_dir, f)) | |
| except OSError as e: | |
| if e.errno != errno.ENOENT: | |
| logging.error('Failed accessing PCH directory "%s": %s', pch_dir, e) | |
| else: | |
| logging.debug('PCH directory %s was missing', pch_dir) |
|
|
||
| link_method = choose_link_method(options) | ||
|
|
||
| if options.enable_pch: |
There was a problem hiding this comment.
PCH header copying is guarded by options.enable_pch, but the rest of the build templates are guarded by template_vars['enable_pch'] (which also checks cc.supports_pch()). On unsupported compilers this will still create ${build}/obj/pch and copy pch_*.h, but no build rules will consume them and cleanup.py won't remove them (since pch_dir isn't in build_config). Consider keying this on template_vars['enable_pch'] to keep the filesystem outputs consistent with the generated build rules.
| if options.enable_pch: | |
| if template_vars.get('enable_pch'): |
| %{for lib_build_info} | ||
| build %{obj}: compile_lib %{src} | ||
| isa_flags = %{isa_flags} | ||
| build %{obj}: compile_lib %{src} | %{pch_target} | ||
| %{endfor} | ||
|
|
||
| %{for cli_build_info} | ||
| build %{obj}: compile_exe %{src} | ||
| build %{obj}: compile_exe %{src} | %{pch_target} | ||
| %{endfor} | ||
|
|
||
| %{for test_build_info} | ||
| build %{obj}: compile_exe %{src} | ||
| build %{obj}: compile_exe %{src} | %{pch_target} | ||
| %{endfor} |
There was a problem hiding this comment.
When precompiled headers are disabled/unsupported, pch_target expands to an empty string, leaving a dangling | in lines like build %{obj}: ... | %{pch_target}. This produces an invalid build.ninja and will break e.g. MSVC/clangcl builds where enable_pch becomes false. Emit the order-only dependency only when enable_pch is true (or include the leading | in the variable so it can be empty safely).
| deps = %{ninja_header_deps_style} | ||
| %{endif} | ||
| command = %{cxx} ${ABI_FLAGS} ${LANG_FLAGS} ${CXXFLAGS} -DBOTAN_IS_BEING_BUILT ${WARN_FLAGS} ${isa_flags} %{public_include_flags} %{internal_include_flags} %{external_include_flags} %{header_deps_flag} %{header_deps_out|concat: $out.d} %{dash_c} $in %{dash_o}$out | ||
| command = %{cxx} %{pch_include_for_exe} ${ABI_FLAGS} ${LANG_FLAGS} ${CXXFLAGS} -DBOTAN_IS_BEING_BUILT ${WARN_FLAGS} %{public_include_flags} %{internal_include_flags} %{external_include_flags} %{header_deps_flag} %{header_deps_out|concat: $out.d} %{dash_c} $in %{dash_o}$out |
There was a problem hiding this comment.
compile_exe now injects %{pch_include_for_exe}. However some Ninja build edges using compile_exe (e.g. fuzzers, bogo_shim_object, ct_selftest_object) don't have an order-only dependency on the PCH build. In parallel builds this can lead to nondeterministic PCH usage and, in the worst case, attempting to read a partially-written PCH file. Add the same | pch order-only dependency for all compile_exe invocations when enable_pch is enabled (or ensure these targets don't use the PCH include flag).
| command = %{cxx} %{pch_include_for_exe} ${ABI_FLAGS} ${LANG_FLAGS} ${CXXFLAGS} -DBOTAN_IS_BEING_BUILT ${WARN_FLAGS} %{public_include_flags} %{internal_include_flags} %{external_include_flags} %{header_deps_flag} %{header_deps_out|concat: $out.d} %{dash_c} $in %{dash_o}$out | |
| command = %{cxx} ${ABI_FLAGS} ${LANG_FLAGS} ${CXXFLAGS} -DBOTAN_IS_BEING_BUILT ${WARN_FLAGS} %{public_include_flags} %{internal_include_flags} %{external_include_flags} %{header_deps_flag} %{header_deps_out|concat: $out.d} %{dash_c} $in %{dash_o}$out |
#5164
Stacked on #5297
Tried at MSVC support but it's a quite different model and I got tired of going back and forth with CI. Left to an interested party, hopefully one with a local Windows machine for faster iterations.
On my laptop, building with Clang 21:
3973.09s user 70.02s system 770% cpu 8:44.74 totalmasterplus Split part of concepts.h into range_concepts.h #5294 + Various changes to reduce header dependencies in TLS #5296:3238.27s user 95.83s system 770% cpu 7:12.99 total2070.82s user 58.87s system 772% cpu 4:35.66 total