[SanitizerCommon] Print the current value of options when printing out help.

[delcypher]

Summary

Previously it wasn't obvious what the default value of various sanitizer
options were. A very close approximation of the "default values" for the
options are the current value of the options at the time of printing the
help output.

In the case that no other options are provided then the current values
are the default values (apart from help).

ASAN_OPTIONS=help=1 ./program

This patch causes the current option values to be printed when the
help output is enabled. The original intention for this patch was to append
(Default: <value>) to an option's help text. However because this
is technically wrong (and misleading) I've opted to append
(Current Value: <value>) instead.

When trying to implement a way of displaying the default value of the
options I tried another solution where the default value used in *.inc files
were used to create compile time strings that where used when printing
the help output. This solution was not satisfactory for several reasons:

• Stringifying the default values with the preprocessor did not work very

well in several cases. Some options contain boolean operators which no
amount of macro expansion can get rid of.

• It was much more invasive than this patch. Every sanitizer had to be changed.
• The settings of __<sanitizer>_default_options() are ignored.

For those reasons I opted for the solution in this patch.

rdar://problem/42567204

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cpp

70

How about printing a truncated value in this case, maybe replacing the last few characters with dots (...)? At least for string flags.

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

89

const char *str = *t_ ? "true" : "false;
uptr copy_count = internal_strnlen(str, size);

104

default integer promotions apply to varargs

118

all of this can be replaced with internal_strncpy

compiler-rt/test/sanitizer_common/TestCases/options-help-current-values.cpp

2 ↗

(On Diff #226832)

Remove options-help.cpp test, this new one is strictly better.

[delcypher]

Note there is a way to modify this patch so that it does actually show the defaults in the .inc files. This would require adding storage for the default values and assigning to it in CommonFlags::SetDefaults() (and all other similar sanitizer flag parsers). Then the CurrentValueAsString() functions could be renamed to DefaultValueAsString() and could be modified to read the default value rather than the current value.

This would be more invasive than this patch and adds additional size to the runtime library, just so we can print out help=1 in a nicer way which seems wasteful. If this is something that reviewers think we should do then I would suggest doing it as a follow up patch to try to keep this change small.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cpp

70

In which case? Currently success being false is meant to indicate any type of failure (e.g. the current value is a nullptr which we can't print). A failure doesn't imply that truncation happened. Currently I interpret failure to mean that the buffer should not be read from because it's not in a defined state.

We could change the semantics to be that failure means truncation if that's what you want. At this point in the code though we don't know what the type of the flag is though so we can't make a string vs not string distinction.

I'd prefer to keep the current semantics of failure and just change FlagHandler<const char *>::CurrentValueAsString(...) to replace the last few characters with ... in the case of truncation.

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

104

I didn't know that. I'll fix this.

118

I'll fix this.

156

@eugenis I'd like to add an implementation here but I'm unsure what format string to use for the sanitizer's internal implementation of snprintf. Normally I'd use the PRId64 macro from inttypes.h but we probably can't rely on that header in sanitizer_common. Do we have an equivalent or a known format string that means "signed 64-bit integer" for all platforms?

compiler-rt/test/sanitizer_common/TestCases/options-help-current-values.cpp

2 ↗

(On Diff #226832)

Sure.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

89

That's a lot more concise. I'll go with that.

[yln]

Thanks for this patch, I am pretty sure I will find this useful! :)

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cpp

70

+1 for truncation. I think we can always print a value and never fail.

Completely unnecessary, but for bonus points we could do the following.

With a buffer[n+4]: <n/2 characters>...<n/2 characters>:
/super/long/path/to/something -> /super/lo...mething

Or even smarter, with some heuristic for path components:
/super/long/path/to/something -> /super/.../to/something

This would be just "nice to have". I am fine with just truncating at the end.

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

25

Parsing can fail, but formatting a correct (truncated) value into a buffer should always be possible. So I think the return value of this can be void.

89

+1

compiler-rt/test/sanitizer_common/TestCases/options-help-current-values.cpp

4 ↗

(On Diff #226832)

Can you extend the test to show that we display the current value, i.e, in separate RUN and CHECK lines provide some option and assert that the printed value is different from the default.

[eugenis]

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cpp

70

I agree with all of the above. It only makes sense to do this for string arguments, and smart truncation is nice to have but not necessary.

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

156

If you look in sanitizer_printf.cpp, "ll" stands for s64 / u64, so "%lld".

[delcypher]

• CurrentValueAsString return code now indicates truncation
• Print "Truncated" if printed current value was truncated.
• Clean up CurrentValueAsString implementations.
• Add Support in FlagHandlerInclude for showing current value (not quite right but good enough).

[delcypher]

@yln @eugenis Can we do another round of review? I made some slightly different design choices to what was previously discussed. Here's my reasoning:

• ... is not appended to truncated strings. I thought that might be ambiguous (Is ... part of the string or is the string truncated?). So instead every time an option gets truncated it prints (Current Value Truncated: <value>) rather than (Current Value: <value>) which is not ambiguous.
• Due to the above CurrentValueAsString keeps its boolean return value but now returning false means truncating occurred.
• Due to implementing the above I discovered I hadn't implemented support in FlagHandlerInclude. This is now done.

[yln]

In D69546#1730860, @delcypher wrote:

I thought that might be ambiguous (Is ... part of the string or is the string truncated?).

Do you anticipate this to be an issue? My feeling is that truncation would only happen with file paths?! I have a slight preference towards the other because it seems simpler (simple, not easy), but I am not blocking on this.

• Due to implementing the above I discovered I hadn't implemented support in FlagHandlerInclude. This is now done.

There is another one: FlagHandlerKeepGoing

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cpp

65

Trust yourself! ;)

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

25

I think this should really be an abstract method to force subclasses to implement it. Or if we can't make it abstract/pure virtual because compiler-rt is too special, then "not implemented". Same thing holds for Parse, I guess.

54

Maybe we can have a more ergonomic method name? Format or FormatInto to act as a counterpart of Parse.

83

Unnecessary when source is a \0 terminated string and copy_count is correct.

compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp

82

If I understand correctly, this is a special FlagHandler, that reads a file and sets multiple other options based on the contents of the file.

The question is: does it make sense to show the file path as the current value?
I would assume that as a result of this parser, that other flags current value are updated.
If things get complicated here, I think it would be good enough for now to show nothing or some string "<options from file>".

Can we add a test for this parser type?

101

See above.

compiler-rt/test/sanitizer_common/TestCases/options-help-include-current-value.cpp

1 ↗

(On Diff #227530)

This test checks the truncation, but the file name is "options-help-include-current-value". I think we can move the truncation test into the the "options-help" test file.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cpp

65

This is some defensive programming in case any of the implementations leave the buffer unterminated which would cause Printf(...) to read beyond the buffer.

I don't think the performance cost of doing this is significant so I'd rather keep it unless we have a good reason to remove it.

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

54

I guess that's a bit clearer because the method name has the verb`Format` is a verb here. So sure let's make that change.

83

That is not true. If *t_ is a string that has size or more printable characters then internal_strncpy will just copy over the size characters without adding a null terminator.

Given how error prone this is perhaps we should use internal_strlcpy(...) instead? It seems to handle this case.

compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp

82

I think it makes sense to show the path to where the options were read from. I would prefer if we showed the versions where the substitutions were expanded but I refrained from doing that here because that would require us to not free the mmaped memory.

The test is options-help-include-current-value.cpp.

compiler-rt/test/sanitizer_common/TestCases/options-help-include-current-value.cpp

1 ↗

(On Diff #227530)

I'm not a fan of bundling checking of lots of different things into the same test. When the test fails it can make it much harder to understand exactly what is failing.

However. Given that I've bundled the other checks I guess we could move this into the options-help.cpp too.

[delcypher]

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

25

I'm just copying the style of code that was there before. I'm not against what you suggest but I'd rather fix this in a follow-up patch.

[delcypher]

• Rename CurrentValueAsString to Format.
• Implement support for FlagHandlerKeepGoing.
• Use Mmap to allocate memory for string representation of option value.
• Switch to using internal_strlcpy instead of internal_strncpy.
• Remove separate test for include_if_exists

[delcypher]

• Change path name in test.

[delcypher]

@yln @eugenis This ready for another round of review.

[yln]

In D69546#1731024, @yln wrote:

In D69546#1730860, @delcypher wrote:

I thought that might be ambiguous (Is ... part of the string or is the string truncated?).

Do you anticipate this to be an issue? My feeling is that truncation would only happen with file paths?! I have a slight preference towards the other because it seems simpler (simple, not easy), but I am not blocking on this.

^ Please comment, thanks.

compiler-rt/lib/msan/msan.cpp

126

// keep_going is an old name for halt_on_error, and it has inverse meaning.

:(

Maybe flip operands and remove negation to make it easier to follow.

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

112–115

const char *str = *t_ ? *t_ : "(nullptr)";

More consistent with the other implementations.

compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp

82

Makes sense.

compiler-rt/test/sanitizer_common/TestCases/options-help.cpp

7

I think we dropped the test for truncation?

[delcypher]

compiler-rt/test/sanitizer_common/TestCases/options-help.cpp

7

Yes. I've made the buffer much larger which makes testing truncation a little trickier so I just dropped it. I could add it back but I think I'll have to set the option via code rather than an environment variable because I don't think windows likes long command lines.

[delcypher]

• Add trunation test.
• Style clean up in FlagHandler<const char *>::Format(char *buffer, uptr size).
• Style clean up in FlagHandlerKeepGoing.

[delcypher]

In D69546#1731024, @yln wrote:

In D69546#1730860, @delcypher wrote:

I thought that might be ambiguous (Is ... part of the string or is the string truncated?).

Do you anticipate this to be an issue? My feeling is that truncation would only happen with file paths?! I have a slight preference towards the other because it seems simpler (simple, not easy), but I am not blocking on this.

Well visually ... is very similar to .. which actually has a meaning in paths. In the current implementation truncation is very unlikely to happen because we allocate a much larger buffer to store strings representing the current value.

That said. I'm pretty happy with the implementation now. Only in extreme cases will truncation happen and we handle it is a way that is unambiguous and hopefully robust.

[delcypher]

@yln @eugenis Good to go?

[delcypher]

• Go back to a small buffer for printing values. Mmap is probably overkill here.
• Remove defensive check.

[delcypher]

Put in a proper defensive check rather than "defensive programming" so that we crash if someone screws up.

[yln]

compiler-rt/lib/msan/msan.cpp

126

Does this compile? halt_on_error seems to be missing _.

[delcypher]

compiler-rt/lib/msan/msan.cpp

126

Good catch. MSan doesn't build on darwin so I missed this.

[delcypher]

• Fix MSan compilation error.

[delcypher]

@yln ping.

[delcypher]

• Clean up test case to not use compiled in default options.
• Adopt a few testing tweaks suggested by @yln offline.
• Refactor string printing code into helper method to reduce code duplication.

[delcypher]

• Drop some comments. There are comments on internal_snprintf that explain the same thing.
• Use internal_snprintf in FormatString. It handles the nullptr case already.

compiler-rt/lib/msan/msan.cpp

explicit FlagHandlerKeepGoing(bool *halt_on_error)
       : halt_on_error_(halt_on_error) {}
   bool Parse(const char *value) final {
     bool tmp;
     FlagHandler<bool> h(&tmp);
     if (!h.Parse(value)) return false;
     *halt_on_error_ = !tmp;
     return true;
   }
+  bool Format(char *buffer, uptr size) final {
+    const char *keep_going_str = (*halt_on_error_) ? "false" : "true";
+    return FormatString(buffer, size, keep_going_str);
+  }
 };
 
 static void RegisterMsanFlags(FlagParser *parser, Flags *f) {
 #define MSAN_FLAG(Type, Name, DefaultValue, Description) \
   RegisterFlag(parser, #Name, Description, &f->Name);
 #include "msan_flags.inc"
 #undef MSAN_FLAG
 

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h

 #include "sanitizer_libc.h"
 #include "sanitizer_common.h"
 
 namespace __sanitizer {
 
 class FlagHandlerBase {
  public:
   virtual bool Parse(const char *value) { return false; }
+  // Write the C string representation of the current value (truncated to fit)
+  // into the buffer of size `size`. Returns false if truncation occurred and
+  // returns true otherwise.
+  virtual bool Format(char *buffer, uptr size) {
+    if (size > 0)
+      buffer[0] = '\0';
+    return false;
+  }
 
  protected:
   ~FlagHandlerBase() {}
+
+  inline bool FormatString(char *buffer, uptr size, const char *str_to_use) {
+    uptr num_symbols_should_write =
+        internal_snprintf(buffer, size, "%s", str_to_use);
+    return num_symbols_should_write < size;
+  }
 };
 
 template <typename T>
 class FlagHandler : public FlagHandlerBase {
   T *t_;
 
  public:
   explicit FlagHandler(T *t) : t_(t) {}
   bool Parse(const char *value) final;
+  bool Format(char *buffer, uptr size) final;
 };
 
 inline bool ParseBool(const char *value, bool *b) {
   if (internal_strcmp(value, "0") == 0 ||
       internal_strcmp(value, "no") == 0 ||
       internal_strcmp(value, "false") == 0) {
     *b = false;
     return true;
   }
   if (internal_strcmp(value, "1") == 0 ||
       internal_strcmp(value, "yes") == 0 ||
       internal_strcmp(value, "true") == 0) {
     *b = true;
     return true;
   }
   return false;
 }
 
 template <>
 inline bool FlagHandler<bool>::Parse(const char *value) {
   if (ParseBool(value, t_)) return true;
   Printf("ERROR: Invalid value for bool option: '%s'\n", value);
   return false;
 }
 
 template <>
+inline bool FlagHandler<bool>::Format(char *buffer, uptr size) {
+  return FormatString(buffer, size, *t_ ? "true" : "false");
+}
+
+template <>
 inline bool FlagHandler<HandleSignalMode>::Parse(const char *value) {
   bool b;
   if (ParseBool(value, &b)) {
     *t_ = b ? kHandleSignalYes : kHandleSignalNo;
     return true;
   }
   if (internal_strcmp(value, "2") == 0 ||
       internal_strcmp(value, "exclusive") == 0) {
     *t_ = kHandleSignalExclusive;
     return true;
   }
   Printf("ERROR: Invalid value for signal handler option: '%s'\n", value);
   return false;
 }
 
 template <>
+inline bool FlagHandler<HandleSignalMode>::Format(char *buffer, uptr size) {
+  uptr num_symbols_should_write = internal_snprintf(buffer, size, "%d", *t_);
+  return num_symbols_should_write < size;
+}
+
+template <>
 inline bool FlagHandler<const char *>::Parse(const char *value) {
   *t_ = value;
   return true;
 }
 
 template <>
+inline bool FlagHandler<const char *>::Format(char *buffer, uptr size) {
+  return FormatString(buffer, size, *t_);
+}
+
+template <>
 inline bool FlagHandler<int>::Parse(const char *value) {
   const char *value_end;
   *t_ = internal_simple_strtoll(value, &value_end, 10);
   bool ok = *value_end == 0;
   if (!ok) Printf("ERROR: Invalid value for int option: '%s'\n", value);
   return ok;
 }
 
 template <>
+inline bool FlagHandler<int>::Format(char *buffer, uptr size) {
+  uptr num_symbols_should_write = internal_snprintf(buffer, size, "%d", *t_);
+  return num_symbols_should_write < size;
+}
+
+template <>
 inline bool FlagHandler<uptr>::Parse(const char *value) {
   const char *value_end;
   *t_ = internal_simple_strtoll(value, &value_end, 10);
   bool ok = *value_end == 0;
   if (!ok) Printf("ERROR: Invalid value for uptr option: '%s'\n", value);
   return ok;
 }
 
 template <>
+inline bool FlagHandler<uptr>::Format(char *buffer, uptr size) {
+  uptr num_symbols_should_write = internal_snprintf(buffer, size, "%p", *t_);
+  return num_symbols_should_write < size;
+}
+
+template <>
 inline bool FlagHandler<s64>::Parse(const char *value) {
   const char *value_end;
   *t_ = internal_simple_strtoll(value, &value_end, 10);
   bool ok = *value_end == 0;
   if (!ok) Printf("ERROR: Invalid value for s64 option: '%s'\n", value);
   return ok;
 }
 
+template <>
+inline bool FlagHandler<s64>::Format(char *buffer, uptr size) {
+  uptr num_symbols_should_write = internal_snprintf(buffer, size, "%lld", *t_);
+  return num_symbols_should_write < size;
+}
+
 class FlagParser {
   static const int kMaxFlags = 200;
   struct Flag {
     const char *name;
     const char *desc;
     FlagHandlerBase *handler;
   } *flags_;
   int n_flags_;

compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cpp

char *FlagParser::ll_strndup(const char *s, uptr n) {
   uptr len = internal_strnlen(s, n);
   char *s2 = (char*)Alloc.Allocate(len + 1);
   internal_memcpy(s2, s, len);
   s2[len] = 0;
   return s2;
 }
 
 void FlagParser::PrintFlagDescriptions() {
+  char buffer[128];
+  buffer[sizeof(buffer) - 1] = '\0';
   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);
+  for (int i = 0; i < n_flags_; ++i) {
+    bool truncated = !(flags_[i].handler->Format(buffer, sizeof(buffer)));
+    CHECK_EQ(buffer[sizeof(buffer) - 1], '\0');
+    const char *truncation_str = truncated ? " Truncated" : "";
+    Printf("\t%s\n\t\t- %s (Current Value%s: %s)\n", flags_[i].name,
+           flags_[i].desc, truncation_str, buffer);
+  }
 }
 
 void FlagParser::fatal_error(const char *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';

compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp

void SubstituteForFlagValue(const char *s, char *out, uptr out_size) {
   }
   CHECK(out < out_end - 1);
   *out = '\0';
 }
 
 class FlagHandlerInclude : public FlagHandlerBase {
   FlagParser *parser_;
   bool ignore_missing_;
+  const char *original_path_;
 
  public:
   explicit FlagHandlerInclude(FlagParser *parser, bool ignore_missing)
-      : parser_(parser), ignore_missing_(ignore_missing) {}
+      : parser_(parser), ignore_missing_(ignore_missing), original_path_("") {}
   bool Parse(const char *value) final {
+    original_path_ = value;
     if (internal_strchr(value, '%')) {
       char *buf = (char *)MmapOrDie(kMaxPathLength, "FlagHandlerInclude");
       SubstituteForFlagValue(value, buf, kMaxPathLength);
       bool res = parser_->ParseFile(buf, ignore_missing_);
       UnmapOrDie(buf, kMaxPathLength);
       return res;
     }
     return parser_->ParseFile(value, ignore_missing_);
   }
+  bool Format(char *buffer, uptr size) {
+    // Note `original_path_` isn't actually what's parsed due to `%`
+    // substitutions. Printing the substituted path would require holding onto
+    // mmap'ed memory.
+    return FormatString(buffer, size, original_path_);
+  }
 };
 
 void RegisterIncludeFlags(FlagParser *parser, CommonFlags *cf) {
   FlagHandlerInclude *fh_include = new (FlagParser::Alloc)
       FlagHandlerInclude(parser, /*ignore_missing*/ false);
   parser->RegisterHandler("include", fh_include,
                           "read more options from the given file");
   FlagHandlerInclude *fh_include_if_exists = new (FlagParser::Alloc)
       FlagHandlerInclude(parser, /*ignore_missing*/ true);
   parser->RegisterHandler(

compiler-rt/test/sanitizer_common/TestCases/options-help.cpp

 // RUN: %clangxx -O0 %s -o %t
-// RUN: %env_tool_opts=help=1 %run %t 2>&1 | FileCheck %s
+// RUN: %env_tool_opts=help=1,include_if_exists=___some_path_that_does_not_exist___  %run %t 2>&1 | FileCheck %s
+// RUN: %env_tool_opts=help=1,symbolize=0 %run %t 2>&1 | FileCheck --check-prefix=CHECK-CV %s
+// RUN: %env_tool_opts=help=1,sancov_path=/long/path/that/requires/truncation/aaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaaB \
+// RUN:   %run %t 2>&1 | FileCheck --check-prefix=CHECK-TRUNCATION %s
 
 int main() {
 }
 
 // CHECK: Available flags for {{.*}}Sanitizer:
-// CHECK: handle_segv
+
+//
+// Bool option
+// CHECK: {{^[ \t]+symbolize$}}
+// CHECK-NEXT: (Current Value: true)
+//
+// String option
+// CHECK: {{^[ \t]+log_path$}}
+// CHECK-NEXT: (Current Value: {{.+}})
+//
+// int option
+// CHECK: {{^[ \t]+verbosity$}}
+// CHECK-NEXT: (Current Value: {{-?[0-9]+}})
+//
+// HandleSignalMode option
+// CHECK: {{^[ \t]+handle_segv$}}
+// CHECK-NEXT: (Current Value: {{0|1|2}})
+//
+// uptr option
+// CHECK: {{^[ \t]+mmap_limit_mb$}}
+// CHECK-NEXT: (Current Value: 0x{{[0-9a-fA-F]+}})
+//
+// FlagHandlerInclude option
+// CHECK: include_if_exists
+// CHECK-NEXT: (Current Value: ___some_path_that_does_not_exist___)
+
+// Test we show the current value and not the default.
+// CHECK-CV: {{^[ \t]+symbolize$}}
+// CHECK-CV-NEXT: (Current Value: false)
+
+// Test truncation of long paths.
+// CHECK-TRUNCATION: sancov_path
+// CHECK-TRUNCATION-NEXT: (Current Value Truncated: /long/path/that/requires/truncation/aaa{{a+}})