Page MenuHomePhabricator

Change llvm call once check for building Swift for PowerPC(ppc64le)
ClosedPublic

Authored by sarveshtamba on Dec 9 2018, 10:24 PM.

Details

Summary

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

Repository
rL LLVM

Event Timeline

sarveshtamba created this revision.Dec 9 2018, 10:24 PM

Please review the attached change as this is a blocker for our development tasks.

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.

Previous review (for the swift-llvm GitHub repo): https://github.com/apple/swift-llvm/pull/129

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)

Yes, the swift toolchain successfully gets built with this change.

sarveshtamba marked an inline comment as done.

Incorporating review comments.

Please review the suggested changes and approve/merge if this looks good. Thanks!

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!

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.

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.

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+++):

PPC64LEPPC64BEPPC32
CLANGcompiler-rtcompiler-rtcompiler-rt
GCCcompiler-rtGNUGNU

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.

compnerd accepted this revision.Dec 12 2018, 10:07 AM

@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?

This revision is now accepted and ready to land.Dec 12 2018, 10:07 AM

@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!

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.

I like the idea for the simplification, I'll do that as I commit it. Thanks for the hints @nemanjai!

compnerd closed this revision.Dec 12 2018, 12:39 PM

SVN r348970

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.

Update for PowerPC64LE

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).

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)?

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)?

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.

@nemanjai, thanks for clarifying! Yes, and separating out PPC64LE from the platform check helps build and execute Swift on PPC64LE. Please suggest.

Hello @nemanjai @compnerd, please let me know how to proceed on this further.

Hello @nemanjai @compnerd, please let me know how to proceed on this further.

I will have to verify that this doesn't break sanitizers on PPC64LE. I'll run with this updated patch and let you know.

@nemanjai Thanks, please do let me know in case you need anything from my side.

Hello @nemanjai Any updates on verification of sanitizers on PPC64LE with this PR?

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:

  1. Post a new revision for this so we're not communicating via a closed review
  2. 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.