Enable PCHs for IR headers

[asl]

Fixes #5032

[asl]

Tofino failures are due to inclusion of two C files into a C++ project / library (without proper language specification in the CMake project):

bf-utils/src/dynamic_hash/dynamic_hash.c
bf-utils/src/dynamic_hash/bfn_hash_algorithm.c

Can these be just ported to C++?

[fruffy]

Tofino failures are due to inclusion of two C files into a C++ project / library (without proper language specification in the CMake project):

bf-utils/src/dynamic_hash/dynamic_hash.c
bf-utils/src/dynamic_hash/bfn_hash_algorithm.c

Can these be just ported to C++?

Yes! Iirc this used to be a separate library which was added to the compiler during open-sourcing.

[asl]

So, there are two issues here:

1. Tofino backend is using C sources in C++ mode
2. Somehow PCH generation conflicts with PIE codegen. This seems to depend on the particular cmake version being used as it fails on Ubuntu 20, but fine everywhere else.

Looks like p4c is compiled always with position independent code. This looks like some workaround to me:

# Always build position-independent code. This is important when linking with Protobuf.
set(CMAKE_POSITION_INDEPENDENT_CODE ON)

Does anyone remember why this was needed? Is it still needed? @fruffy

[fruffy]

Does anyone remember why this was needed? Is it still needed? @fruffy

Iirc if you do not enable this on clang this will cause linking issues with protobuf. I haven't found a better solution than enabling it for all CMake targets.

[asl]

Iirc if you do not enable this on clang this will cause linking issues with protobuf. I haven't found a better solution than enabling it for all CMake targets.

Looks like this is not needed anymore? I removed this from this PR and everything works fine.

[fruffy]

Iirc if you do not enable this on clang this will cause linking issues with protobuf. I haven't found a better solution than enabling it for all CMake targets.

Looks like this is not needed anymore? I removed this from this PR and everything works fine.

Sanitizers is failing. Let's see whether it throws an error after compilation. I would be happy to remove this.

[asl]

Sanitizers is failing. Let's see whether it throws an error after compilation. I would be happy to remove this.

Yes, need to see what is wrong, but likely unrelated.

[asl]

@fruffy clang bug wrt __builtin_bit_cast and PCH: https://github.com/llvm/llvm-project/commit/95d7ccb70b9cbd53f1f137c0b2411852c42c122b

It seems to be fixed in clang 11. The CI is using very old clang 10. Is it possible to upgrade? :)

[fruffy]

@fruffy clang bug wrt __builtin_bit_cast and PCH: llvm/llvm-project@95d7ccb

It seems to be fixed in clang 11. The CI is using very old clang 10. Is it possible to upgrade? :)

Unfortunately Ubuntu 20.04 uses clang-10 by default. As long as we are committed to supporting the system this won't be possible. Unless we raise the minimum version to 11/12 and demand that users install that version.

This is an issue that should be raised separately.

[fruffy]

Alternatively maybe we can simply patch Abseil for this version?

[asl]

Actually, I think we need to discuss if we'd want to enable PCH by default:

• It definitely speeds up clean rebuild
• However, it interferes with ccache or other caches – precompiled header essentially makes everything uncached

So, for small changes (not affecting IR, for example) PCH would yield longer compile times as compared to ccache. For larger changes PCHs seem to be fine.

[asl]

So, I disabled PCHs by default for now. Looks like there are some tradeoffs wrt ccache, so one needs to decide what might be preferable.

Alternatively, we can have PCHs enabled by default if there is no ccache support.

[fruffy]

So, I disabled PCHs by default for now. Looks like there are some tradeoffs wrt ccache, so one needs to decide what might be preferable.

Alternatively, we can have PCHs enabled by default if there is no ccache support.

Thanks for the work! I am actually a little confused why we have this conflict with ccache. Shouldn't the headers stay the same?

It looks like there are some options for the headers: https://ccache.dev/manual/3.2.4.html#_precompiled_headers

[asl]

Thanks for the work! I am actually a little confused why we have this conflict with ccache. Shouldn't the headers stay the same?

Headers are the same, indeed. However, ccache tracks also:

• command line (so to ensure that options did not change)
• the contents of all the files involved (including the PCH)

In general, PCH contents could be different even if the files did not change, and as a result, ccache might decide a full recompile is necessary. There are some options to relax some PCH checks, but these should be done on the client, not on the project side.