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

[Julian Lettner via Phabricator via llvm-commits]

yln added a comment.

In D69546#1730860 <https://reviews.llvm.org/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 this 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



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.cpp:64
+        !(flags_[i].handler->CurrentValueAsString(buffer, sizeof(buffer)));
+    buffer[sizeof(buffer) - 1] = '\0';  // Ensure buffer is terminated.
+    const char *truncation_str = truncated ? " Truncated" : "";
----------------
Trust yourself! ;)


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:25
   virtual bool Parse(const char *value) { return false; }
+  virtual bool CurrentValueAsString(char *buffer, uptr size) {
+    buffer[0] = '\0';
----------------
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.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:44
+  // otherwise.
+  bool CurrentValueAsString(char *buffer, uptr size) final;
 };
----------------
Maybe we can have a more ergonomic method name? `Format` or `FormatInto` to act as a counterpart of `Parse`.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:76
+  internal_strncpy(buffer, bool_str, copy_count);
+  buffer[copy_count - 1] = '\0';
+  return bool_str_len < size;
----------------
Unnecessary when source is a `\0` terminated string and copy_count is correct.


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp:82
   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 {
----------------
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?


================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flags.cpp:101
+    internal_strncpy(buffer, original_path_, copy_count);
+    buffer[copy_count - 1] = '\0';
+    return str_len < size;
----------------
See above.


================
Comment at: compiler-rt/test/sanitizer_common/TestCases/options-help-include-current-value.cpp:1
+// RUN: %clangxx -O0 %s -o %t
+// RUN: %env_tool_opts=help=1,include_if_exists=some_ridculously_long_path_that_will_get_truncated_and_does_not_exist_on_file_system__________ %run %t 2>&1 | FileCheck %s
----------------
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69546