feat(build): Switch to use C++20 standard #10866
Conversation
facebook-github-bot
added
the
CLA Signed
✅ Deploy Preview for meta-velox canceled.
|
czentgr
force-pushed
the
cz_c++20_support
branch
from
ba486bb
to
569a770
Compare
velox/common/base/Semaphore.h
Outdated
| --numWaiting_; | ||
| --count_; | ||
| } | ||
| sem_.acquire(); |
There was a problem hiding this comment.
What's the point to protect semaphore with a lock? This would cause deadlock
There was a problem hiding this comment.
I kept the previous behavior. There is a difference between the counting_semaphore and the mutex in how they work (what they protect - a semaphore can be changed by other threads, with the mutex this is prevented).
And so I used the counting_semaphore to replace the volatile counter (and the way it was triggered).
Because the mutex is used before (with its behavior) I didn't want to change the behavior and allow new race conditions. This can be revisited as a TODO.
There was a problem hiding this comment.
Then you probably should not change it, and add the reason to comment why this cannot be a drop-in replacement for std:: counting_semaphore. The change now will cause deadlock.
There was a problem hiding this comment.
I can undo it and remove the volatile keyword for the members - which is what the complaint of the compiler is.
When I made the change I thought about the behavior change. The fact that a mutex was always taken hasn't changed. I understood that using the counting_semaphore is a bit more permissive in terms of access. The fact that the mutex is taken still allows only a single thread to access it at any given time. But it doesn't change the counting.
There was a problem hiding this comment.
A thread blocked on acquire would hold the lock and block another thread to call release.
condition_variable::wait is releasing the lock in the current code, that's a behavior change.
|
Fyi, happened to see that the following file also contains many hard-coded velox/scripts/setup-helper-functions.sh Line 187 in ede7d0b |
|
Also, Footnotes |
|
Both Clang15 and gcc12 (will test gcc11) look good. No major problems, just a few things here and there. The bigger issue is macOS apple-clang 15. Looks like the SDK is providing a function already that is not ambiguous because the date.h also implements one with one difference which is using the reference operator for the second argument. And it doesn't know which one to use. This affects a few of these. I'm thinking to try and using the SDK functions - Edit: This is a bigger problem. As the CI pipeline shows the macOS 13 build failed because the operator<< is not overloaded for the types - that this function defines it for. |
Yes. This is ok and will actually be removed (there is a PR that does this I believe). This setting is used to compile the dependencies. This doesn't affect the Velox build directly so it doesn't change anything dne here. All that this PR will do is given a set of dependencies build the Velox code (not its dependencies) with C++20. Some of the dependencies likely don't support C++20 just yet. Edit: It is true that this is picked up in the CMakeLists.txt because it is reading it from the file too. But the version is also set using the CMake option and so the |
I have not actually. I'm fixing some errors that are reported as deprecated usages but I haven't seen anything fail because of this particular issue yet. Perhaps something to cleanup later. Edit: Do you have an example for this? I looked and didn't find an instance of this usage. Because it was deprecated in C++17 it should have failed the build then. |
czentgr
force-pushed
the
cz_c++20_support
branch
4 times, most recently
from
e05dd84
to
053e13b
Compare
|
So the compiler bug in gcc 12.2.1 is pretty bad because it affects third-party headers such as gtest: with this code Obviously gtest hasn't changed this code. So the only way to address it is by patching. In this case gtest is built as a bundle so it could be done but if we get it from a header of a pre-installed dependency - well there is no workaround. The only way is to upgrade gcc12 or switch to Clang completely. The other fun part is that the bug only affects release builds. With debug mode the error does not occur. |
czentgr
force-pushed
the
cz_c++20_support
branch
2 times, most recently
from
1277d7e
to
af0b130
Compare
| @@ -63,8 +63,8 @@ class Semaphore { | |||
| private: | |||
| std::mutex mutex_; | |||
| std::condition_variable cv_; | |||
| volatile int32_t count_; | |||
| volatile int32_t numWaiting_{0}; | |||
| std::atomic<int32_t> count_; | |||
There was a problem hiding this comment.
You can just remove the volatile, these are already fully protected by the mutex.
There was a problem hiding this comment.
@majetideepak was concerned that without volatile some code might be optimized away that existed previously. There must have been some reason to use it before. So I changed it to use atomic to prevent any issue as this is the recommended change for situations like this.
There was a problem hiding this comment.
With modern compiler and CPU volatile is not needed. The only case I needed it in my career is in CUDA.
There was a problem hiding this comment.
Thank you. I defer to you on this. I have never used it either that I remember. I assume there was some reason the original author used it. Given we do have mutex protection single threaded access is guaranteed. I'm removing the std::atomic use here.
velox/common/base/Semaphore.h
Outdated
| --numWaiting_; | ||
| --count_; | ||
| } | ||
| sem_.acquire(); |
There was a problem hiding this comment.
A thread blocked on acquire would hold the lock and block another thread to call release.
condition_variable::wait is releasing the lock in the current code, that's a behavior change.
czentgr
force-pushed
the
cz_c++20_support
branch
2 times, most recently
from
91b6685
to
cc39c0f
Compare
|
The last remaining issue is a segv in The stack trace shows the issue occurs in folly when the ThriftServer is starting up. I tried the same with the latest version of folly and fbthrift ( Edit: This PR fixes the issue: #11018 |
czentgr
force-pushed
the
cz_c++20_support
branch
from
cc39c0f
to
b2b0f5b
Compare
I did not remember since when I encountered the build issue related to deprecated |
czentgr
force-pushed
the
cz_c++20_support
branch
from
b2b0f5b
to
7e33c29
Compare
|
Waiting on resolution to #11318. |
czentgr
force-pushed
the
cz_c++20_support
branch
4 times, most recently
from
e8b45ea
to
0573605
Compare
czentgr
changed the title
Fixes as a result: - fix warnings with deprecated usage of volatile variables - replace with std::atomic - replace usage of deprecated std::is_pod_v - fix deprecated usage of implicit capture of ‘this’ via ‘[=]’ - fix ambiguity when calling == operator for LambdaTypedExpr - rearrange some link libraries order to prevent undefined symbols - handle AppleClang versions overloading operator<< for sys_seconds input - handle AppleClang format_to function ambiguity between fmt and system headers - remove std=c++17 from the setup helper script which influences the build - disable folly coroutine headers because the library is not built to support it
czentgr
changed the title
czentgr
force-pushed
the
cz_c++20_support
branch
from
0573605
to
e7164f6
Compare
|
The only failure in the CI pipeline is the performance test for a specific cast function showing a regression. The |
| # For C++20 support we need GNU GCC11 (or later versions) or Clang/AppleClang 15 | ||
| # (or later versions) to get support for the used features. |
There was a problem hiding this comment.
We could also check for specific the specific features we need (see docs) this would make this portable to other compilers. What are we using that's only in gcc 11? Afaik 10 should have full(?) C++20 support?
There was a problem hiding this comment.
gcc11 and clang15 have full C++20 support. gcc10 has only partial C++20 support. And one other reason was to set a minimum of compilers as a base from which we can build and check for features?
There was a problem hiding this comment.
I see there are a few features missing https://en.cppreference.com/w/cpp/compiler_support#cpp20
I am fine with setting a minimum version (should be documented in the readme if it's not) but this specific check will not trigger if someone uses the Intel compiler or something but I guess that would still throw an error once the compiler doesn't understand the C++20 flag so 🤷
Fixes as a result:
Resolves: #10814