These changes are required to resolve the call_once related errors seen while building the Swift toolchain on PowerPC64LE. The existing macro checks are inadequate while building Swift on PowerPC64LE and results in use of std::call_once, instead of llvm's implementation.
Diff Detail
Diff Detail
- Repository
- rL LLVM
Event Timeline
Comment Actions
Earlier diff created was using the command "git show HEAD -U999999 > mypatch.patch" as mentioned in "https://llvm.org/docs/Phabricator.html#phabricator-request-review-web".
The updated patch is created using git diff and reflects the correct diff of the changes.
Comment Actions
Previous review (for the swift-llvm GitHub repo): https://github.com/apple/swift-llvm/pull/129
Comment Actions
It is best to always upload all patches with full context (-U99999).
Do the tests pass on that ppc64le?
| include/llvm/Support/Threading.h | ||
|---|---|---|
| 30 | clang-format please (linewrap 80 col) | |
Comment Actions
Hi Reviewers,
Can you please review this? This change is resolving a blocker while building swift toolchain on PowerPC64LE and is a bit urgent. Since this is a small change, can you please take this up? Thanks!
Comment Actions
It is okay to fallback to the hand-rolled CAS based once initializer. It would be nice to have a clearer explanation for why the C++ runtime on the target cannot support std::call_once which is a C++11 standard function.
Comment Actions
As per the comments in the code:-
std::call_once from libc++ is used on all Unix platforms. Other
implementations like libstdc++ are known to have problems on NetBSD,
// OpenBSD and PowerPC.
There were some crashes seen when std::call_once was used on PowerPC64LE. There is a macro check already for PPC (i.e "!(defined(NetBSD) || defined(OpenBSD) || defined(ppc)))" ). However this check is not enough(since ppc could conceivably be 32-bit PPC as well). Hence this patch checks for PPC as well as LITTLE_ENDIAN which is the case for PowerPC64LE and further works fine without any issues on this platform.
Comment Actions
This code has been in place for quite a long time as there is some sort of bug in libstdc++. I don't remember what the problem was but I remember that this was required because of an issue with the GNU C++ runtime.
My concern is that the intent with defining LLVM_THREADING_USE_STD_CALL_ONCE to zero when __ppc__ is defined is that we would use the non GNU one on all PPC platforms.
However, gcc does not define __ppc__.
So the behaviour before this change is that anything built with clang on any PPC system will use the clang version of std::call_once and anything built with gcc will use the GNU version. This was the case on all little endian and big endian systems (both 32 and 64 bit). After this change, this is what the situation will be (compiler-rt == clang's libc++, GNU == GNU libstdc+++):
| PPC64LE | PPC64BE | PPC32 | |
| CLANG | compiler-rt | compiler-rt | compiler-rt |
| GCC | compiler-rt | GNU | GNU |
Of course, that may be what we want, but I don't know for sure. Perhaps @hfinkel, @echristo or @seurer can shed some further light on this.
Comment Actions
@nemanjai - I'm not following you with regards to the libc++ being used. This is simply used to replace the use of std::call_once with a local implementation, it doesn't introduce a dependency on compiler-rt at all and it doesn't change the C++ runtime that you are depending on.
That said, with the additional context, this seems reasonable. @sarveshtamba, do you need someone to commit this on your behalf?
Comment Actions
@compnerd, thanks for the explanation and accepting this change revision. Yes, I would need someone to commit this on my behalf, as I might not have the rights to do a commit. Please go ahead and do it for me if you can. Thanks in advance!
Comment Actions
Yeah, as we discussed over IRC, all I was suggesting is that there is a difference when we choose one over the other with this patch. But I don't think we really care about endianness here, so we might want to just change this to defined(__ppc__) || defined(__PPC__) so we always define LLVM_THREADING_USE_STD_CALL_ONCE the same way on all PPC platforms.
Comment Actions
I like the idea for the simplification, I'll do that as I commit it. Thanks for the hints @nemanjai!
Comment Actions
For your interest NetBSD no longer needs it (at least for amd64) as we have fixed the issues.. but we will keep it for a while (a year or so) in order to keep users of oldstable release to be able to use newer LLVM.
Comment Actions
Hi, The changes committed as a part of commit r348970 are different from the original patch which I submitted and which also fixed build issues while building Swift 5.0 toolchain (lldb component). The new patch committed still breaks the swift toolchain build as before:-
[32/914] Building CXX object source/Commands/CMakeFiles/lldbCommands.dir/CommandObjectPlatform.cpp.o
FAILED: source/Commands/CMakeFiles/lldbCommands.dir/CommandObjectPlatform.cpp.o
/usr/bin/clang++ -DGTEST_HAS_RTTI=0 -DHAVE_ROUND -DLIBXML2_DEFINED -DLLDB_CONFIGURATION_RELEASE -D_DEBUG -DSTDC_CONSTANT_MACROS -DSTDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Isource/Commands -I/root/swift-source/lldb/source/Commands -Iinclude -I/root/swift-source/lldb/include -I/root/swift-source/build/buildbot_linux/llvm-linux-powerpc64le/include -I/root/swift-source/build/buildbot_linux/llvm-linux-powerpc64le/tools/clang/include -I/root/swift-source/llvm/include -I/include -I/root/swift-source/llvm/tools/clang/include -I/root/swift-source/build/buildbot_linux/swift-linux-powerpc64le/include -I/root/swift-source/swift/include -I/root/swift-source/lldb/source -I/usr/include/python3.5m -I/root/swift-source/lldb/tools/clang/include -I../clang/include -I/usr/include/libxml2 -I/root/swift-source/lldb/source/. -fPIC -fvisibility-inlines-hidden -Werror=date-time -std=c++11 -w -fdiagnostics-color -ffunction-sections -fdata-sections -Wno-deprecated-declarations -Wno-unknown-pragmas -Wno-strict-aliasing -Wno-deprecated-register -Wno-vla-extension -O3 -UNDEBUG -fno-exceptions -fno-rtti -MMD -MT source/Commands/CMakeFiles/lldbCommands.dir/CommandObjectPlatform.cpp.o -MF source/Commands/CMakeFiles/lldbCommands.dir/CommandObjectPlatform.cpp.o.d -o source/Commands/CMakeFiles/lldbCommands.dir/CommandObjectPlatform.cpp.o -c /root/swift-source/lldb/source/Commands/CommandObjectPlatform.cpp
/root/swift-source/lldb/source/Commands/CommandObjectPlatform.cpp:1248:7: error: no matching function for call to 'call_once'
llvm::call_once(g_once_flag, []() {
^~~~~~~~~~~~~~~/root/swift-source/llvm/include/llvm/Support/Threading.h:100:8: note: candidate function [with Function = (lambda at /root/swift-source/lldb/source/Commands/CommandObjectPlatform.cpp:1248:36), Args = <>] not viable: no known conversion from 'std::once_flag' to 'llvm::once_flag &' for 1st argument
void call_once(once_flag &flag, Function &&F, Args &&... ArgList) {
^1 error generated.
[33/914] Building CXX object source/Commands/CMakeFiles/lldbCommands.dir/CommandObjectLog.cpp.o
[34/914] Building CXX object source/Expression/CMakeFiles/lldbExpression.dir/REPL.cpp.o
[35/914] Building CXX object source/Commands/CMakeFiles/lldbCommands.dir/CommandObjectMemory.cpp.o
[36/914] Building CXX object source/Commands/CMakeFiles/lldbCommands.dir/CommandObjectPlugin.cpp.o
[37/914] Building CXX object source/Commands/CMakeFiles/lldbCommands.dir/CommandObjectQuit.cpp.o
[38/914] Building CXX object source/Expression/CMakeFiles/lldbExpression.dir/UserExpression.cpp.o
[39/914] Building CXX object source/Commands/CMakeFiles/lldbCommands.dir/CommandObjectProcess.cpp.o
ninja: build stopped: subcommand failed.
./utils/build-script: fatal error: command terminated with a non-zero exit status 1, aborting
./utils/build-script: fatal error: command terminated with a non-zero exit status 1, aborting
I have revised the patch once again to include ppc and PPC besides my original changes for PowerPC64LE & tested the same for PowerPC64LE.
Please review.
PS:- I know it can get a bit confusing, but just to clarify, "(defined(PPC) && defined(LITTLE_ENDIAN))", was not an additional OR condition within the original !(platform checks) i.e. not within "!(defined(NetBSD) || defined(OpenBSD) || defined(ppc))", but was a separate OR condition outside, i.e. !(platform checks) || (PPC && LE).
Comment Actions
The Swift 5.0 toolchain on PowerPC64LE also works fine if we remove the "defined(ppc)" check altogether in the original code i.e keep only "!(defined(NetBSD) || defined(OpenBSD)))".
Would that affect other PPC platforms (32/64 and BE/LE PPC other than PowerPC64LE)?
Comment Actions
Oh I see. What you are suggesting is that on PPC64LE we *want to use std::call_once*. Basically, get rid of the workaround for PPC64LE. That seems in direct contrast to what this code was originally meant to do then doesn't it? In any case, it would require fairly extensive testing of affected code before we could commit such a change.
Comment Actions
@nemanjai, thanks for clarifying! Yes, and separating out PPC64LE from the platform check helps build and execute Swift on PPC64LE. Please suggest.
Comment Actions
I will have to verify that this doesn't break sanitizers on PPC64LE. I'll run with this updated patch and let you know.
Comment Actions
Sarvesh, I've tried this on a PPC64LE machine with one version of GLIBC and found no problems. However, I am reluctant to just go ahead with a change without testing with a few other versions of GLIBC.
Would you be able to:
- Post a new revision for this so we're not communicating via a closed review
- Disentangle the conditions into perhaps multiple #if/#elif` blocks as this is very difficult to parse
Then we can do another round of review and testing before we finalize this.
clang-format please (linewrap 80 col)