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

[Dan Liew via Phabricator via llvm-commits]

delcypher marked 5 inline comments as done.
delcypher added inline comments.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cpp:69
+    else
+      Printf("\t%s\n\t\t- %s\n", flags_[i].name, flags_[i].desc);
+  }
----------------
eugenis wrote:
> How about printing a truncated value in this case, maybe replacing the last few characters with dots (...)? At least for string flags.
> 
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.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:108
+  int data = *t_;
+  uptr num_symbols_should_write = internal_snprintf(buffer, size, "%d", data);
+  // Not `<=` because `num_symbols_should_write` does not include null
----------------
eugenis wrote:
> default integer promotions apply to varargs
I didn't know that. I'll fix this.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:128
+    return false;
+  bool success = internal_memcpy(buffer, *t_, str_length);
+  CHECK(buffer[str_length - 1] == '\0');
----------------
eugenis wrote:
> all of this can be replaced with internal_strncpy
I'll fix this.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:178
+inline bool FlagHandler<s64>::CurrentValueAsString(char *buffer, uptr size) {
+  return false;
+}
----------------
@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?


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/options-help-current-values.cpp:2
+// RUN: %clangxx -O0 %s -o %t
+// RUN: %env_tool_opts=help=1 %run %t 2>&1 | FileCheck %s
+
----------------
eugenis wrote:
> Remove options-help.cpp test, this new one is strictly better.
Sure.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D69546/new/

https://reviews.llvm.org/D69546