Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

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

Closed
wants to merge 14 commits into from
26 changes: 13 additions & 13 deletions .travis.yml
Original file line number Diff line number Diff line change
Expand Up @@ -58,7 +58,7 @@ matrix:
- $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh
script:
- $TRAVIS_BUILD_DIR/ci/travis_lint.sh
- name: "C++ unit tests w/ Valgrind, clang 6.0"
- name: "C++ unit tests w/ Valgrind, clang 7.0"
language: cpp
os: linux
env:
Expand All @@ -70,12 +70,12 @@ matrix:
- ARROW_TRAVIS_GANDIVA=1
- ARROW_TRAVIS_USE_SYSTEM_JAVA=1
- ARROW_BUILD_WARNING_LEVEL=CHECKIN
- CC="clang-7"
- CXX="clang++-7"
before_script:
- if [ $ARROW_CI_CPP_AFFECTED != "1" ]; then exit; fi
- export CC="clang-6.0"
- export CXX="clang++-6.0"
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
# If either C++ or Python changed, we must install the C++ libraries
- git submodule update --init
- $TRAVIS_BUILD_DIR/ci/travis_before_script_cpp.sh
Expand All @@ -99,8 +99,8 @@ matrix:
- ARROW_BUILD_WARNING_LEVEL=CHECKIN
before_script:
- if [ $ARROW_CI_CPP_AFFECTED != "1" ] && [ $ARROW_CI_JAVA_AFFECTED != "1" ]; then exit; fi
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
# If either C++ or Python changed, we must install the C++ libraries
- git submodule update --init
- $TRAVIS_BUILD_DIR/ci/travis_before_script_cpp.sh
Expand Down Expand Up @@ -133,8 +133,8 @@ matrix:
- eval `python $TRAVIS_BUILD_DIR/ci/detect-changes.py`
before_script:
- if [ $ARROW_CI_CPP_AFFECTED != "1" ] && [ $ARROW_CI_JAVA_AFFECTED != "1" ]; then exit; fi
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
# If either C++ or Python changed, we must install the C++ libraries
- git submodule update --init
- $TRAVIS_BUILD_DIR/ci/travis_before_script_cpp.sh
Expand All @@ -160,8 +160,8 @@ matrix:
# - ARROW_TRAVIS_PYTHON_BENCHMARKS=1
before_script:
- if [ $ARROW_CI_PYTHON_AFFECTED != "1" ] && [ $ARROW_CI_DOCS_AFFECTED != "1" ]; then exit; fi
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_toolchain.sh
script:
- $TRAVIS_BUILD_DIR/ci/travis_script_java.sh || travis_terminate 1
Expand Down Expand Up @@ -225,7 +225,7 @@ matrix:
- name: "[manylinux1] Python"
language: cpp
before_script:
- if [ $ARROW_CI_PYTHON_AFFECTED == "1" ]; then docker pull quay.io/xhochy/arrow_manylinux1_x86_64_base:latest; fi
- if [ $ARROW_CI_PYTHON_AFFECTED == "1" ]; then docker pull quay.io/xhochy/arrow_manylinux1_x86_64_base:llvm-7-manylinux1; fi
script:
- if [ $ARROW_CI_PYTHON_AFFECTED == "1" ]; then $TRAVIS_BUILD_DIR/ci/travis_script_manylinux.sh; fi
- name: "Java w/ OpenJDK 8"
Expand Down Expand Up @@ -254,20 +254,20 @@ matrix:
- if [ $ARROW_CI_JAVA_AFFECTED != "1" ]; then exit; fi
script:
- $TRAVIS_BUILD_DIR/ci/travis_script_java.sh
- name: "Integration w/ OpenJDK 8"
- name: "Integration w/ OpenJDK 8, clang 7"
language: java
os: linux
env: ARROW_TEST_GROUP=integration
jdk: openjdk8
env:
- ARROW_TRAVIS_PLASMA=1
- ARROW_TRAVIS_PLASMA_JAVA_CLIENT=1
- CC="clang-7"
- CXX="clang++-7"
before_script:
- if [ $ARROW_CI_INTEGRATION_AFFECTED != "1" ]; then exit; fi
- export CC="clang-6.0"
- export CXX="clang++-6.0"
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
- nvm install 11.6
- $TRAVIS_BUILD_DIR/ci/travis_before_script_js.sh
- $TRAVIS_BUILD_DIR/ci/travis_before_script_cpp.sh
Expand Down Expand Up @@ -297,8 +297,8 @@ matrix:
- ARROW_TRAVIS_PLASMA=1
before_script:
- if [ $ARROW_CI_RUBY_AFFECTED != "1" ]; then exit; fi
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_clang_tools.sh
- $TRAVIS_BUILD_DIR/ci/travis_install_linux.sh
- $TRAVIS_BUILD_DIR/ci/travis_before_script_cpp.sh --only-library
- $TRAVIS_BUILD_DIR/ci/travis_before_script_c_glib.sh
- $TRAVIS_BUILD_DIR/ci/travis_before_script_ruby.sh
Expand Down
2 changes: 1 addition & 1 deletion c_glib/Brewfile
Original file line number Diff line number Diff line change
Expand Up @@ -24,7 +24,7 @@ brew "git"
brew "gobject-introspection"
brew "gtk-doc"
brew "libtool"
brew "llvm@6"
brew "llvm@7"
brew "lua"
brew "luarocks"
brew "ninja"
Expand Down
6 changes: 6 additions & 0 deletions ci/travis_env_common.sh
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,12 @@ export MINICONDA=$HOME/miniconda
export CONDA_PKGS_DIRS=$HOME/.conda_packages
export CONDA_BINUTILS_VERSION=2.31

export ARROW_LLVM_VERSION=7.0
export CONDA_LLVM_VERSION="7.0.*"

# extract the major version
export ARROW_LLVM_MAJOR_VERSION=$(echo $ARROW_LLVM_VERSION | cut -d. -f1)

export ARROW_CPP_DIR=$TRAVIS_BUILD_DIR/cpp
export ARROW_PYTHON_DIR=$TRAVIS_BUILD_DIR/python
export ARROW_C_GLIB_DIR=$TRAVIS_BUILD_DIR/c_glib
Expand Down
4 changes: 2 additions & 2 deletions ci/travis_install_clang_tools.sh
Original file line number Diff line number Diff line change
Expand Up @@ -24,6 +24,6 @@ source $TRAVIS_BUILD_DIR/ci/travis_env_common.sh

wget -O - https://apt.llvm.org/llvm-snapshot.gpg.key|sudo apt-key add -
sudo apt-add-repository -y \
"deb https://apt.llvm.org/$DISTRO_CODENAME/ llvm-toolchain-$DISTRO_CODENAME-6.0 main"
"deb https://apt.llvm.org/$DISTRO_CODENAME/ llvm-toolchain-$DISTRO_CODENAME-$ARROW_LLVM_MAJOR_VERSION main"
sudo apt-get update -qq
sudo apt-get install -q clang-6.0 clang-format-6.0 clang-tidy-6.0
sudo apt-get install -q clang-$ARROW_LLVM_MAJOR_VERSION clang-format-$ARROW_LLVM_MAJOR_VERSION clang-tidy-$ARROW_LLVM_MAJOR_VERSION
10 changes: 5 additions & 5 deletions ci/travis_install_linux.sh
Original file line number Diff line number Diff line change
Expand Up @@ -38,11 +38,12 @@ if [ "$ARROW_TRAVIS_COVERAGE" == "1" ]; then
fi

set -x
if [ "$DISTRO_CODENAME" != "trusty" ]; then
if [ "$ARROW_TRAVIS_GANDIVA" == "1" ]; then
sudo apt-get install -y -qq llvm-6.0-dev
fi
if [ "$ARROW_TRAVIS_GANDIVA" == "1" ]; then
sudo apt-get install -y -qq llvm-$ARROW_LLVM_MAJOR_VERSION-dev
fi

set -x
if [ "$DISTRO_CODENAME" != "trusty" ]; then
sudo apt-get install -y -qq maven

# Remove Travis-specific versions of Java
Expand All @@ -56,4 +57,3 @@ if [ "$DISTRO_CODENAME" != "trusty" ]; then
java -version
mvn -v
fi

2 changes: 1 addition & 1 deletion ci/travis_install_toolchain.sh
Original file line number Diff line number Diff line change
Expand Up @@ -26,7 +26,7 @@ if [ ! -e $CPP_TOOLCHAIN ]; then
CONDA_LABEL=""

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"
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

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

Copy link
Member Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

done

fi

if [ $TRAVIS_OS_NAME == "linux" ]; then
Expand Down
2 changes: 1 addition & 1 deletion ci/travis_script_manylinux.sh
Original file line number Diff line number Diff line change
Expand Up @@ -21,7 +21,7 @@
set -ex

pushd python/manylinux1
docker run --shm-size=2g --rm -e PYARROW_PARALLEL=3 -v $PWD:/io -v $PWD/../../:/arrow quay.io/xhochy/arrow_manylinux1_x86_64_base:latest /io/build_arrow.sh
docker run --shm-size=2g --rm -e PYARROW_PARALLEL=3 -v $PWD:/io -v $PWD/../../:/arrow quay.io/xhochy/arrow_manylinux1_x86_64_base:llvm-7-manylinux1 /io/build_arrow.sh

# Testing for https://issues.apache.org/jira/browse/ARROW-2657
# These tests cannot be run inside of the docker container, since TensorFlow
Expand Down
8 changes: 7 additions & 1 deletion cpp/CMakeLists.txt
Original file line number Diff line number Diff line change
Expand Up @@ -74,7 +74,9 @@ endif()

set(BUILD_SUPPORT_DIR "${CMAKE_SOURCE_DIR}/build-support")

set(CLANG_FORMAT_VERSION "6.0")
set(ARROW_LLVM_VERSION "7.0")
STRING(REGEX REPLACE "^([0-9]+)\\.[0-9]+" "\\1" ARROW_LLVM_MAJOR_VERSION "${ARROW_LLVM_VERSION}")
STRING(REGEX REPLACE "^[0-9]+\\.([0-9]+)" "\\1" ARROW_LLVM_MINOR_VERSION "${ARROW_LLVM_VERSION}")
find_package(ClangTools)
if ("$ENV{CMAKE_EXPORT_COMPILE_COMMANDS}" STREQUAL "1" OR CLANG_TIDY_FOUND)
# Generate a Clang compile_commands.json "compilation database" file for use
Expand Down Expand Up @@ -123,6 +125,10 @@ if("${CMAKE_SOURCE_DIR}" STREQUAL "${CMAKE_CURRENT_SOURCE_DIR}")
"Use ccache when compiling (if available)"
ON)

option(ARROW_USE_LD_GOLD
"Use ld.gold for linking on Linux (if available)"
OFF)

option(ARROW_USE_TSAN
"Enable Thread Sanitizer checks"
OFF)
Expand Down
6 changes: 3 additions & 3 deletions cpp/README.md
Original file line number Diff line number Diff line change
Expand Up @@ -292,15 +292,15 @@ In addition to the arrow dependencies, gandiva requires :
On Ubuntu/Debian you can install these requirements with:

```shell
sudo apt-add-repository -y "deb http://llvm.org/apt/trusty/ llvm-toolchain-trusty-6.0 main"
sudo apt-add-repository -y "deb http://llvm.org/apt/trusty/ llvm-toolchain-trusty-7.0 main"
sudo apt-get update -qq
sudo apt-get install llvm-6.0-dev
sudo apt-get install llvm-7.0-dev
```

On macOS, you can use [Homebrew][1]:

```shell
brew install llvm@6
brew install llvm@7
```

The optional `gandiva` libraries and tests can be built by passing
Expand Down
25 changes: 10 additions & 15 deletions cpp/cmake_modules/FindClangTools.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -57,12 +57,8 @@ set(CLANG_TOOLS_SEARCH_PATHS
"${HOMEBREW_PREFIX}/bin")

find_program(CLANG_TIDY_BIN
NAMES clang-tidy-4.0
clang-tidy-3.9
clang-tidy-3.8
clang-tidy-3.7
clang-tidy-3.6
clang-tidy
NAMES clang-tidy-${ARROW_LLVM_VERSION}
clang-tidy-${ARROW_LLVM_MAJOR_VERSION}
PATHS ${CLANG_TOOLS_SEARCH_PATHS} NO_DEFAULT_PATH
)

Expand All @@ -74,35 +70,34 @@ else()
message(STATUS "clang-tidy found at ${CLANG_TIDY_BIN}")
endif()

if (CLANG_FORMAT_VERSION)
if (ARROW_LLVM_VERSION)
find_program(CLANG_FORMAT_BIN
NAMES clang-format-${CLANG_FORMAT_VERSION}
NAMES clang-format-${ARROW_LLVM_VERSION}
clang-format-${ARROW_LLVM_MAJOR_VERSION}
PATHS ${CLANG_TOOLS_SEARCH_PATHS} NO_DEFAULT_PATH
)

# If not found yet, search alternative locations
if ("${CLANG_FORMAT_BIN}" STREQUAL "CLANG_FORMAT_BIN-NOTFOUND")
STRING(REGEX REPLACE "^([0-9]+)\\.[0-9]+" "\\1" CLANG_FORMAT_MAJOR_VERSION "${CLANG_FORMAT_VERSION}")
STRING(REGEX REPLACE "^[0-9]+\\.([0-9]+)" "\\1" CLANG_FORMAT_MINOR_VERSION "${CLANG_FORMAT_VERSION}")
if (APPLE)
# Homebrew ships older LLVM versions in /usr/local/opt/llvm@version/
if ("${CLANG_FORMAT_MINOR_VERSION}" STREQUAL "0")
if ("${ARROW_LLVM_MINOR_VERSION}" STREQUAL "0")
find_program(CLANG_FORMAT_BIN
NAMES clang-format
PATHS "${HOMEBREW_PREFIX}/opt/llvm@${CLANG_FORMAT_MAJOR_VERSION}/bin"
PATHS "${HOMEBREW_PREFIX}/opt/llvm@${ARROW_LLVM_MAJOR_VERSION}/bin"
NO_DEFAULT_PATH
)
else()
find_program(CLANG_FORMAT_BIN
NAMES clang-format
PATHS "${HOMEBREW_PREFIX}/opt/llvm@${CLANG_FORMAT_VERSION}/bin"
PATHS "${HOMEBREW_PREFIX}/opt/llvm@${ARROW_LLVM_VERSION}/bin"
NO_DEFAULT_PATH
)
endif()

if ("${CLANG_FORMAT_BIN}" STREQUAL "CLANG_FORMAT_BIN-NOTFOUND")
# binary was still not found, look into Cellar
file(GLOB CLANG_FORMAT_PATH "${HOMEBREW_PREFIX}/Cellar/llvm/${CLANG_FORMAT_VERSION}.*")
file(GLOB CLANG_FORMAT_PATH "${HOMEBREW_PREFIX}/Cellar/llvm/${ARROW_LLVM_VERSION}.*")
find_program(CLANG_FORMAT_BIN
NAMES clang-format
PATHS "${CLANG_FORMAT_PATH}/bin"
Expand All @@ -119,7 +114,7 @@ if (CLANG_FORMAT_VERSION)
execute_process(COMMAND ${CLANG_FORMAT_BIN} "-version"
OUTPUT_VARIABLE CLANG_FORMAT_FOUND_VERSION_MESSAGE
OUTPUT_STRIP_TRAILING_WHITESPACE)
if (NOT ("${CLANG_FORMAT_FOUND_VERSION_MESSAGE}" MATCHES "^clang-format version ${CLANG_FORMAT_MAJOR_VERSION}\\.${CLANG_FORMAT_MINOR_VERSION}.*"))
if (NOT ("${CLANG_FORMAT_FOUND_VERSION_MESSAGE}" MATCHES "^clang-format version ${ARROW_LLVM_MAJOR_VERSION}\\.${ARROW_LLVM_MINOR_VERSION}.*"))
set(CLANG_FORMAT_BIN "CLANG_FORMAT_BIN-NOTFOUND")
endif()
endif()
Expand Down
33 changes: 21 additions & 12 deletions cpp/cmake_modules/FindLLVM.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -20,21 +20,20 @@
# find_package(LLVM)
#

set(GANDIVA_LLVM_VERSION 6.0)

if (APPLE)
# Also look in homebrew for a matching llvm version
find_program(BREW_BIN brew)
if (BREW_BIN)
execute_process(
COMMAND ${BREW_BIN} --prefix "llvm@6"
COMMAND ${BREW_BIN} --prefix "llvm@7"
OUTPUT_VARIABLE LLVM_BREW_PREFIX
OUTPUT_STRIP_TRAILING_WHITESPACE
)
endif()
endif()

find_package(LLVM ${GANDIVA_LLVM_VERSION} REQUIRED CONFIG HINTS
find_package(LLVM ${ARROW_LLVM_VERSION} REQUIRED CONFIG HINTS
/usr/lib
/usr/local/opt/llvm
/usr/share
${LLVM_BREW_PREFIX}
Expand All @@ -45,14 +44,6 @@ message(STATUS "Using LLVMConfig.cmake in: ${LLVM_DIR}")
# Find the libraries that correspond to the LLVM components
llvm_map_components_to_libnames(LLVM_LIBS core mcjit native ipo bitreader target linker analysis debuginfodwarf)

find_program(CLANG_EXECUTABLE clang
HINTS ${LLVM_TOOLS_BINARY_DIR})
if (CLANG_EXECUTABLE)
message(STATUS "Found clang ${CLANG_EXECUTABLE}")
else ()
message(FATAL_ERROR "Couldn't find clang")
endif ()

find_program(LLVM_LINK_EXECUTABLE llvm-link
HINTS ${LLVM_TOOLS_BINARY_DIR})
if (LLVM_LINK_EXECUTABLE)
Expand All @@ -61,6 +52,24 @@ else ()
message(FATAL_ERROR "Couldn't find llvm-link")
endif ()

find_program(CLANG_EXECUTABLE
NAMES clang-${ARROW_LLVM_VERSION}
clang-${ARROW_LLVM_MAJOR_VERSION}
HINTS ${LLVM_TOOLS_BINARY_DIR})
if (CLANG_EXECUTABLE)
message(STATUS "Found clang ${ARROW_LLVM_VERSION} ${CLANG_EXECUTABLE}")
else ()
# If clang-7 not available, switch to normal clang.
find_program(CLANG_EXECUTABLE
NAMES clang
HINTS ${LLVM_TOOLS_BINARY_DIR})
if (CLANG_EXECUTABLE)
message(STATUS "Found clang ${CLANG_EXECUTABLE}")
else ()
message(FATAL_ERROR "Couldn't find clang")
endif ()
endif ()

add_library(LLVM::LLVM_INTERFACE INTERFACE IMPORTED)

set_target_properties(LLVM::LLVM_INTERFACE PROPERTIES
Expand Down
11 changes: 6 additions & 5 deletions cpp/cmake_modules/SetupCxxFlags.cmake
Original file line number Diff line number Diff line change
Expand Up @@ -291,7 +291,7 @@ if (NOT WIN32 AND NOT APPLE)
GET_GOLD_VERSION()
if (GOLD_VERSION)
set(MUST_USE_GOLD 1)
else()
elseif(ARROW_USE_LD_GOLD)
# Can the compiler optionally enable the gold linker?
GET_GOLD_VERSION("-fuse-ld=gold")

Expand Down Expand Up @@ -359,10 +359,11 @@ endif()
# -DARROW_CXXFLAGS="-g" to add them
if (NOT MSVC)
if(ARROW_GGDB_DEBUG)
set(C_FLAGS_DEBUG "-ggdb -O0")
set(C_FLAGS_FASTDEBUG "-ggdb -O1")
set(CXX_FLAGS_DEBUG "-ggdb -O0")
set(CXX_FLAGS_FASTDEBUG "-ggdb -O1")
set(ARROW_DEBUG_SYMBOL_TYPE "gdb")
set(C_FLAGS_DEBUG "-g${ARROW_DEBUG_SYMBOL_TYPE} -O0")
set(C_FLAGS_FASTDEBUG "-g${ARROW_DEBUG_SYMBOL_TYPE} -O1")
set(CXX_FLAGS_DEBUG "-g${ARROW_DEBUG_SYMBOL_TYPE} -O0")
set(CXX_FLAGS_FASTDEBUG "-g${ARROW_DEBUG_SYMBOL_TYPE} -O1")
else()
set(C_FLAGS_DEBUG "-g -O0")
set(C_FLAGS_FASTDEBUG "-g -O1")
Expand Down
2 changes: 2 additions & 0 deletions cpp/src/gandiva/engine.cc
Original file line number Diff line number Diff line change
Expand Up @@ -45,8 +45,10 @@
#include <llvm/Support/raw_ostream.h>
#include <llvm/Transforms/IPO.h>
#include <llvm/Transforms/IPO/PassManagerBuilder.h>
#include <llvm/Transforms/InstCombine/InstCombine.h>
#include <llvm/Transforms/Scalar.h>
#include <llvm/Transforms/Scalar/GVN.h>
#include <llvm/Transforms/Utils.h>
#include <llvm/Transforms/Vectorize.h>

#if defined(_MSC_VER)
Expand Down
2 changes: 1 addition & 1 deletion cpp/src/gandiva/eval_batch.h
Original file line number Diff line number Diff line change
Expand Up @@ -85,7 +85,7 @@ class EvalBatch {
/// An array of 'num_buffers_', each containing a buffer. The buffer
/// sizes depends on the data type, but all of them have the same
/// number of slots (equal to num_records_).
std::unique_ptr<uint8_t* []> buffers_array_;
std::unique_ptr<uint8_t*[]> buffers_array_;

std::unique_ptr<LocalBitMapsHolder> local_bitmaps_holder_;

Expand Down
2 changes: 1 addition & 1 deletion cpp/src/gandiva/local_bitmaps_holder.h
Original file line number Diff line number Diff line change
Expand Up @@ -53,7 +53,7 @@ class LocalBitMapsHolder {
std::vector<std::unique_ptr<uint8_t[]>> local_bitmaps_vec_;

/// An array of the local bitmaps.
std::unique_ptr<uint8_t* []> local_bitmaps_array_;
std::unique_ptr<uint8_t*[]> local_bitmaps_array_;

int64_t local_bitmap_size_;
};
Expand Down
Loading