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
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 🤷
There was a problem hiding this comment.
I understand now. I didn't think about the possibility of using other compilers (e.g. Intel) as we don't claim to support them.
In prestissimo we have a table with the compilers. I will add it to the Velox Readme too.
There was a problem hiding this comment.
I added a table in the readme. Please take a look.
czentgr
force-pushed
the
cz_c++20_support
branch
2 times, most recently
from
15205c4 to
d94a38e
Compare
|
One of the consequences for this PR is that dependents will also need to build with C++20 or make a change to ignore warnings. I can try to build Prestissimo with C++20 as well and see what will happen. not sure if turning off this warning is ok. We should be building Velox with C++20. @assignUser do you have thoughts on this? |
|
@czentgr Well it's not as easy as bumping up cmake. What do we gain from using 20? Are there specific features we want to use (std::chrono comes to mind) or is it more the general DX improvements? What is the Velox userbase? Would they be ok with this? (Velox is to new to have legacy users?) Are there things that would be blocked by requiring C++20 that we might want in the future (e.g. distribution via apt, which we probably don't?) . |
|
@assignUser There is a discussion item on why we want to go to C++20 outlining the reasons: #10692 |
czentgr
force-pushed
the
cz_c++20_support
branch
from
d94a38e to
d4634a4
Compare
|
@czentgr ah I missed that discussion, seems there is a well reasoned consensus with PLC involvement. Gluten seems to be fine with it as well so presto is the other major downstream user at this point. You and @majetideepak are working on that as well right? Checkinkg that it build with C++20 is probably common courtesy ^^ |
There was a problem hiding this comment.
@assignUser That's correct. @majetideepak and me would handle Prestissimo (and possibly might help with Gluten as well if needed). I'm going to see what would stop Prestissimo from upgrading to C++20.
CMakeLists.txt
Outdated
| endif() | ||
|
|
||
| set(KNOWN_WARNINGS | ||
| "-Wno-unused \ | ||
| -Wno-unused-parameter \ | ||
| -Wno-sign-compare \ | ||
| -Wno-ignored-qualifiers \ | ||
| -Wno-c++20-extensions \ |
There was a problem hiding this comment.
@assignUser This warning will prevent Prestissimo from failing. This is useful for now because when consumers use C++17 it will suppress the warnings of the Velox changes. If we build with C++20 in Velox itself this warning will not come out.
I don't think it is a problem because the compilers used in projects consuming Velox should adhere to the Velox minimum compiler version and they understand the syntax.
There was a problem hiding this comment.
Meh this doesn't work for everything because upstream projects pull in headers which can elicit the warning when not build velox libraries. They would need to add this flag on their own or go to C++20.
There was a problem hiding this comment.
Turns out we can turn off the deprecated capture this warning as it is not a generic deprecated messages (as I originally thought) with -Wdeprecated-this-capture. Prestissimo has a single occurrence of this.
There was a problem hiding this comment.
I think handling these warnings should be on the consumer, we would be hiding them without letting them know.
There was a problem hiding this comment.
My thought was that we would remove this eventually when we update Gluten and Prestissimo as the main upstream consumers. But I can remove this. I can deal with the Velox update in Prestissimo once this merged and doesn't cause any trouble for someone.
czentgr
force-pushed
the
cz_c++20_support
branch
from
d4634a4 to
6a531c0
Compare
Fixes as a result: - fix warnings with deprecated usage of volatile variables - replace with std::atomic - replace usage of deprecated std::is_pod_v - turn on warning suppression for 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 If the implicit capture of this is fixed it causes upstream problems by issuing warning c++20-extensions as upstream users are not yet on C++20. Thus, adding to suppress the warning -Wdeprecated-this-capture for now.
czentgr
force-pushed
the
cz_c++20_support
branch
from
6a531c0 to
33f0a8f
Compare
Fixes as a result:
Resolves: #10814