[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has submitted this change and it was merged. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. [build] Move default sanitizer options into build from shell scripts This moves the sanitizer options and suppressions from the build scripts using TSAN_OPTIONS/LSAN_OPTIONS/etc into the build and linked at build time. This ensures the same configurations are used for C++ and Java tests. Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Reviewed-on: http://gerrit.cloudera.org:8080/11176 Reviewed-by: Adar Dembo Tested-by: Grant Henke --- M CMakeLists.txt M README.adoc M build-support/dist_test.py M build-support/jenkins/build-and-test.sh D build-support/lsan-suppressions.txt M build-support/run-test.sh M build-support/run_dist_test.py D build-support/tsan-suppressions.txt M src/kudu/benchmarks/CMakeLists.txt M src/kudu/cfile/CMakeLists.txt M src/kudu/client/CMakeLists.txt M src/kudu/clock/CMakeLists.txt M src/kudu/codegen/CMakeLists.txt M src/kudu/common/CMakeLists.txt M src/kudu/consensus/CMakeLists.txt M src/kudu/experiments/CMakeLists.txt M src/kudu/fs/CMakeLists.txt M src/kudu/hms/CMakeLists.txt M src/kudu/integration-tests/CMakeLists.txt M src/kudu/master/CMakeLists.txt M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/rpc/CMakeLists.txt M src/kudu/security/CMakeLists.txt M src/kudu/server/CMakeLists.txt M src/kudu/tablet/CMakeLists.txt M src/kudu/tools/CMakeLists.txt M src/kudu/tserver/CMakeLists.txt M src/kudu/util/CMakeLists.txt A src/kudu/util/sanitizer_options.cc 29 files changed, 312 insertions(+), 241 deletions(-) Approvals: Adar Dembo: Looks good to me, approved Grant Henke: Verified -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: merged Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 14 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 13: Verified+1 -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 13 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 24 Aug 2018 20:57:18 + Gerrit-HasComments: No
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has removed a vote on this change. Change subject: [build] Move default sanitizer options into build from shell scripts .. Removed Verified-1 by Kudu Jenkins (120) -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: deleteVote Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 13 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 11: I verified this works locally and that overrides work locally as well. I also verified the behavior when llvm-symbolizer isn't found by CMake manually. I will run the tests locally one more time before submitting. -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 11 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 24 Aug 2018 19:00:36 + Gerrit-HasComments: No
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/11176 )
Change subject: [build] Move default sanitizer options into build from shell
scripts
..
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11176/11/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/11/src/kudu/util/CMakeLists.txt@282
PS11, Line 282: add_library(kudu_sanitizer_options STATIC sanitizer_options.cc)
> Nit: could call this target just 'sanitizer_options'; we generally prefix w
Done
http://gerrit.cloudera.org:8080/#/c/11176/11/src/kudu/util/CMakeLists.txt@283
PS11, Line 283: target_link_libraries(gutil)
> This should be:
Done
http://gerrit.cloudera.org:8080/#/c/11176/11/src/kudu/util/CMakeLists.txt@291
PS11, Line 291: target_compile_definitions(kudu_sanitizer_options
KUDU_EXTERNAL_SYMBOLIZER_PATH=${KUDU_LLVM_SYMBOLIZER_PATH})
> I think this needs the keyword PRIVATE (after the target name) so that the
Done
--
To view, visit http://gerrit.cloudera.org:8080/11176
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645
Gerrit-Change-Number: 11176
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 24 Aug 2018 18:57:17 +
Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 13: Code-Review+2 Thanks for all the hard work. Please also verify that this works when not using dist-test (precommit uses dist-test I believe). -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 13 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 24 Aug 2018 18:55:56 + Gerrit-HasComments: No
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11176 to look at the new patch set (#13). Change subject: [build] Move default sanitizer options into build from shell scripts .. [build] Move default sanitizer options into build from shell scripts This moves the sanitizer options and suppressions from the build scripts using TSAN_OPTIONS/LSAN_OPTIONS/etc into the build and linked at build time. This ensures the same configurations are used for C++ and Java tests. Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 --- M CMakeLists.txt M README.adoc M build-support/dist_test.py M build-support/jenkins/build-and-test.sh D build-support/lsan-suppressions.txt M build-support/run-test.sh M build-support/run_dist_test.py D build-support/tsan-suppressions.txt M src/kudu/benchmarks/CMakeLists.txt M src/kudu/cfile/CMakeLists.txt M src/kudu/client/CMakeLists.txt M src/kudu/clock/CMakeLists.txt M src/kudu/codegen/CMakeLists.txt M src/kudu/common/CMakeLists.txt M src/kudu/consensus/CMakeLists.txt M src/kudu/experiments/CMakeLists.txt M src/kudu/fs/CMakeLists.txt M src/kudu/hms/CMakeLists.txt M src/kudu/integration-tests/CMakeLists.txt M src/kudu/master/CMakeLists.txt M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/rpc/CMakeLists.txt M src/kudu/security/CMakeLists.txt M src/kudu/server/CMakeLists.txt M src/kudu/tablet/CMakeLists.txt M src/kudu/tools/CMakeLists.txt M src/kudu/tserver/CMakeLists.txt M src/kudu/util/CMakeLists.txt A src/kudu/util/sanitizer_options.cc 29 files changed, 312 insertions(+), 241 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/11176/13 -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 13 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11176 to look at the new patch set (#12). Change subject: [build] Move default sanitizer options into build from shell scripts .. [build] Move default sanitizer options into build from shell scripts This moves the sanitizer options and suppressions from the build scripts using TSAN_OPTIONS/LSAN_OPTIONS/etc into the build and linked at build time. This ensures the same configurations are used for C++ and Java tests. Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 --- M CMakeLists.txt M README.adoc M build-support/dist_test.py M build-support/jenkins/build-and-test.sh D build-support/lsan-suppressions.txt M build-support/run-test.sh M build-support/run_dist_test.py D build-support/tsan-suppressions.txt M src/kudu/benchmarks/CMakeLists.txt M src/kudu/cfile/CMakeLists.txt M src/kudu/client/CMakeLists.txt M src/kudu/clock/CMakeLists.txt M src/kudu/codegen/CMakeLists.txt M src/kudu/common/CMakeLists.txt M src/kudu/consensus/CMakeLists.txt M src/kudu/experiments/CMakeLists.txt M src/kudu/fs/CMakeLists.txt M src/kudu/hms/CMakeLists.txt M src/kudu/integration-tests/CMakeLists.txt M src/kudu/master/CMakeLists.txt M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/rpc/CMakeLists.txt M src/kudu/security/CMakeLists.txt M src/kudu/server/CMakeLists.txt M src/kudu/tablet/CMakeLists.txt M src/kudu/tools/CMakeLists.txt M src/kudu/tserver/CMakeLists.txt M src/kudu/util/CMakeLists.txt A src/kudu/util/sanitizer_options.cc 29 files changed, 312 insertions(+), 241 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/11176/12 -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 12 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11176 )
Change subject: [build] Move default sanitizer options into build from shell
scripts
..
Patch Set 11:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11176/11/src/kudu/util/CMakeLists.txt
File src/kudu/util/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/11/src/kudu/util/CMakeLists.txt@282
PS11, Line 282: add_library(kudu_sanitizer_options STATIC sanitizer_options.cc)
Nit: could call this target just 'sanitizer_options'; we generally prefix with
kudu only when the rest of the target name is vague.
http://gerrit.cloudera.org:8080/#/c/11176/11/src/kudu/util/CMakeLists.txt@283
PS11, Line 283: target_link_libraries(gutil)
This should be:
target_link_libraries(kudu_sanitizer_options, gutil)
http://gerrit.cloudera.org:8080/#/c/11176/11/src/kudu/util/CMakeLists.txt@291
PS11, Line 291: target_compile_definitions(kudu_sanitizer_options
KUDU_EXTERNAL_SYMBOLIZER_PATH=${KUDU_LLVM_SYMBOLIZER_PATH})
I think this needs the keyword PRIVATE (after the target name) so that the
compile definitions don't make it to other dependent targets.
--
To view, visit http://gerrit.cloudera.org:8080/11176
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645
Gerrit-Change-Number: 11176
Gerrit-PatchSet: 11
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 24 Aug 2018 18:48:50 +
Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11176 to look at the new patch set (#11). Change subject: [build] Move default sanitizer options into build from shell scripts .. [build] Move default sanitizer options into build from shell scripts This moves the sanitizer options and suppressions from the build scripts using TSAN_OPTIONS/LSAN_OPTIONS/etc into the build and linked at build time. This ensures the same configurations are used for C++ and Java tests. Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 --- M CMakeLists.txt M README.adoc M build-support/dist_test.py M build-support/jenkins/build-and-test.sh D build-support/lsan-suppressions.txt M build-support/run-test.sh M build-support/run_dist_test.py D build-support/tsan-suppressions.txt M src/kudu/benchmarks/CMakeLists.txt M src/kudu/cfile/CMakeLists.txt M src/kudu/client/CMakeLists.txt M src/kudu/clock/CMakeLists.txt M src/kudu/codegen/CMakeLists.txt M src/kudu/common/CMakeLists.txt M src/kudu/consensus/CMakeLists.txt M src/kudu/experiments/CMakeLists.txt M src/kudu/fs/CMakeLists.txt M src/kudu/hms/CMakeLists.txt M src/kudu/integration-tests/CMakeLists.txt M src/kudu/master/CMakeLists.txt M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/rpc/CMakeLists.txt M src/kudu/security/CMakeLists.txt M src/kudu/server/CMakeLists.txt M src/kudu/tablet/CMakeLists.txt M src/kudu/tools/CMakeLists.txt M src/kudu/tserver/CMakeLists.txt M src/kudu/util/CMakeLists.txt A src/kudu/util/sanitizer_options.cc 29 files changed, 312 insertions(+), 241 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/11176/11 -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 11 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc File src/kudu/util/sanitizer_options.cc: http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc@42 PS10, Line 42: #if defined(KUDU_EXTERNAL_SYMBOLIZER_PATH) > I will move it. I think it's valid an potentially useful to allow KUDU_EXTERNAL_SYMBOLIZER_PATH to be unset. I used it while testing CMake file changes and overrides. Given our build reliably defines it I don't think we need to make it an error. -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 24 Aug 2018 17:58:55 + Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc File src/kudu/util/sanitizer_options.cc: http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc@35 PS10, Line 35: // A macro to turn a symbol into a string. : #define AS_STRING(x) AS_STRING_INTERNAL(x) : #define AS_STRING_INTERNAL(x) #x > Given this is all I use I preferred not to link in gutil. I will just link this. http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc@42 PS10, Line 42: #if defined(KUDU_EXTERNAL_SYMBOLIZER_PATH) > I liked the idea of defining this in the same location as all the other san I will move it. -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 24 Aug 2018 17:47:44 + Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc File src/kudu/util/sanitizer_options.cc: http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc@35 PS10, Line 35: // A macro to turn a symbol into a string. : #define AS_STRING(x) AS_STRING_INTERNAL(x) : #define AS_STRING_INTERNAL(x) #x > Can't you just include gutil/macros.h and use that? Given this is all I use I preferred not to link in gutil. http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc@42 PS10, Line 42: #if defined(KUDU_EXTERNAL_SYMBOLIZER_PATH) > > Also, where are we setting this? I expected an add_compile_definitions() I liked the idea of defining this in the same location as all the other sanitizer configs. -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 24 Aug 2018 17:40:59 + Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 10: (1 comment) http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc File src/kudu/util/sanitizer_options.cc: http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc@42 PS10, Line 42: #if defined(KUDU_EXTERNAL_SYMBOLIZER_PATH) > Also, where are we setting this? I expected an add_compile_definitions() or > something equivalent in util/CMakeLists.txt. I missed the diff in CMakeLists.txt. Rather than adding this definition to every target, could we add it just to the target that builds sanitizer_options.cc? -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 24 Aug 2018 17:35:38 + Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 10: (2 comments) http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc File src/kudu/util/sanitizer_options.cc: http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc@35 PS10, Line 35: // A macro to turn a symbol into a string. : #define AS_STRING(x) AS_STRING_INTERNAL(x) : #define AS_STRING_INTERNAL(x) #x Can't you just include gutil/macros.h and use that? http://gerrit.cloudera.org:8080/#/c/11176/10/src/kudu/util/sanitizer_options.cc@42 PS10, Line 42: #if defined(KUDU_EXTERNAL_SYMBOLIZER_PATH) Perhaps we should #error if this isn't defined? Do we really want to allow that case? Also, where are we setting this? I expected an add_compile_definitions() or something equivalent in util/CMakeLists.txt. -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Fri, 24 Aug 2018 17:34:13 + Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/11176 )
Change subject: [build] Move default sanitizer options into build from shell
scripts
..
Patch Set 10:
(7 comments)
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1279
PS8, Line 1279:
> extra space here
Done
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1280
PS8, Line 1280: AGS "$
> could be
Done
http://gerrit.cloudera.org:8080/#/c/11176/9/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/9/CMakeLists.txt@1281
PS9, Line 1281:
> Do we still need these two in the list now that we have our fancy macro?
yeah, these are still the base KUDU_MIN_TEST_LIBS.
http://gerrit.cloudera.org:8080/#/c/11176/9/CMakeLists.txt@1284
PS9, Line 1284:
> You mean KUDU_TEST_LINK_LIBS here.
Done
http://gerrit.cloudera.org:8080/#/c/11176/9/CMakeLists.txt@1291
PS9, Line 1291: # Prepend SANITIZER_OPTIONS_OVERRIDE if this is a sanitizer
build.
> Have you looked into cmake's list() functions? They may simplify this a bit
Done
http://gerrit.cloudera.org:8080/#/c/11176/8/build-support/run-test.sh
File build-support/run-test.sh:
http://gerrit.cloudera.org:8080/#/c/11176/8/build-support/run-test.sh@115
PS8, Line 115: # Set a 15-minute timeout for tests run via 'make test'.
: # This keeps our jenkins builds from hanging in the case that
there's
: # a deadlock or anything.
: #
: # NOTE: this should be kept in sync with the default value of
ARG_TIMEOUT
: # in the definition of ADD_KUDU_TEST in the top-level
CMakeLists.txt.
: KUDU_TEST_TIMEOUT=${KUDU_TEST_TIMEOUT:-900}
:
: # Allow for collecting core dumps.
: KUDU_TEST_ULIMIT_CORE=${KUDU_TEST_ULIMIT_CORE:-0}
: ulim
> I added TODOs to follow up and remove both of these with a patch that pushe
I learned a bit more about CMake and pushed this down into the build and
removed it from both locations.
http://gerrit.cloudera.org:8080/#/c/11176/9/src/kudu/benchmarks/CMakeLists.txt
File src/kudu/benchmarks/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/9/src/kudu/benchmarks/CMakeLists.txt@59
PS9, Line 59: SET_KUDU_TEST_LINK_LIBS(tpch)
> Not using the macro here?
Done
--
To view, visit http://gerrit.cloudera.org:8080/11176
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645
Gerrit-Change-Number: 11176
Gerrit-PatchSet: 10
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 24 Aug 2018 17:17:59 +
Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11176 to look at the new patch set (#10). Change subject: [build] Move default sanitizer options into build from shell scripts .. [build] Move default sanitizer options into build from shell scripts This moves the sanitizer options and suppressions from the build scripts using TSAN_OPTIONS/LSAN_OPTIONS/etc into the build and linked at build time. This ensures the same configurations are used for C++ and Java tests. Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 --- M CMakeLists.txt M README.adoc M build-support/dist_test.py M build-support/jenkins/build-and-test.sh D build-support/lsan-suppressions.txt M build-support/run-test.sh M build-support/run_dist_test.py D build-support/tsan-suppressions.txt M src/kudu/benchmarks/CMakeLists.txt M src/kudu/cfile/CMakeLists.txt M src/kudu/client/CMakeLists.txt M src/kudu/clock/CMakeLists.txt M src/kudu/codegen/CMakeLists.txt M src/kudu/common/CMakeLists.txt M src/kudu/consensus/CMakeLists.txt M src/kudu/experiments/CMakeLists.txt M src/kudu/fs/CMakeLists.txt M src/kudu/hms/CMakeLists.txt M src/kudu/integration-tests/CMakeLists.txt M src/kudu/master/CMakeLists.txt M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/rpc/CMakeLists.txt M src/kudu/security/CMakeLists.txt M src/kudu/server/CMakeLists.txt M src/kudu/tablet/CMakeLists.txt M src/kudu/tools/CMakeLists.txt M src/kudu/tserver/CMakeLists.txt M src/kudu/util/CMakeLists.txt A src/kudu/util/sanitizer_options.cc 29 files changed, 311 insertions(+), 240 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/11176/10 -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 10 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11176 )
Change subject: [build] Move default sanitizer options into build from shell
scripts
..
Patch Set 9:
(6 comments)
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1279
PS8, Line 1279:
> extra space here
Missed this.
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1280
PS8, Line 1280: coulbe
> could be
And this.
http://gerrit.cloudera.org:8080/#/c/11176/9/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/9/CMakeLists.txt@1281
PS9, Line 1281: kudu_test_main kudu_test_util
Do we still need these two in the list now that we have our fancy macro?
http://gerrit.cloudera.org:8080/#/c/11176/9/CMakeLists.txt@1284
PS9, Line 1284: SET_KUDU_TEST_LINK_LIBS
You mean KUDU_TEST_LINK_LIBS here.
http://gerrit.cloudera.org:8080/#/c/11176/9/CMakeLists.txt@1291
PS9, Line 1291: set(KUDU_TEST_LINK_LIBS ${KUDU_TEST_LINK_LIBS} ${ARG})
Have you looked into cmake's list() functions? They may simplify this a bit:
https://cmake.org/cmake/help/latest/command/list.html#append
http://gerrit.cloudera.org:8080/#/c/11176/9/src/kudu/benchmarks/CMakeLists.txt
File src/kudu/benchmarks/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/9/src/kudu/benchmarks/CMakeLists.txt@59
PS9, Line 59: set(KUDU_TEST_LINK_LIBS ${KUDU_TEST_LINK_LIBS} tpch)
Not using the macro here?
--
To view, visit http://gerrit.cloudera.org:8080/11176
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645
Gerrit-Change-Number: 11176
Gerrit-PatchSet: 9
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 24 Aug 2018 16:25:18 +
Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/11176 )
Change subject: [build] Move default sanitizer options into build from shell
scripts
..
Patch Set 8:
(2 comments)
http://gerrit.cloudera.org:8080/#/c/11176/8/build-support/run-test.sh
File build-support/run-test.sh:
http://gerrit.cloudera.org:8080/#/c/11176/8/build-support/run-test.sh@115
PS8, Line 115: # By default the sanitizers use addr2line utility to symbolize
reports.
: # llvm-symbolizer is faster, consumes less memory and produces
much better reports.
: #
https://github.com/google/sanitizers/wiki/SanitizerCommonFlags
:
SYMBOLIZER_PATH="$SOURCE_ROOT/thirdparty/installed/uninstrumented/bin/llvm-symbolizer"
: for SANITIZER in ASAN LSAN MSAN TSAN UBSAN; do
: VAR_NAME="${SANITIZER}_OPTIONS"
: # Only add the external_symbolizer_path if it is not set
already.
: if [[ ${!VAR_NAME} != *"external_symbolizer_path="* ]]; then
:export ${SANITIZER}_OPTIONS="$(echo ${!VAR_NAME})
external_symbolizer_path=$SYMBOLIZER_PATH"
: fi
: done
> There are only 2 copies right now, and I think this one can go away once we
I added TODOs to follow up and remove both of these with a patch that pushes
the logic into the build.
http://gerrit.cloudera.org:8080/#/c/11176/8/src/kudu/cfile/CMakeLists.txt
File src/kudu/cfile/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/8/src/kudu/cfile/CMakeLists.txt@58
PS8, Line 58: set(KUDU_TEST_LINK_LIBS ${KUDU_MIN_TEST_LIBS} cfile)
> I tried modifying the build with functions and macros all failing to work.
I figured this out. I need to use a macro for scoping reasons and appending
ARGN directly wasn't working as I expected.
--
To view, visit http://gerrit.cloudera.org:8080/11176
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645
Gerrit-Change-Number: 11176
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 24 Aug 2018 14:00:15 +
Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11176 to look at the new patch set (#9). Change subject: [build] Move default sanitizer options into build from shell scripts .. [build] Move default sanitizer options into build from shell scripts This moves the sanitizer options and suppressions from the build scripts using TSAN_OPTIONS/LSAN_OPTIONS/etc into the build and linked at build time. This ensures the same configurations are used for C++ and Java tests. Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 --- M CMakeLists.txt M README.adoc M build-support/dist_test.py M build-support/jenkins/build-and-test.sh D build-support/lsan-suppressions.txt M build-support/run-test.sh M build-support/run_dist_test.py D build-support/tsan-suppressions.txt M src/kudu/benchmarks/CMakeLists.txt M src/kudu/cfile/CMakeLists.txt M src/kudu/client/CMakeLists.txt M src/kudu/clock/CMakeLists.txt M src/kudu/codegen/CMakeLists.txt M src/kudu/common/CMakeLists.txt M src/kudu/consensus/CMakeLists.txt M src/kudu/experiments/CMakeLists.txt M src/kudu/fs/CMakeLists.txt M src/kudu/hms/CMakeLists.txt M src/kudu/integration-tests/CMakeLists.txt M src/kudu/master/CMakeLists.txt M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/rpc/CMakeLists.txt M src/kudu/security/CMakeLists.txt M src/kudu/server/CMakeLists.txt M src/kudu/tablet/CMakeLists.txt M src/kudu/tools/CMakeLists.txt M src/kudu/tserver/CMakeLists.txt M src/kudu/util/CMakeLists.txt A src/kudu/util/sanitizer_options.cc 29 files changed, 294 insertions(+), 231 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/11176/9 -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 9 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. (
http://gerrit.cloudera.org:8080/11176 )
Change subject: [build] Move default sanitizer options into build from shell
scripts
..
Patch Set 8:
(3 comments)
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1221
PS8, Line 1221: set(KRB5_REALM_OVERRIDE -Wl,-U,krb5_realm_override_loaded
krb5_realm_override)
> The old comment said that macOS's krb5 is new enough that it doesn't need t
It said the reason it didn't do it was because the flag was not supported.
Since I found a supported flag, I added it for consistency. This makes Mac
behave the same way as Linux.
http://gerrit.cloudera.org:8080/#/c/11176/8/build-support/run-test.sh
File build-support/run-test.sh:
http://gerrit.cloudera.org:8080/#/c/11176/8/build-support/run-test.sh@115
PS8, Line 115: # By default the sanitizers use addr2line utility to symbolize
reports.
: # llvm-symbolizer is faster, consumes less memory and produces
much better reports.
: #
https://github.com/google/sanitizers/wiki/SanitizerCommonFlags
:
SYMBOLIZER_PATH="$SOURCE_ROOT/thirdparty/installed/uninstrumented/bin/llvm-symbolizer"
: for SANITIZER in ASAN LSAN MSAN TSAN UBSAN; do
: VAR_NAME="${SANITIZER}_OPTIONS"
: # Only add the external_symbolizer_path if it is not set
already.
: if [[ ${!VAR_NAME} != *"external_symbolizer_path="* ]]; then
:export ${SANITIZER}_OPTIONS="$(echo ${!VAR_NAME})
external_symbolizer_path=$SYMBOLIZER_PATH"
: fi
: done
> Can all of the shell versions of this snippet be decomposed into a helper s
There are only 2 copies right now, and I think this one can go away once we
move the local llvm-symbolizer config to the binary.
http://gerrit.cloudera.org:8080/#/c/11176/8/src/kudu/cfile/CMakeLists.txt
File src/kudu/cfile/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/8/src/kudu/cfile/CMakeLists.txt@58
PS8, Line 58: set(KUDU_TEST_LINK_LIBS ${KUDU_MIN_TEST_LIBS} cfile)
> It would be nice to enforce this by making KUDU_TEST_LINK_LIBS additive onl
I tried modifying the build with functions and macros all failing to work. I
will take another crack at it in the morning.
--
To view, visit http://gerrit.cloudera.org:8080/11176
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645
Gerrit-Change-Number: 11176
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Fri, 24 Aug 2018 03:49:59 +
Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Adar Dembo has posted comments on this change. (
http://gerrit.cloudera.org:8080/11176 )
Change subject: [build] Move default sanitizer options into build from shell
scripts
..
Patch Set 8:
(5 comments)
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt
File CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1221
PS8, Line 1221: set(KRB5_REALM_OVERRIDE -Wl,-U,krb5_realm_override_loaded
krb5_realm_override)
The old comment said that macOS's krb5 is new enough that it doesn't need this,
so why do it?
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1279
PS8, Line 1279:
extra space here
http://gerrit.cloudera.org:8080/#/c/11176/8/CMakeLists.txt@1280
PS8, Line 1280: coulbe
could be
http://gerrit.cloudera.org:8080/#/c/11176/8/build-support/run-test.sh
File build-support/run-test.sh:
http://gerrit.cloudera.org:8080/#/c/11176/8/build-support/run-test.sh@115
PS8, Line 115: # By default the sanitizers use addr2line utility to symbolize
reports.
: # llvm-symbolizer is faster, consumes less memory and produces
much better reports.
: #
https://github.com/google/sanitizers/wiki/SanitizerCommonFlags
:
SYMBOLIZER_PATH="$SOURCE_ROOT/thirdparty/installed/uninstrumented/bin/llvm-symbolizer"
: for SANITIZER in ASAN LSAN MSAN TSAN UBSAN; do
: VAR_NAME="${SANITIZER}_OPTIONS"
: # Only add the external_symbolizer_path if it is not set
already.
: if [[ ${!VAR_NAME} != *"external_symbolizer_path="* ]]; then
:export ${SANITIZER}_OPTIONS="$(echo ${!VAR_NAME})
external_symbolizer_path=$SYMBOLIZER_PATH"
: fi
: done
Can all of the shell versions of this snippet be decomposed into a helper shell
script in build-support that is then sourced?
http://gerrit.cloudera.org:8080/#/c/11176/8/src/kudu/cfile/CMakeLists.txt
File src/kudu/cfile/CMakeLists.txt:
http://gerrit.cloudera.org:8080/#/c/11176/8/src/kudu/cfile/CMakeLists.txt@58
PS8, Line 58: set(KUDU_TEST_LINK_LIBS ${KUDU_MIN_TEST_LIBS} cfile)
It would be nice to enforce this by making KUDU_TEST_LINK_LIBS additive only.
You could do that by definition a cmake function that calls set() and always
puts the new stuff at the end. Then as long as everyone uses that function to
modify KUDU_TEST_LINK_LIBS, there's no danger of accidentally fudging the order.
It's not total enforcement (someone could always set(KUDU_TEST_LINK_LIBS ...)
themselves), but it's something.
--
To view, visit http://gerrit.cloudera.org:8080/11176
To unsubscribe, visit http://gerrit.cloudera.org:8080/settings
Gerrit-Project: kudu
Gerrit-Branch: master
Gerrit-MessageType: comment
Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645
Gerrit-Change-Number: 11176
Gerrit-PatchSet: 8
Gerrit-Owner: Grant Henke
Gerrit-Reviewer: Adar Dembo
Gerrit-Reviewer: Grant Henke
Gerrit-Reviewer: Kudu Jenkins
Gerrit-Reviewer: Todd Lipcon
Gerrit-Comment-Date: Thu, 23 Aug 2018 22:42:45 +
Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11176 to look at the new patch set (#8). Change subject: [build] Move default sanitizer options into build from shell scripts .. [build] Move default sanitizer options into build from shell scripts This moves the sanitizer options and suppressions from the build scripts using TSAN_OPTIONS/LSAN_OPTIONS/etc into the build and linked at build time. This ensures the same configurations are used for C++ and Java tests. Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 --- M CMakeLists.txt M README.adoc M build-support/dist_test.py M build-support/jenkins/build-and-test.sh D build-support/lsan-suppressions.txt M build-support/run-test.sh M build-support/run_dist_test.py D build-support/tsan-suppressions.txt M src/kudu/cfile/CMakeLists.txt M src/kudu/client/CMakeLists.txt M src/kudu/clock/CMakeLists.txt M src/kudu/codegen/CMakeLists.txt M src/kudu/common/CMakeLists.txt M src/kudu/consensus/CMakeLists.txt M src/kudu/experiments/CMakeLists.txt M src/kudu/fs/CMakeLists.txt M src/kudu/hms/CMakeLists.txt M src/kudu/integration-tests/CMakeLists.txt M src/kudu/master/CMakeLists.txt M src/kudu/mini-cluster/CMakeLists.txt M src/kudu/rpc/CMakeLists.txt M src/kudu/security/CMakeLists.txt M src/kudu/server/CMakeLists.txt M src/kudu/tablet/CMakeLists.txt M src/kudu/tools/CMakeLists.txt M src/kudu/tserver/CMakeLists.txt M src/kudu/util/CMakeLists.txt A src/kudu/util/sanitizer_options.cc 28 files changed, 279 insertions(+), 221 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/11176/8 -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 8 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/11176/7/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/11176/7/build-support/jenkins/build-and-test.sh@176 PS7, Line 176: # Sanitizer suppressions require symbolization. > Does this work? Don't PATH entries need to be directories? No it doesn't..which is why the tsan build failed. This was a late night edit just so I could kick off a build and see in the morning. http://gerrit.cloudera.org:8080/#/c/11176/7/build-support/jenkins/build-and-test.sh@180 PS7, Line 180: export PATH=$(pwd)/thirdparty/installed/uninstrumented/bin/llvm-symbolizer:$PATH > So the downside of placing this here is that we don't get it if we run Java I will do some manual testing today and see what I can come up with. If I can't resolve this we are no worse off than we were before. -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 21 Aug 2018 15:24:41 + Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 7: (3 comments) The past few runs have been failing in TSAN in the same way. I suspect the suppressions (or maybe all of the options) aren't getting through. http://gerrit.cloudera.org:8080/#/c/11176/7//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11176/7//COMMIT_MSG@17 PS7, Line 17: The only option that was not provided at build time is : the external_symbolizer_path. Instead the : llvm_symbolizer from thirdparty is added to the PATH : so it can be found if no custom : external_symbolizer_path is provided. Please explain why this deviation was necessary. http://gerrit.cloudera.org:8080/#/c/11176/7/build-support/jenkins/build-and-test.sh File build-support/jenkins/build-and-test.sh: http://gerrit.cloudera.org:8080/#/c/11176/7/build-support/jenkins/build-and-test.sh@176 PS7, Line 176: # Sanitizer suppressions require symbolization. Does this work? Don't PATH entries need to be directories? $ PATH=~/Source/kudu/thirdparty/installed/uninstrumented/bin/llvm-symbolizer:$PATH; llvm-symbolizer --version Command 'llvm-symbolizer' not found, but can be installed with: sudo apt install llvm $ PATH=~/Source/kudu/thirdparty/installed/uninstrumented/bin:$PATH; llvm-symbolizer --version LLVM (http://llvm.org/): LLVM version 6.0.0 Optimized build. Default target: x86_64-unknown-linux-gnu Host CPU: skylake http://gerrit.cloudera.org:8080/#/c/11176/7/build-support/jenkins/build-and-test.sh@180 PS7, Line 180: export PATH=$(pwd)/thirdparty/installed/uninstrumented/bin/llvm-symbolizer:$PATH So the downside of placing this here is that we don't get it if we run Java tests locally, or via an IDE, or basically in any other way that doesn't go through build-and-test.sh and doesn't use dist-test. -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 21 Aug 2018 04:28:29 + Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 7: (2 comments) http://gerrit.cloudera.org:8080/#/c/11176/6/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11176/6/src/kudu/util/CMakeLists.txt@327 PS6, Line 327: kudu_test_util) > Nit: should precede kudu_util. Done http://gerrit.cloudera.org:8080/#/c/11176/6/src/kudu/util/sanitizer_options.cc File src/kudu/util/sanitizer_options.cc: http://gerrit.cloudera.org:8080/#/c/11176/6/src/kudu/util/sanitizer_options.cc@35 PS6, Line 35: #endif // ADDRESS_SANITIZER > Nit: could we inline kAsanDefaultOptions directly into the return statement Done -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Tue, 21 Aug 2018 03:27:54 + Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11176 to look at the new patch set (#7). Change subject: [build] Move default sanitizer options into build from shell scripts .. [build] Move default sanitizer options into build from shell scripts This moves the sanitizer options and suppressions from the build scripts using TSAN_OPTIONS/LSAN_OPTIONS/etc into the build and linked at build time. This ensures the same configurations are used for C++ and Java tests. The only option that was not provided at build time is the external_symbolizer_path. Instead the llvm_symbolizer from thirdparty is added to the PATH so it can be found if no custom external_symbolizer_path is provided. Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 --- M README.adoc M build-support/dist_test.py M build-support/jenkins/build-and-test.sh D build-support/lsan-suppressions.txt M build-support/run-test.sh M build-support/run_dist_test.py D build-support/tsan-suppressions.txt M src/kudu/util/CMakeLists.txt A src/kudu/util/sanitizer_options.cc 9 files changed, 207 insertions(+), 178 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/11176/7 -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 7 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 6: (3 comments) http://gerrit.cloudera.org:8080/#/c/11176/6//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11176/6//COMMIT_MSG@9 PS6, Line 9: supressions suppressions http://gerrit.cloudera.org:8080/#/c/11176/6/src/kudu/util/CMakeLists.txt File src/kudu/util/CMakeLists.txt: http://gerrit.cloudera.org:8080/#/c/11176/6/src/kudu/util/CMakeLists.txt@327 PS6, Line 327: kudu_sanitizer_options) Nit: should precede kudu_util. http://gerrit.cloudera.org:8080/#/c/11176/6/src/kudu/util/sanitizer_options.cc File src/kudu/util/sanitizer_options.cc: http://gerrit.cloudera.org:8080/#/c/11176/6/src/kudu/util/sanitizer_options.cc@35 PS6, Line 35: return kAsanDefaultOptions; Nit: could we inline kAsanDefaultOptions directly into the return statement? As is you have to follow from the variable definition to the return statement in __asan_default_options; inlining would eliminate that following. Same for the other options/suppressions below. -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 13 Aug 2018 22:28:06 + Gerrit-HasComments: Yes
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Hello Kudu Jenkins, Adar Dembo, Todd Lipcon, I'd like you to reexamine a change. Please visit http://gerrit.cloudera.org:8080/11176 to look at the new patch set (#6). Change subject: [build] Move default sanitizer options into build from shell scripts .. [build] Move default sanitizer options into build from shell scripts This moves the sanitizer options and supressions from the build scripts using TSAN_OPTIONS/LSAN_OPTIONS/etc into the build and linked at build time. This ensures the same configurations are used for C++ and Java tests. Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 --- M README.adoc M build-support/dist_test.py M build-support/jenkins/build-and-test.sh D build-support/lsan-suppressions.txt M build-support/run-test.sh M build-support/run_dist_test.py D build-support/tsan-suppressions.txt M src/kudu/util/CMakeLists.txt A src/kudu/util/sanitizer_options.cc 9 files changed, 205 insertions(+), 181 deletions(-) git pull ssh://gerrit.cloudera.org:29418/kudu refs/changes/76/11176/6 -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: newpatchset Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 6 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon
[kudu-CR] [build] Move default sanitizer options into build from shell scripts
Grant Henke has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 ) Change subject: [build] Move default sanitizer options into build from shell scripts .. Patch Set 5: (9 comments) http://gerrit.cloudera.org:8080/#/c/11176/5//COMMIT_MSG Commit Message: http://gerrit.cloudera.org:8080/#/c/11176/5//COMMIT_MSG@9 PS5, Line 9: This moves the sanitzer options and supressions > Nit: sanitizer Done http://gerrit.cloudera.org:8080/#/c/11176/4/src/kudu/sanitizers/sanitizer_options.cc File src/kudu/sanitizers/sanitizer_options.cc: http://gerrit.cloudera.org:8080/#/c/11176/4/src/kudu/sanitizers/sanitizer_options.cc@57 PS4, Line 57: "history_size=7 " > Just to clarify, what's the exact behavior when the user sets TSAN_OPTIONS? These are treated as defaults that the environment variables can override. The right most flag is the final value used. Here is where the defaults and then env flags are processed: https://github.com/llvm-mirror/compiler-rt/blob/96ccf181c8c1bf446bae8fc738b3dc8da5a65cfe/lib/tsan/rtl/tsan_flags.cc#L89-L96 Here is where the duplicate flags are handled: https://github.com/llvm-mirror/compiler-rt/blob/master/lib/sanitizer_common/sanitizer_flag_parser.cc#L148-L152 http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/sanitizer_options.cc File src/kudu/sanitizers/sanitizer_options.cc: http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/sanitizer_options.cc@19 PS5, Line 19: #if defined(ADDRESS_SANITIZER) || defined(LEAK_SANITIZER) || \ : defined(MEMORY_SANITIZER) || defined(THREAD_SANITIZER) || \ : defined(UNDEFINED_SANITIZER) > I don't see why the definition of SANITIZER_HOOK_ATTRIBUTE needs to be cond Done http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/sanitizer_options.cc@30 PS5, Line 30: > Nit: remove this. Done http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/sanitizer_options.cc@45 PS5, Line 45: defined(OS_LINUX) > I don't think this is ever set by the build. oops, meant to remove this. http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/sanitizer_options.cc@54 PS5, Line 54: // TODO: " external_symbolizer_path=" + env['ASAN_SYMBOLIZER_PATH'] > So what does this TODO mean? Can this patch be merged without it? What happ The llvm-symbolizer needs to be passed in order to support suppressions and improved stack traces on sanitizer errors. I am not sure the best way to default to the one in thirdparty from here. Perhaps we can't and I just need to use the env variables we were using before for the symbolizer. See "external_symbolizer_path" here: https://github.com/google/sanitizers/wiki/ThreadSanitizerFlags See ASAN_SYMBOLIZER_PATH here: https://clang.llvm.org/docs/AddressSanitizer.html#symbolizing-the-reports http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/sanitizer_options.cc@64 PS5, Line 64: extern char kTSanDefaultSuppressions[]; > Can we avoid this by moving __tsan_default_suppressions into tsan_suppressi I can just move it all into this file. http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/tsan_suppressions.cc File src/kudu/sanitizers/tsan_suppressions.cc: http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/tsan_suppressions.cc@21 PS5, Line 21: // Please make sure the code below declares a single string variable : // kTSanDefaultSuppressions which contains TSan suppressions delimited by : // newlines. > Not really understanding this either; the structure of kTsanDefaultSuppress This syntax weirdness is carry over from the examples I used. I will remove all of it. http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/tsan_suppressions.cc@120 PS5, Line 120: ; // Please keep this semicolon. > Not sure I get the comment; without the semicolon this is a syntax error, n Done -- To view, visit http://gerrit.cloudera.org:8080/11176 To unsubscribe, visit http://gerrit.cloudera.org:8080/settings Gerrit-Project: kudu Gerrit-Branch: master Gerrit-MessageType: comment Gerrit-Change-Id: I9eab2d3ef0b107ae9a4e971bf8a6514bf425f645 Gerrit-Change-Number: 11176 Gerrit-PatchSet: 5 Gerrit-Owner: Grant Henke Gerrit-Reviewer: Adar Dembo Gerrit-Reviewer: Grant Henke Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon Gerrit-Comment-Date: Mon, 13 Aug 2018 17:43:06 + Gerrit-HasComments: Yes
