[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:64
+        !(flags_[i].handler->CurrentValueAsString(buffer, sizeof(buffer)));
+    buffer[sizeof(buffer) - 1] = '\0';  // Ensure buffer is terminated.
+    const char *truncation_str = truncated ? " Truncated" : "";
----------------
yln wrote:
> Trust yourself! ;)
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. 


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


================
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;
----------------
yln wrote:
> Unnecessary when source is a `\0` terminated string and copy_count is correct.
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.


================
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 {
----------------
yln wrote:
> 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?
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`.


================
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
----------------
yln wrote:
> 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.
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.


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69546