Adar Dembo has posted comments on this change. ( http://gerrit.cloudera.org:8080/11176 )
Change subject: WIP: Move default sanitizer options into build from shell scripts ...................................................................... Patch Set 5: (10 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 Also your line wrapping is weird below. 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 " > Yes, you can still use $TSAN_OPTIONS Just to clarify, what's the exact behavior when the user sets TSAN_OPTIONS? Is kTsanDefaultOptions overridden? 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 conditioned on this. It's just a macro; if it's unused, it won't affect the rest of the program at all. http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/sanitizer_options.cc@30 PS5, Line 30: Nit: remove this. 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. http://gerrit.cloudera.org:8080/#/c/11176/5/src/kudu/sanitizers/sanitizer_options.cc@46 PS5, Line 46: // Default options for ThreadSanitizer: Nit: could you insert each option's comment just above the option itself, rather than having two separate blocks (one for comments and one for options)? Same for the other stuff here. 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 happens? 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_suppressions.cc? Same for the extern of kLsanDefaultSuppressions. If the issue is wanting to take advantage of SANITIZER_HOOK_ATTRIBUTE, you could define that in a common header included from those cc files. 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 kTsanDefaultSuppressions is self-evident. If it were empty I might understand it. Same in lsan_suppressions.cc. 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, no? Same in lsan_suppressions.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: 5 Gerrit-Owner: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Adar Dembo <a...@cloudera.com> Gerrit-Reviewer: Grant Henke <granthe...@apache.org> Gerrit-Reviewer: Kudu Jenkins Gerrit-Reviewer: Todd Lipcon <t...@apache.org> Gerrit-Comment-Date: Fri, 10 Aug 2018 19:35:09 +0000 Gerrit-HasComments: Yes
