ARROW-3972: [C++] Migrate to LLVM 7. Add option to disable using ld.gold #3499

wesm · 2019-01-27T16:39:10Z

All in all this wasn't too painful, and Gandiva seems to run fine (I wouldn't have expected otherwise).

I tested this locally on Ubuntu 18.10, and found that the LLVM libraries from apt fail to link with ld.gold (binutils 1.16), so I added an option to toggle ld.gold ON and OFF (it's now OFF by default; using ld by default should yield strictly fewer bugs / build failures). Not sure why that is

I will need some help testing and debugging the Crossbow and packaging tasks to make sure this doesn't break anything else before we merge it

wesm · 2019-01-27T16:39:52Z

@kszucs can you help with running / checking the crossbow tasks?

@xhochy what's involved in upgrading manylinux1 beyond changing the build_llvm.sh script?

wesm · 2019-01-27T16:41:08Z

@pravindra I wasn't sure whether this would be controversial but we need to move to LLVM 7 to be able to package Gandiva with the Python wheels in the next major release (0.13.0)

xhochy · 2019-01-27T16:42:23Z

@xhochy what's involved in upgrading manylinux1 beyond changing the build_llvm.sh script?

Updating the scripts/build_clang.sh script. Otherwise that should be it.

xhochy · 2019-01-27T16:44:00Z

@wesm ld.gold problems are normally good indicators of real linking problems. What did you face?

wesm · 2019-01-27T21:38:56Z

I'll try building again later and post the errors here

pravindra · 2019-01-28T10:10:51Z

I'm pleasantly surprised that there are no gandiva issues :-).

Once the CI build stabilises, I will try your change with the crossbow task used by dremio. Can you please hold off on merging this change till then ?

wesm · 2019-01-28T14:33:58Z

Yes, of course

pitrou · 2019-01-29T12:15:52Z

ci/travis_install_toolchain.sh

@@ -25,7 +25,7 @@ if [ ! -e $CPP_TOOLCHAIN ]; then
    CONDA_PACKAGES=""

    if [ $ARROW_TRAVIS_GANDIVA == "1" ] && [ $TRAVIS_OS_NAME == "osx" ]; then
-        CONDA_PACKAGES="$CONDA_PACKAGES llvmdev=6.0.1"
+        CONDA_PACKAGES="$CONDA_PACKAGES llvmdev=$CONDA_LLVM_VERSION"


Can use the syntax llvmdev="7.0.*" instead of hardcoding the bugfix level.

wesm · 2019-01-30T03:56:41Z

Here's what the ld.gold errors look like:

https://gist.github.com/wesm/c11c4ebd90be74f9e536f86d11f540c5

This was on Ubuntu Cosmic / 18.10

I'm going to try this on Ubuntu 14.04 also to see if the issue is somehow related to LLVM or whether it is particular to Ubuntu 18.10

wesm · 2019-01-30T05:19:31Z

The build worked on Ubuntu 14.04 using both ld and gold. Not sure where to go for here, this is not my expertise

I think the prudent thing is to disable ld.gold by default -- if there is a build configuration where it works well and reduces build times, then we can definitely turn it on

wesm · 2019-02-01T14:28:06Z

@xhochy do you want to take care of the LLVM 7 upgrade in this patch or in a different one?

Anything else you all want to take care of in this patch?

xhochy · 2019-02-01T14:44:16Z

@xhochy do you want to take care of the LLVM 7 upgrade in this patch or in a different one?

Anything else you all want to take care of in this patch?

You mean in the manylinux1 container? Or in total? I would only do the former.

pravindra · 2019-02-04T10:53:21Z

@wesm I tried the crossbow builds used by dremio with your patch. I found two issues

conda requires full version (llvmdev-7.0), whereas apt-get requires only major number (clang-7)
on mac, the script couldn't find clang-7.
- I couldn't find clang-7 on conda. so, for now, i just modified the script to switch to default clang if clang-7 is not found.

My changes are here : pravindra@3da660b

xhochy · 2019-02-04T13:38:54Z

@pravindra The clang package called clangdev on conda.

wesm · 2019-02-04T21:21:28Z

thanks @pravindra! I rebased and cherry picked your changes. I'll merge this once the build is once the build is passing

wesm · 2019-02-05T05:27:08Z

@pravindra @kou @xhochy I need your assistance to complete this work

I tried upgrading LLVM to 7 in manylinux1 build but

FAILED: lib/Basic/CMakeFiles/clangBasic.dir/Targets/AMDGPU.cpp.o 
/opt/rh/devtoolset-2/root/usr/bin/c++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Basic -I../lib/Basic -I../include -Iinclude -I//include -fPIC -fvisibility-inlines-hidden -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -Wdelete-non-virtual-dtor -Wno-comment -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -pedantic -Wno-long-long -O3    -UNDEBUG  -fno-exceptions -fno-rtti -MD -MT lib/Basic/CMakeFiles/clangBasic.dir/Targets/AMDGPU.cpp.o -MF lib/Basic/CMakeFiles/clangBasic.dir/Targets/AMDGPU.cpp.o.d -o lib/Basic/CMakeFiles/clangBasic.dir/Targets/AMDGPU.cpp.o -c ../lib/Basic/Targets/AMDGPU.cpp
../lib/Basic/Targets/AMDGPU.cpp: In constructor ‘clang::targets::AMDGPUTargetInfo::AMDGPUTargetInfo(const llvm::Triple&, const clang::TargetOptions&)’:
../lib/Basic/Targets/AMDGPU.cpp:333:49: error: ‘OpenCL’ is not a member of ‘llvm::Triple’
                      Triple.getEnvironment() == llvm::Triple::OpenCL ||
                                                 ^
...
ninja: build stopped: subcommand failed.
The command '/bin/sh -c /build_clang.sh' returned a non-zero code: 1

Some initial Googling didn't produce an obvious answer

I also have a build failure in normal Xenial with

: && /home/travis/build/apache/arrow/cpp-toolchain/bin/ccache /usr/lib/ccache/g++  -ggdb -O0  -Wall -Wconversion -Wno-sign-conversion -Werror -msse4.2  -B/home/travis/build/apache/arrow/cpp-toolchain/bin -g  -rdynamic src/gandiva/CMakeFiles/gandiva-decimal_type_util_test.dir/decimal_type_util_test.cc.o  -o debug/gandiva-decimal_type_util_test  -Wl,-rpath,/home/travis/build/apache/arrow/cpp-build/debug:/home/travis/build/apache/arrow/cpp-toolchain/lib debug/libgandiva.so.13.0.0 /home/travis/build/apache/arrow/cpp-toolchain/lib/libre2.a debug/libarrow_testing.so.13.0.0 debug/libarrow.so.13.0.0 -ldl /home/travis/build/apache/arrow/cpp-toolchain/lib/libdouble-conversion.a /home/travis/build/apache/arrow/cpp-toolchain/lib/libboost_system.so /home/travis/build/apache/arrow/cpp-toolchain/lib/libboost_filesystem.so /home/travis/build/apache/arrow/cpp-toolchain/lib/libboost_regex.so /home/travis/build/apache/arrow/cpp-toolchain/lib/libgtest.a /home/travis/build/apache/arrow/cpp-toolchain/lib/libgmock_main.a /home/travis/build/apache/arrow/cpp-toolchain/lib/libgmock.a -ldl jemalloc_ep-prefix/src/jemalloc_ep/dist//lib/libjemalloc_pic.a -pthread -lrt -Wl,-rpath-link,/home/travis/build/apache/arrow/cpp-toolchain/lib && :
/home/travis/build/apache/arrow/cpp-toolchain/bin/ld: debug/libgandiva.so.13.0.0: undefined reference to `typeinfo for llvm::ErrorInfoBase'

I added the missing link library libLLVMSupport.a and it didn't help. I'm going to try the "all" option

pravindra · 2019-02-05T05:42:20Z

@wesm, I can work on fixing the xenial build.

wesm · 2019-02-05T05:47:13Z

Linking to all the static libraries didn't make that error go away...

wesm · 2019-02-05T05:47:19Z

Thanks

xhochy · 2019-02-05T15:30:15Z

@pravindra @kou @xhochy I need your assistance to complete this work

I tried upgrading LLVM to 7 in manylinux1 build but

FAILED: lib/Basic/CMakeFiles/clangBasic.dir/Targets/AMDGPU.cpp.o 
/opt/rh/devtoolset-2/root/usr/bin/c++  -DGTEST_HAS_RTTI=0 -D_DEBUG -D_GNU_SOURCE -D__STDC_CONSTANT_MACROS -D__STDC_FORMAT_MACROS -D__STDC_LIMIT_MACROS -Ilib/Basic -I../lib/Basic -I../include -Iinclude -I//include -fPIC -fvisibility-inlines-hidden -std=c++11 -Wall -Wextra -Wno-unused-parameter -Wwrite-strings -Wcast-qual -Wno-missing-field-initializers -Wdelete-non-virtual-dtor -Wno-comment -ffunction-sections -fdata-sections -fno-common -Woverloaded-virtual -fno-strict-aliasing -pedantic -Wno-long-long -O3    -UNDEBUG  -fno-exceptions -fno-rtti -MD -MT lib/Basic/CMakeFiles/clangBasic.dir/Targets/AMDGPU.cpp.o -MF lib/Basic/CMakeFiles/clangBasic.dir/Targets/AMDGPU.cpp.o.d -o lib/Basic/CMakeFiles/clangBasic.dir/Targets/AMDGPU.cpp.o -c ../lib/Basic/Targets/AMDGPU.cpp
../lib/Basic/Targets/AMDGPU.cpp: In constructor ‘clang::targets::AMDGPUTargetInfo::AMDGPUTargetInfo(const llvm::Triple&, const clang::TargetOptions&)’:
../lib/Basic/Targets/AMDGPU.cpp:333:49: error: ‘OpenCL’ is not a member of ‘llvm::Triple’
                      Triple.getEnvironment() == llvm::Triple::OpenCL ||
                                                 ^
...
ninja: build stopped: subcommand failed.
The command '/bin/sh -c /build_clang.sh' returned a non-zero code: 1

Some initial Googling didn't produce an obvious answer

I also have a build failure in normal Xenial with

: && /home/travis/build/apache/arrow/cpp-toolchain/bin/ccache /usr/lib/ccache/g++  -ggdb -O0  -Wall -Wconversion -Wno-sign-conversion -Werror -msse4.2  -B/home/travis/build/apache/arrow/cpp-toolchain/bin -g  -rdynamic src/gandiva/CMakeFiles/gandiva-decimal_type_util_test.dir/decimal_type_util_test.cc.o  -o debug/gandiva-decimal_type_util_test  -Wl,-rpath,/home/travis/build/apache/arrow/cpp-build/debug:/home/travis/build/apache/arrow/cpp-toolchain/lib debug/libgandiva.so.13.0.0 /home/travis/build/apache/arrow/cpp-toolchain/lib/libre2.a debug/libarrow_testing.so.13.0.0 debug/libarrow.so.13.0.0 -ldl /home/travis/build/apache/arrow/cpp-toolchain/lib/libdouble-conversion.a /home/travis/build/apache/arrow/cpp-toolchain/lib/libboost_system.so /home/travis/build/apache/arrow/cpp-toolchain/lib/libboost_filesystem.so /home/travis/build/apache/arrow/cpp-toolchain/lib/libboost_regex.so /home/travis/build/apache/arrow/cpp-toolchain/lib/libgtest.a /home/travis/build/apache/arrow/cpp-toolchain/lib/libgmock_main.a /home/travis/build/apache/arrow/cpp-toolchain/lib/libgmock.a -ldl jemalloc_ep-prefix/src/jemalloc_ep/dist//lib/libjemalloc_pic.a -pthread -lrt -Wl,-rpath-link,/home/travis/build/apache/arrow/cpp-toolchain/lib && :
/home/travis/build/apache/arrow/cpp-toolchain/bin/ld: debug/libgandiva.so.13.0.0: undefined reference to `typeinfo for llvm::ErrorInfoBase'

I added the missing link library libLLVMSupport.a and it didn't help. I'm going to try the "all" option

The error here is not in LLVM but in clang (separate build step). My PR to your repo fixes that.

pravindra · 2019-02-06T11:48:20Z

@wesm - the xenial error was happening because it was picking up clang/llvm from travis (pre-installed) rather than the one from apt-get. Using the one from apt-get fixed the issue. I also reverted the change to link all llvm libs (i.e we are back to linking only the required llvm libs).

wesm#7

wesm · 2019-02-06T14:36:11Z

That's great, thank you.

Change-Id: Ieaf5302599a83f35ceaf3c4c8ffbe63c03619d98

Change-Id: If6b93a6ef467a9afb62fe91783b0bff8a1e78438

Change-Id: I73985c52be2837496590443f7c990466712ec037

Change-Id: I7e6d154d21b673e6c070bed8c046ca4dab0559f2

Change-Id: I6d13457a689406ed9a98840463a157bc2be7430d

Change-Id: I07da2e2e663150cbc9c4fa51a48e4e0aa4bd93cc

Change-Id: I54d32ad72a87713ce82e17769a589dde00965102

wesm · 2019-02-06T16:38:26Z

Travis CI looking OK now. I just rebased to make sure we get a clean build. I'll then merge this and notify the mailing list about upgrading clang toolchains

This is good also because Visual Studio 2017 requires at least clang 7 -- I hit that when doing Windows testing

wesm · 2019-02-06T21:52:38Z

Appveyor build https://ci.appveyor.com/project/wesm/arrow/builds/22177695

All in all this wasn't too painful, and Gandiva seems to run fine (I wouldn't have expected otherwise). I tested this locally on Ubuntu 18.10, and found that the LLVM libraries from apt fail to link with ld.gold (binutils 1.16), so I added an option to toggle ld.gold ON and OFF (it's now OFF by default; using `ld` by default should yield strictly fewer bugs / build failures). Not sure why that is I will need some help testing and debugging the Crossbow and packaging tasks to make sure this doesn't break anything else before we merge it Author: Wes McKinney <[email protected]> Author: Pindikura Ravindra <[email protected]> Author: Korn, Uwe <[email protected]> Author: Uwe L. Korn <[email protected]> Closes #3499 from wesm/llvm-7 and squashes the following commits: 5c48eed <Korn, Uwe> Switch to docker image on branch 93aadd7 <Uwe L. Korn> Update clang e155fad <Pindikura Ravindra> Use the llvm from apt-get (instead of travis) 987fc5f <Wes McKinney> Enable all LLVM libraries ea1f013 <Wes McKinney> Link libLLVMSupport.a later 034b17d <Wes McKinney> Add missing libLLVMSupport.a dependency 4448243 <Wes McKinney> Install clang before calling travis_install_linux.sh 59d170a <Wes McKinney> Fix more usages of LLVM version ce831e2 <Wes McKinney> Fix clang executable name in .travis.yml 69fd486 <Pindikura Ravindra> ARROW-3972: misc fixes for LLVM7 c858441 <Wes McKinney> Some CI fixes 87434da <Wes McKinney> Code comments d198eb8 <Wes McKinney> Decruft. Turn off ld.gold by default 2fc3a26 <Wes McKinney> Build project with LLVM 7. Add option to disable using ld.gold

pitrou reviewed Jan 29, 2019

View reviewed changes

wesm force-pushed the llvm-7 branch from 8afcdf2 to 71e4e9b Compare January 30, 2019 03:57

wesm force-pushed the llvm-7 branch from 93d417b to 9f6b11e Compare February 1, 2019 14:27

wesm force-pushed the llvm-7 branch from 9f6b11e to 1988f37 Compare February 4, 2019 21:19

wesm changed the title ~~WIP ARROW-3972: [C++] Migrate to LLVM 7. Add option to disable using ld.gold~~ ARROW-3972: [C++] Migrate to LLVM 7. Add option to disable using ld.gold Feb 4, 2019

wesm mentioned this pull request Feb 5, 2019

Update clang wesm/arrow#6

Closed

wesm added 3 commits February 6, 2019 10:36

Build project with LLVM 7. Add option to disable using ld.gold

2fc3a26

Decruft. Turn off ld.gold by default

d198eb8

Code comments

87434da

wesm and others added 8 commits February 6, 2019 10:36

Some CI fixes

c858441

Change-Id: Ieaf5302599a83f35ceaf3c4c8ffbe63c03619d98

ARROW-3972: misc fixes for LLVM7

69fd486

Fix clang executable name in .travis.yml

ce831e2

Change-Id: If6b93a6ef467a9afb62fe91783b0bff8a1e78438

Fix more usages of LLVM version

59d170a

Change-Id: I73985c52be2837496590443f7c990466712ec037

Install clang before calling travis_install_linux.sh

4448243

Change-Id: I7e6d154d21b673e6c070bed8c046ca4dab0559f2

Add missing libLLVMSupport.a dependency

034b17d

Change-Id: I6d13457a689406ed9a98840463a157bc2be7430d

Link libLLVMSupport.a later

ea1f013

Change-Id: I07da2e2e663150cbc9c4fa51a48e4e0aa4bd93cc

Enable all LLVM libraries

987fc5f

Change-Id: I54d32ad72a87713ce82e17769a589dde00965102

wesm force-pushed the llvm-7 branch from 202bed3 to 987fc5f Compare February 6, 2019 16:37

Pindikura Ravindra and others added 3 commits February 6, 2019 11:32

Use the llvm from apt-get (instead of travis)

e155fad

Update clang

93aadd7

Switch to docker image on branch

5c48eed

wesm closed this in c81fbaa Feb 6, 2019

wesm deleted the llvm-7 branch February 6, 2019 21:53

This was referenced Feb 22, 2019

[C++] Update to LLVM and Clang bits to 7.0 #20575

Closed

[Packaging] Update linux packaging tasks to align with the LLVM 7 migration #21077

Closed

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

ARROW-3972: [C++] Migrate to LLVM 7. Add option to disable using ld.gold #3499

ARROW-3972: [C++] Migrate to LLVM 7. Add option to disable using ld.gold #3499

wesm commented Jan 27, 2019

wesm commented Jan 27, 2019

wesm commented Jan 27, 2019

xhochy commented Jan 27, 2019

xhochy commented Jan 27, 2019

wesm commented Jan 27, 2019

pravindra commented Jan 28, 2019

wesm commented Jan 28, 2019

pitrou Jan 29, 2019

wesm Jan 30, 2019

wesm commented Jan 30, 2019

wesm commented Jan 30, 2019

wesm commented Feb 1, 2019

xhochy commented Feb 1, 2019

pravindra commented Feb 4, 2019

xhochy commented Feb 4, 2019

wesm commented Feb 4, 2019

wesm commented Feb 5, 2019

pravindra commented Feb 5, 2019

wesm commented Feb 5, 2019

wesm commented Feb 5, 2019

xhochy commented Feb 5, 2019

pravindra commented Feb 6, 2019 •

edited

Loading

wesm commented Feb 6, 2019

wesm commented Feb 6, 2019

wesm commented Feb 6, 2019

ARROW-3972: [C++] Migrate to LLVM 7. Add option to disable using ld.gold #3499

ARROW-3972: [C++] Migrate to LLVM 7. Add option to disable using ld.gold #3499

Conversation

wesm commented Jan 27, 2019

wesm commented Jan 27, 2019

wesm commented Jan 27, 2019

xhochy commented Jan 27, 2019

xhochy commented Jan 27, 2019

wesm commented Jan 27, 2019

pravindra commented Jan 28, 2019

wesm commented Jan 28, 2019

pitrou Jan 29, 2019

Choose a reason for hiding this comment

wesm Jan 30, 2019

Choose a reason for hiding this comment

wesm commented Jan 30, 2019

wesm commented Jan 30, 2019

wesm commented Feb 1, 2019

xhochy commented Feb 1, 2019

pravindra commented Feb 4, 2019

xhochy commented Feb 4, 2019

wesm commented Feb 4, 2019

wesm commented Feb 5, 2019

pravindra commented Feb 5, 2019

wesm commented Feb 5, 2019

wesm commented Feb 5, 2019

xhochy commented Feb 5, 2019

pravindra commented Feb 6, 2019 • edited Loading

wesm commented Feb 6, 2019

wesm commented Feb 6, 2019

wesm commented Feb 6, 2019

pravindra commented Feb 6, 2019 •

edited

Loading