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

[marxin]

Summary

It's handling isses as described here:
https://gcc.gnu.org/bugzilla/show_bug.cgi?id=89832

Patch by Martin Liška.

[marxin]

I've made the error message more verbose.

[marxin]

May I please ping this..

[marxin]

Updated patch with a test-case.

[vitalybuka]

lib/asan/asan_flags.cc

123

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

can you make it sanitizer_common test?

[marxin]

lib/asan/asan_flags.cc

123

No, good point!

[marxin]

Updated version of the patch.

[vitalybuka]

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

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

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

[marxin]

lib/sanitizer_common/sanitizer_flag_parser.h

118

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.

[vitalybuka]

LGTM

[marxin]

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

[vitalybuka]

In D59876#1541080, @marxin wrote:

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.

lib/asan/asan_flags.cc

#if CAN_SANITIZE_UB
   ubsan_parser.ParseString(ubsan_default_options);
 #endif
 #if CAN_SANITIZE_LEAKS
   const char *lsan_default_options = __lsan::MaybeCallLsanDefaultOptions();
   lsan_parser.ParseString(lsan_default_options);
 #endif
 
   // Override from command line.
-  asan_parser.ParseString(GetEnv("ASAN_OPTIONS"));
+  asan_parser.ParseString(GetEnv("ASAN_OPTIONS"), "ASAN_OPTIONS");
 #if CAN_SANITIZE_LEAKS
-  lsan_parser.ParseString(GetEnv("LSAN_OPTIONS"));
+  lsan_parser.ParseString(GetEnv("LSAN_OPTIONS"), "LSAN_OPTIONS");
 #endif
 #if CAN_SANITIZE_UB
-  ubsan_parser.ParseString(GetEnv("UBSAN_OPTIONS"));
+  ubsan_parser.ParseString(GetEnv("UBSAN_OPTIONS"), "UBSAN_OPTIONS");
 #endif
 
   InitializeCommonFlags();
 
   // TODO(eugenis): dump all flags at verbosity>=2?
   if (Verbosity()) ReportUnrecognizedFlags();
 
   if (common_flags()->help) {

lib/lsan/lsan.cc

static void InitializeFlags() {
 
   FlagParser parser;
   RegisterLsanFlags(&parser, f);
   RegisterCommonFlags(&parser);
 
   // Override from user-specified string.
   const char *lsan_default_options = MaybeCallLsanDefaultOptions();
   parser.ParseString(lsan_default_options);
-  parser.ParseString(GetEnv("LSAN_OPTIONS"));
+  parser.ParseString(GetEnv("LSAN_OPTIONS"), "LSAN_OPTIONS");
 
   SetVerbosity(common_flags()->verbosity);
 
   if (Verbosity()) ReportUnrecognizedFlags();
 
   if (common_flags()->help) parser.PrintFlagDescriptions();
 
   __sanitizer_set_report_path(common_flags()->log_path);

lib/sanitizer_common/sanitizer_flag_parser.h

class FlagParser {
 
   const char *buf_;
   uptr pos_;
 
  public:
   FlagParser();
   void RegisterHandler(const char *name, FlagHandlerBase *handler,
                        const char *desc);
-  void ParseString(const char *s);
+  void ParseString(const char *s, const char *env_option_name = 0);
   bool ParseFile(const char *path, bool ignore_missing);
   void PrintFlagDescriptions();
 
   static LowLevelAllocator Alloc;
 
  private:
   void fatal_error(const char *err);
   bool is_space(char c);
   void skip_whitespace();
-  void parse_flags();
-  void parse_flag();
+  void parse_flags(const char *env_option_name);
+  void parse_flag(const char *env_option_name);
   bool run_handler(const char *name, const char *value);
   char *ll_strndup(const char *s, uptr n);
 };
 
 template <typename T>
 static void RegisterFlag(FlagParser *parser, const char *name, const char *desc,
                          T *var) {
   FlagHandler<T> *fh = new (FlagParser::Alloc) FlagHandler<T>(var);  // NOLINT
   parser->RegisterHandler(name, fh, desc);
 }
 
 void ReportUnrecognizedFlags();
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_FLAG_REGISTRY_H

lib/sanitizer_common/sanitizer_flag_parser.cc

 
 void FlagParser::PrintFlagDescriptions() {
   Printf("Available flags for %s:\n", SanitizerToolName);
   for (int i = 0; i < n_flags_; ++i)
     Printf("\t%s\n\t\t- %s\n", flags_[i].name, flags_[i].desc);
 }
 
 void FlagParser::fatal_error(const char *err) {
-  Printf("ERROR: %s\n", err);
+  Printf("%s: ERROR: %s\n", SanitizerToolName, err);
   Die();
 }
 
 bool FlagParser::is_space(char c) {
   return c == ' ' || c == ',' || c == ':' || c == '\n' || c == '\t' ||
          c == '\r';
 }
 
 void FlagParser::skip_whitespace() {
   while (is_space(buf_[pos_])) ++pos_;
 }
 
-void FlagParser::parse_flag() {
+void FlagParser::parse_flag(const char *env_option_name) {
   uptr name_start = pos_;
   while (buf_[pos_] != 0 && buf_[pos_] != '=' && !is_space(buf_[pos_])) ++pos_;
-  if (buf_[pos_] != '=') fatal_error("expected '='");
+  if (buf_[pos_] != '=') {
+    if (env_option_name) {
+      Printf("%s: ERROR: expected '=' in environment variable %s\n",
+          SanitizerToolName, env_option_name);
+      Die();
+    } else
+      fatal_error("expected '='");
+  }
   char *name = ll_strndup(buf_ + name_start, pos_ - name_start);
 
   uptr value_start = ++pos_;
   char *value;
   if (buf_[pos_] == '\'' || buf_[pos_] == '"') {
     char quote = buf_[pos_++];
     while (buf_[pos_] != 0 && buf_[pos_] != quote) ++pos_;
     if (buf_[pos_] == 0) fatal_error("unterminated string");
     value = ll_strndup(buf_ + value_start + 1, pos_ - value_start - 1);
     ++pos_; // consume the closing quote
   } else {
     while (buf_[pos_] != 0 && !is_space(buf_[pos_])) ++pos_;
     if (buf_[pos_] != 0 && !is_space(buf_[pos_]))
       fatal_error("expected separator or eol");
     value = ll_strndup(buf_ + value_start, pos_ - value_start);
   }
 
   bool res = run_handler(name, value);
   if (!res) fatal_error("Flag parsing failed.");
 }
 
-void FlagParser::parse_flags() {
+void FlagParser::parse_flags(const char *env_option_name) {
   while (true) {
     skip_whitespace();
     if (buf_[pos_] == 0) break;
-    parse_flag();
+    parse_flag(env_option_name);
   }
 
   // Do a sanity check for certain flags.
   if (common_flags_dont_use.malloc_context_size < 1)
     common_flags_dont_use.malloc_context_size = 1;
 }
 
-void FlagParser::ParseString(const char *s) {
+void FlagParser::ParseString(const char *s, const char *env_option_name) {
   if (!s) return;
   // Backup current parser state to allow nested ParseString() calls.
   const char *old_buf_ = buf_;
   uptr old_pos_ = pos_;
   buf_ = s;
   pos_ = 0;
 
-  parse_flags();
+  parse_flags(env_option_name);
 
   buf_ = old_buf_;
   pos_ = old_pos_;
 }
 
 bool FlagParser::ParseFile(const char *path, bool ignore_missing) {
   static const uptr kMaxIncludeSize = 1 << 15;
   char *data;

lib/tsan/rtl/tsan_flags.h

 #define TSAN_FLAG(Type, Name, DefaultValue, Description) Type Name;
 #include "tsan_flags.inc"
 #undef TSAN_FLAG
 
   void SetDefaults();
   void ParseFromString(const char *str);
 };
 
-void InitializeFlags(Flags *flags, const char *env);
+void InitializeFlags(Flags *flags, const char *env,
+    const char *env_option_name);
 }  // namespace __tsan
 
 #endif  // TSAN_FLAGS_H

lib/tsan/rtl/tsan_flags.cc

 #include "tsan_flags.inc"
 #undef TSAN_FLAG
   // DDFlags
   RegisterFlag(parser, "second_deadlock_stack",
       "Report where each mutex is locked in deadlock reports",
       &f->second_deadlock_stack);
 }
 
-void InitializeFlags(Flags *f, const char *env) {
+void InitializeFlags(Flags *f, const char *env, const char *env_option_name) {
   SetCommonFlagsDefaults();
   {
     // Override some common flags defaults.
     CommonFlags cf;
     cf.CopyFrom(*common_flags());
     cf.allow_addr2line = true;
     if (SANITIZER_GO) {
       // Does not work as expected for Go: runtime handles SIGABRT and crashes.
#endif
 
   // Let a frontend override.
   parser.ParseString(__tsan_default_options());
 #if TSAN_CONTAINS_UBSAN
   const char *ubsan_default_options = __ubsan::MaybeCallUbsanDefaultOptions();
   ubsan_parser.ParseString(ubsan_default_options);
 #endif
   // Override from command line.
-  parser.ParseString(env);
+  parser.ParseString(env, env_option_name);
 #if TSAN_CONTAINS_UBSAN
-  ubsan_parser.ParseString(GetEnv("UBSAN_OPTIONS"));
+  ubsan_parser.ParseString(GetEnv("UBSAN_OPTIONS"), "UBSAN_OPTIONS");
 #endif
 
   // Sanity check.
   if (!f->report_bugs) {
     f->report_thread_leaks = false;
     f->report_destroy_locked = false;
     f->report_signal_unsafe = false;
   }

lib/tsan/rtl/tsan_rtl.cc

void Initialize(ThreadState *thr) {
   is_initialized = true;
   // We are not ready to handle interceptors yet.
   ScopedIgnoreInterceptors ignore;
   SanitizerToolName = "ThreadSanitizer";
   // Install tool-specific callbacks in sanitizer_common.
   SetCheckFailedCallback(TsanCheckFailed);
 
   ctx = new(ctx_placeholder) Context;
-  const char *options = GetEnv(SANITIZER_GO ? "GORACE" : "TSAN_OPTIONS");
+  const char *env_name = SANITIZER_GO ? "GORACE" : "TSAN_OPTIONS";
+  const char *options = GetEnv(env_name);
   CacheBinaryName();
   CheckASLR();
-  InitializeFlags(&ctx->flags, options);
+  InitializeFlags(&ctx->flags, options, env_name);
   AvoidCVE_2016_2143();
   __sanitizer::InitializePlatformEarly();
   __tsan::InitializePlatformEarly();
 
 #if !SANITIZER_GO
   // Re-exec ourselves if we need to set additional env or command line args.
   MaybeReexec();
 

lib/ubsan/ubsan_flags.cc

void InitializeFlags() {
 
   FlagParser parser;
   RegisterCommonFlags(&parser);
   RegisterUbsanFlags(&parser, f);
 
   // Override from user-specified string.
   parser.ParseString(MaybeCallUbsanDefaultOptions());
   // Override from environment variable.
-  parser.ParseString(GetFlag("UBSAN_OPTIONS"));
+  parser.ParseString(GetFlag("UBSAN_OPTIONS"), "UBSAN_OPTIONS");
   InitializeCommonFlags();
   if (Verbosity()) ReportUnrecognizedFlags();
 
   if (common_flags()->help) parser.PrintFlagDescriptions();
 }
 
 }  // namespace __ubsan
 
 SANITIZER_INTERFACE_WEAK_DEF(const char *, __ubsan_default_options, void) {
   return "";
 }
 
 #endif  // CAN_SANITIZE_UB

test/asan/TestCases/Linux/asan_options_error_message.cc

+// Check that with an invalid ASAN_OPTIONS, ASan reports user.
+
+// RUN: %clangxx_asan %s -o %t
+
+// Intentionally don't inherit the default ASAN_OPTIONS.
+// RUN: env ASAN_OPTIONS="bad_stuff" not %run %t 2>&1 | FileCheck %s
+
+int main() {
+  // CHECK: {{ERROR: expected '=' in environment variable ASAN_OPTIONS}}
+  return 0;
+}