-
Notifications
You must be signed in to change notification settings - Fork 446
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
Enable PCHs for IR headers #5033
base: main
Are you sure you want to change the base?
Conversation
Tofino failures are due to inclusion of two C files into a C++ project / library (without proper language specification in the CMake
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. |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Great, thank you!
So, there are two issues here:
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 |
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. |
Yes, need to see what is wrong, but likely unrelated. |
@fruffy clang bug wrt 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. |
Alternatively maybe we can simply patch Abseil for this version? |
Actually, I think we need to discuss if we'd want to enable PCH by default:
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. |
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. |
@@ -7,10 +7,6 @@ | |||
#include <string.h> | |||
#endif | |||
|
|||
#ifdef __cplusplus |
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Can we split these changes into a separate PR?
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 |
Headers are the same, indeed. However, ccache tracks also:
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. |
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
…IE always enable (do we really need it though?) Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Signed-off-by: Anton Korobeynikov <anton@korobeynikov.info>
Fixes #5032