Make it easier to disable process executor #7876
Conversation
By adding a DISALLOW_PROCESS_EXECUTOR build option it's possible to conveniently disable the usage of fork() on non-Window platforms that either don't have fork(), or have an incomplete or inefficient implementation that is to be considered a last resort only. In the same commit the DISALLOW_THREAD_EXECUTOR is used for conditionally compiling the thread executor so that both executors are treated in the same way.
|
|
Thanks for your contribution. Why do you need to manually disable this? The idea of not being able to disable the process code was that it is what should always be present since it is more stable (i.e. only the subprocess will exit on a crash/assertion). So being able to disable thread code makes sure it is not accidentally used. Aligning it makes sense though as if we only need/want to use the thread code (e.g. for performance reasons) there is no need to include the process code at all. On a side note - adding process execution for Windows is tracked via https://trac.cppcheck.net/ticket/12464. I have not looked into it yet since I am still working on reworking the execution code so it can be shared which in turn is currently blocked by the suppressions not working properly (i.e. being able to merge #3090 - latest work related to it is #7346). Also some observations outside of the scope of this PR (needs tickets to be filed - and should probably be held off until https://trac.cppcheck.net/ticket/14102 has been resolved):
|
|
Thanks for your quick response! The reason for needing to manually disable the process executor is that I'm building Cppcheck for platforms without a proper fork (just vfork). These OS:es also lack proper memory protection so the stability benefits that you normally get from using a process rather than a thread don't exist. Rather than testing for these OS:es, like it's done with WIN32 today, I opted for the proposed solution, especially since what I'm doing is a very niche thing and would just clutter your code. Since it's a question of disabling, not enabling, I don't think the risk of accidentally using the less robust thread configuration is very high. |
|
Thanks for the insight.
So does this mean it just behaves differently or does it not compile at all? Since the preprocessor checks are still very light I would like to avoid another list of random arch checks (heck, this already got out of sync) as we have in other places and do a proper detection (at least in terms of CMake) to address the latter. |
|
Unfortunately it doesn't build at all and proper detection isn't possible. |
In CMake it can be detected via https://cmake.org/cmake/help/latest/module/CheckFunctionExists.html. But as I said it is not in the scope of this PR. |
Yes, but CMake isn't available either. I'm far out in the dessert :) |
I assumed so. In that case the flag should be enough. And we should probably not add to the preprocessor in source and possibly add it to the Makefile instead. In that case it would be more in line with CMake. |
I filed https://trac.cppcheck.net/ticket/14198 about this. |
2 participants
By adding a DISALLOW_PROCESS_EXECUTOR build option it's possible to conveniently disable the usage of fork() on non-Window platforms that either don't have fork(), or have an incomplete or inefficient implementation that is to be considered a last resort only. In the same commit the DISALLOW_THREAD_EXECUTOR is used for conditionally compiling the thread executor so that both executors are treated in the same way.