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 <[email protected]> Gerrit-Reviewer: Adar Dembo <[email protected]> Gerrit-Reviewer: Grant Henke <[email protected]> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <[email protected]> Gerrit-Comment-Date: Mon, 13 Aug 2018 17:43:06 +0000 Gerrit-HasComments: Yes
