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

[Evgenii Stepanov via Phabricator via llvm-commits]

eugenis 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);
+  }
----------------
yln wrote:
> delcypher wrote:
> > 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.
> +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.
> 
> 
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.



================
Comment at: compiler-rt/lib/sanitizer_common/sanitizer_flag_parser.h:178
+inline bool FlagHandler<s64>::CurrentValueAsString(char *buffer, uptr size) {
+  return false;
+}
----------------
delcypher wrote:
> @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?
If you look in sanitizer_printf.cpp, "ll" stands for s64 / u64, so "%lld".


Repository:
  rG LLVM Github Monorepo

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

https://reviews.llvm.org/D69546