This is an archive of the discontinued LLVM Phabricator instance.

Improve error message when '=' is missing in {ASAN,...}_OPTIONS.
ClosedPublic

Authored by marxin on Mar 27 2019, 6:16 AM.

Diff Detail

Repository
rL LLVM

Event Timeline

marxin created this revision.Mar 27 2019, 6:16 AM
marxin updated this revision to Diff 192578.Mar 28 2019, 1:37 AM

I've made the error message more verbose.

marxin added a comment.Apr 8 2019, 5:17 AM

May I please ping this..

marxin updated this revision to Diff 199771.May 16 2019, 2:35 AM

Updated patch with a test-case.

vitalybuka added inline comments.Jun 10 2019, 12:32 PM
lib/asan/asan_flags.cc
123 ↗(On Diff #199771)

If we are making ParseString to know that this is an env option we should just add

asan_parser.ParseStringFromEnv("ASAN_OPTIONS");
and call GetEnv in implementation

Is anything blocks us from that?

test/asan/TestCases/Linux/asan_options_error_message.cc
6 ↗(On Diff #199771)

can you make it sanitizer_common test?

marxin marked 3 inline comments as done.Jun 11 2019, 1:57 AM
marxin added inline comments.
lib/asan/asan_flags.cc
123 ↗(On Diff #199771)

No, good point!

marxin updated this revision to Diff 203988.Jun 11 2019, 1:57 AM
marxin marked an inline comment as done.

Updated version of the patch.

vitalybuka added inline comments.Jun 11 2019, 11:21 AM
lib/hwasan/hwasan.cpp
132 ↗(On Diff #203988)

parser.ParseStringFromEnv("HWASAN_OPTIONS");

lib/msan/msan.cc
178 ↗(On Diff #203988)

ubsan_parser.ParseStringFromEnv("MSAN_OPTIONS")

if you keep this for VPrintf just move VPrintf into ParseStringFromEnv

lib/sanitizer_common/sanitizer_flag_parser.cc
83 ↗(On Diff #203988)

probably we can skip "environment variable" to handle ubsan_default_options-like sources
Printf("%s: ERROR: expected '=' in %s\n", SanitizerToolName, source);

lib/sanitizer_common/sanitizer_flag_parser.h
118 ↗(On Diff #203988)

How about:
void ParseString(const char *s, const char* source /* without = 0 */);
and set it everywhere?
ParseString(s, "ubsan_default_options()");

marxin updated this revision to Diff 204226.Jun 12 2019, 12:54 AM
marxin marked 3 inline comments as done.
marxin marked an inline comment as done.Jun 12 2019, 12:56 AM
marxin added inline comments.
lib/sanitizer_common/sanitizer_flag_parser.h
118 ↗(On Diff #203988)

Well, I don't like it much. There are quite some usages of the function:

git grep '\<ParseString\>' | cat
lib/asan/asan_activation.cc:      parser.ParseString(env);
lib/asan/asan_flags.cc:  asan_parser.ParseString(asan_compile_def);
lib/asan/asan_flags.cc:  asan_parser.ParseString(asan_default_options);
lib/asan/asan_flags.cc:  ubsan_parser.ParseString(ubsan_default_options);
lib/asan/asan_flags.cc:  lsan_parser.ParseString(lsan_default_options);
lib/cfi/cfi.cpp:  ubsan_parser.ParseString(ubsan_default_options);
lib/hwasan/hwasan.cpp:    parser.ParseString(__hwasan_default_options());
lib/hwasan/hwasan.cpp:  ubsan_parser.ParseString(ubsan_default_options);
lib/lsan/lsan.cc:  parser.ParseString(lsan_default_options);
lib/msan/msan.cc:    parser.ParseString(__msan_default_options());
lib/msan/msan.cc:  ubsan_parser.ParseString(ubsan_default_options);
lib/sanitizer_common/sancov_flags.cc:  parser.ParseString(MaybeCallSancovDefaultOptions());
lib/sanitizer_common/sanitizer_flag_parser.cc:  ParseString(env, env_name);
lib/sanitizer_common/sanitizer_flag_parser.cc:void FlagParser::ParseString(const char *s, const char *env_option_name) {
lib/sanitizer_common/sanitizer_flag_parser.cc:  // Backup current parser state to allow nested ParseString() calls.
lib/sanitizer_common/sanitizer_flag_parser.cc:  ParseString(data, path);
lib/sanitizer_common/sanitizer_flag_parser.h:  void ParseString(const char *s, const char *env_name = 0);
lib/sanitizer_common/tests/sanitizer_flags_test.cc:  parser.ParseString(env);
lib/sanitizer_common/tests/sanitizer_flags_test.cc:  parser.ParseString(env);
lib/sanitizer_common/tests/sanitizer_flags_test.cc:  parser.ParseString(env);
lib/sanitizer_common/tests/sanitizer_flags_test.cc:  parser.ParseString("symbolize=1:heap_profile=false log_path='path/two'");
lib/scudo/scudo_flags.cpp:  ScudoParser.ParseString(getCompileDefinitionScudoDefaultOptions());
lib/scudo/scudo_flags.cpp:  ScudoParser.ParseString(getScudoDefaultOptions());
lib/tsan/rtl/tsan_flags.cc:  parser.ParseString(__tsan_default_options());
lib/tsan/rtl/tsan_flags.cc:  ubsan_parser.ParseString(ubsan_default_options);
lib/tsan/rtl/tsan_flags.cc:  parser.ParseString(env, env_option_name);
lib/ubsan/ubsan_flags.cc:  parser.ParseString(MaybeCallUbsanDefaultOptions());
lib/xray/xray_basic_logging.cc:  P.ParseString(useCompilerDefinedBasicFlags());
lib/xray/xray_basic_logging.cc:  P.ParseString(EnvOpts);
lib/xray/xray_basic_logging.cc:  P.ParseString(static_cast<const char *>(Options));
lib/xray/xray_fdr_logging.cc:  FDRParser.ParseString(useCompilerDefinedFlags());
lib/xray/xray_fdr_logging.cc:  FDRParser.ParseString(useCompilerDefinedFDRFlags());
lib/xray/xray_fdr_logging.cc:  FDRParser.ParseString(EnvOpts);
lib/xray/xray_fdr_logging.cc:  FDRParser.ParseString(static_cast<const char *>(Options));
lib/xray/xray_flags.cc:  XRayParser.ParseString(XRayCompileFlags);
lib/xray/xray_profiling.cc:    ConfigParser.ParseString(profilingCompilerDefinedFlags());
lib/xray/xray_profiling.cc:    ConfigParser.ParseString(Env);
lib/xray/xray_profiling.cc:    ConfigParser.ParseString(static_cast<const char *>(Options));
lib/xray/xray_profiling.cc:    ProfilingParser.ParseString(profilingCompilerDefinedFlags());

sometimes the argument is some pointer casting.

This revision is now accepted and ready to land.Jun 12 2019, 1:56 PM

Thank you for review. Can you please install the commit?

Thank you for review. Can you please install the commit?

http://lab.llvm.org:8011 is down
I'll commit it after buildbots get online.

vitalybuka edited the summary of this revision. (Show Details)Jun 14 2019, 6:19 PM
This revision was automatically updated to reflect the committed changes.
Herald added a project: Restricted Project. · View Herald TranscriptJun 14 2019, 6:34 PM