You are viewing a plain text version of this content. The canonical link for it is here.
Posted to reviews@kudu.apache.org by "Adar Dembo (Code Review)" <ge...@cloudera.org> on 2017/07/19 00:52:58 UTC
[kudu-CR] KUDU-2068: pass --gcc-toolchain into clang codegen build
Hello Dan Burkert, Todd Lipcon,
I'd like you to do a code review. Please visit
http://gerrit.cloudera.org:8080/7463
to review the following change.
Change subject: KUDU-2068: pass --gcc-toolchain into clang codegen build
......................................................................
KUDU-2068: pass --gcc-toolchain into clang codegen build
As a brief refresher: Kudu can be built using various versions of gcc and
clang, but the codegen build always uses clang from thirdparty.
Without this patch, we delegate finding the system header/library prefix to
clang. The problem is that the result isn't guaranteed to match the prefix
used by the compiler building the rest of Kudu, which may lead to obscure
runtime errors due to std:: class layout mismatches. Kudu consumers using
custom toolchains have been forced to work around this issue [1].
I tested this in the following environments:
- gcc 5.4 system compiler on Ubuntu 16 (--prefix=/usr).
- gcc 4.9 devtoolset-3 compiler on CentOS 6.6
(--prefix=/opt/rh/devtoolset-3/root/usr).
- clang 4.0 thirdparty compiler on Ubuntu 16 (no --prefix).
1. https://github.com/cloudera/native-toolchain/blob/f0105fd35527f416799acb4aa92a887cbf511ce5/source/kudu/build.sh
Change-Id: Id3ccb129432e3915401ca7f1f815ddb355faf597
---
M CMakeLists.txt
M cmake_modules/CompilerInfo.cmake
M src/kudu/codegen/CMakeLists.txt
3 files changed, 21 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/7463/1
--
To view, visit http://gerrit.cloudera.org:8080/7463
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newchange
Gerrit-Change-Id: Id3ccb129432e3915401ca7f1f815ddb355faf597
Gerrit-PatchSet: 1
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2068: pass --gcc-toolchain into clang codegen build
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has submitted this change and it was merged.
Change subject: KUDU-2068: pass --gcc-toolchain into clang codegen build
......................................................................
KUDU-2068: pass --gcc-toolchain into clang codegen build
As a brief refresher: Kudu can be built using various versions of gcc and
clang, but the codegen build always uses clang from thirdparty.
Without this patch, we delegate finding the system header/library prefix to
clang. The problem is that the result isn't guaranteed to match the prefix
used by the compiler building the rest of Kudu, which may lead to obscure
runtime errors due to std:: class layout mismatches. Kudu consumers using
custom toolchains have been forced to work around this issue [1].
There were no build issues in the following environments:
- gcc 5.4 system compiler on Ubuntu 16 (--prefix=/usr).
- gcc 4.9 devtoolset-3 compiler on CentOS 6.6
(--prefix=/opt/rh/devtoolset-3/root/usr).
- clang 4.0 thirdparty compiler on Ubuntu 16 (no --prefix).
Additionally, I built Kudu in the following environments:
- gcc 4.8 system compiler on CentOS 7.3 (--prefix=/usr) with devtoolset-3
also installed.
- gcc 6.2 devtoolset-6 compiler on CentOS 7.3
(--prefix=/opt/rh/devtoolset-6/root/usr).
In these cases codegen-test crashed without the patch, and stopped crashing
after the patch was applied.
1. https://github.com/cloudera/native-toolchain/blob/f0105fd35527f416799acb4aa92a887cbf511ce5/source/kudu/build.sh
Change-Id: Id3ccb129432e3915401ca7f1f815ddb355faf597
Reviewed-on: http://gerrit.cloudera.org:8080/7463
Reviewed-by: Dan Burkert <da...@apache.org>
Tested-by: Kudu Jenkins
---
M CMakeLists.txt
M cmake_modules/CompilerInfo.cmake
M src/kudu/codegen/CMakeLists.txt
3 files changed, 21 insertions(+), 7 deletions(-)
Approvals:
Dan Burkert: Looks good to me, approved
Kudu Jenkins: Verified
--
To view, visit http://gerrit.cloudera.org:8080/7463
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: merged
Gerrit-Change-Id: Id3ccb129432e3915401ca7f1f815ddb355faf597
Gerrit-PatchSet: 3
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2068: pass --gcc-toolchain into clang codegen build
Posted by "Adar Dembo (Code Review)" <ge...@cloudera.org>.
Adar Dembo has uploaded a new patch set (#2).
Change subject: KUDU-2068: pass --gcc-toolchain into clang codegen build
......................................................................
KUDU-2068: pass --gcc-toolchain into clang codegen build
As a brief refresher: Kudu can be built using various versions of gcc and
clang, but the codegen build always uses clang from thirdparty.
Without this patch, we delegate finding the system header/library prefix to
clang. The problem is that the result isn't guaranteed to match the prefix
used by the compiler building the rest of Kudu, which may lead to obscure
runtime errors due to std:: class layout mismatches. Kudu consumers using
custom toolchains have been forced to work around this issue [1].
There were no build issues in the following environments:
- gcc 5.4 system compiler on Ubuntu 16 (--prefix=/usr).
- gcc 4.9 devtoolset-3 compiler on CentOS 6.6
(--prefix=/opt/rh/devtoolset-3/root/usr).
- clang 4.0 thirdparty compiler on Ubuntu 16 (no --prefix).
Additionally, I built Kudu in the following environments:
- gcc 4.8 system compiler on CentOS 7.3 (--prefix=/usr) with devtoolset-3
also installed.
- gcc 6.2 devtoolset-6 compiler on CentOS 7.3
(--prefix=/opt/rh/devtoolset-6/root/usr).
In these cases codegen-test crashed without the patch, and stopped crashing
after the patch was applied.
1. https://github.com/cloudera/native-toolchain/blob/f0105fd35527f416799acb4aa92a887cbf511ce5/source/kudu/build.sh
Change-Id: Id3ccb129432e3915401ca7f1f815ddb355faf597
---
M CMakeLists.txt
M cmake_modules/CompilerInfo.cmake
M src/kudu/codegen/CMakeLists.txt
3 files changed, 21 insertions(+), 7 deletions(-)
git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/63/7463/2
--
To view, visit http://gerrit.cloudera.org:8080/7463
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: newpatchset
Gerrit-Change-Id: Id3ccb129432e3915401ca7f1f815ddb355faf597
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
[kudu-CR] KUDU-2068: pass --gcc-toolchain into clang codegen build
Posted by "Dan Burkert (Code Review)" <ge...@cloudera.org>.
Dan Burkert has posted comments on this change.
Change subject: KUDU-2068: pass --gcc-toolchain into clang codegen build
......................................................................
Patch Set 2: Code-Review+2
--
To view, visit http://gerrit.cloudera.org:8080/7463
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-MessageType: comment
Gerrit-Change-Id: Id3ccb129432e3915401ca7f1f815ddb355faf597
Gerrit-PatchSet: 2
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-Owner: Adar Dembo <ad...@cloudera.com>
Gerrit-Reviewer: Dan Burkert <da...@apache.org>
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon <to...@apache.org>
Gerrit-HasComments: No