[sanitizer-common] Reduce ANSI color sequences that have no effect.

[aarongreen]

Summary

sanitizer_common's consumers use SanitizerCommonDecorator to color their output in various cases. In some cases they end up emitting several escape sequences in a row that don't end up having any effect, e.g. PrintShadowBytes will transition to bold+color, print a byte, transition to default, transition back to bold+color, etc. This extra transitions end up being passed to the RawWrite call in sanitizer_printf.cc. If the implementation of RawWrite has a fixed buffer size (e.g. as in sanitizer_fuchsia.cc), this can cause the buffer to be consumed unnecessary and even end up breaking the line in the middle of an escape sequence.

This change attempts to mitigate that problem by providing SanitizerCommonDecorator::Compact, which removes sequences that would simply be overwritten by subsequent sequences.

[phosek]

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc

23

Why not internal_strncmp(s, "\033[0", 3) != 0?

120

nit: space between for and (

149

Can you use braces for else for consistency?

compiler-rt/lib/sanitizer_common/tests/sanitizer_printf_test.cc

192

nit: separate with empty line from the class declaration

[aarongreen]

Addresses phosek's comments.

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc

23

There's a difference between '0' and '\0'.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.h

40

pleas clang-format

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.h

45

just move ansi_ check into ToStr()

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc

22

lets consume chars to simplify indexing

static const char* getCode(const char* s, char* code) {
    if (internal_strncmp(s, "\033[", 2) != 0 || s[2] == '\0' || s[3] != 'm') 
      return nullptr;
   *code = s[2];
    return s + 4;
}
SanitizerCommonDecorator::ToKind(const char *s) {
  char c;
  if (!(s = getCode(s, &c))
    return kUnknown;

  switch(c)
   ....

  if (!(s = getCode(s, &c))
    return kBold;

 switch(c) {
     ....

}

89

please remove SanitizerCommonDecorator::StrLen or replace with internal_strln( ToKind() )

107

z->out?

129

why do you need to check for "*s < 0x7f"

138

if (z != s) can be omitted

compiler-rt/lib/sanitizer_common/tests/sanitizer_printf_test.cc

196

ASNI -> ANSI

225

please use Value-Parameterized Test instead of the loop
https://github.com/google/googletest/blob/master/googletest/docs/advanced.md#specifying-names-for-value-parameterized-test-parameters

229

this test logic is quite fancy, it's going to be harder to debug when it detected a bug
I would prefer simple tests where easy to see from the code what was actual input and expected output:

SanitizerCommonDecorator::Compact(GetParam().input);
EXPECT_STREQ(GetParam().input, GetParam().expected);

[aarongreen]

Addressed vitalybuka's comments. The end result is a bit smaller and more elegant than before. I did end up needing to move some code for ColorizeReport (defined in sanitizer_common.h) in order to avoid causing problems in scudo's small-binary build.

[aarongreen]

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc

22

That doesn't quite work: the colors are 2 chars, i.e. 31.

I've made getCode use internal_simple_strtoll to return an s64 instead, and advance a const char **s_ptr. That way, each call consumes a '\033[...m' sequence and returns the number represented by '...'

I can't promise it's faster, but hopefully it's clearer?

129

0x7f is DEL, and above that isn't ascii. Printable chars should be 0x21 (!) to 0x7e (~).

compiler-rt/lib/sanitizer_common/tests/sanitizer_printf_test.cc

196

seplling is hrad.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common.cpp

99 ↗

(On Diff #215165)

INLINE is not necessary

can this be?

return SANITIZER_FUCHSIA || report_file.SupportsColors();

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cpp

58 ↗

(On Diff #215165)

should these be kDefault

59 ↗

(On Diff #215165)

DecorationKind prev = kUnknown;
DecorationKind next = kUnknown;

69 ↗

(On Diff #215165)

code uses "code = kUnknown;" to only break the loop
if we wrap this into function:

const char* getCode(const char* &in, DecorationKind &next) {
while (true) {
    if (internal_strncmp(in, "\033[", 2) != 0)
      return in;
    const char* endptr = in + 2;
    s64 code = internal_simple_strtoll(endptr, &endptr, 10);
    if (endptr <= in + 2 || *endptr++ != 'm')
       return in;

    switch (code) {
        case kDefault:
        case kBold:
          break;
        case kBlack:
        case kRed:
        case kGreen:
        case kYellow:
        case kBlue:
        case kMagenta:
        case kCyan:
        case kWhite:
          if (next == kBold)
            break;
        default:
           return in;
    }
    next = static_cast<DecorationKind>(code);
}
}

we don't need kUnknown at all

83 ↗

(On Diff #215165)

why next must be kBold?

91 ↗

(On Diff #215165)

increment "endptr" right after != 'm' check, so we know where 1 is from

99 ↗

(On Diff #215165)

why? // Not null-terminated!
ToStr returns regular c-strings

[vitalybuka]

LGTM as-is with few questions and suggestions

compiler-rt/lib/sanitizer_common/CMakeLists.txt

set(SANITIZER_SOURCES_NOTERMINATION
   sanitizer_platform_limits_solaris.cc
   sanitizer_posix.cc
   sanitizer_printf.cc
   sanitizer_procmaps_common.cc
   sanitizer_procmaps_bsd.cc
   sanitizer_procmaps_linux.cc
   sanitizer_procmaps_mac.cc
   sanitizer_procmaps_solaris.cc
+  sanitizer_report_decorator.cc
   sanitizer_rtems.cc
   sanitizer_solaris.cc
   sanitizer_stoptheworld_mac.cc
   sanitizer_suppressions.cc
   sanitizer_tls_get_addr.cc
   sanitizer_thread_registry.cc
   sanitizer_type_traits.cc
   sanitizer_win.cc

compiler-rt/lib/sanitizer_common/sanitizer_printf.cc

 // Internal printf function, used inside run-time libraries.
 // We can't use libc printf because we intercept some of the functions used
 // inside it.
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_common.h"
 #include "sanitizer_flags.h"
 #include "sanitizer_libc.h"
+#include "sanitizer_report_decorator.h"
 
 #include <stdio.h>
 #include <stdarg.h>
 
 #if SANITIZER_WINDOWS && defined(_MSC_VER) && _MSC_VER < 1800 &&               \
       !defined(va_copy)
 # define va_copy(dst, src) ((dst) = (src))
 #endif
#   define CHECK_NEEDED_LENGTH \
     }
     needed_length += VSNPrintf(buffer + needed_length,
                                buffer_size - needed_length, format, args);
     CHECK_NEEDED_LENGTH
     // If the message fit into the buffer, print it and exit.
     break;
 #   undef CHECK_NEEDED_LENGTH
   }
+  SanitizerCommonDecorator::Compact(buffer);
   RawWrite(buffer);
 
   // Remove color sequences from the message.
   RemoveANSIEscapeSequencesFromString(buffer);
   CallPrintfAndReportCallback(buffer);
   LogMessageOnPrintf(buffer);
 
   // If we had mapped any memory, clean up.

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.h

 #include "sanitizer_common.h"
 
 namespace __sanitizer {
 class SanitizerCommonDecorator {
   // FIXME: This is not portable. It assumes the special strings are printed to
   // stdout, which is not the case on Windows (see SetConsoleTextAttribute()).
  public:
   SanitizerCommonDecorator() : ansi_(ColorizeReports()) {}
-  const char *Bold() const { return ansi_ ? "\033[1m" : ""; }
-  const char *Default() const { return ansi_ ? "\033[1m\033[0m"  : ""; }
+  const char *Bold() const { return ansi_ ? ToStr(kBold) : ""; }
+  const char *Default() const { return ansi_ ? ToStr(kDefault) : ""; }
   const char *Warning() const { return Red(); }
   const char *Error() const { return Red(); }
   const char *MemoryByte() const { return Magenta(); }
 
+  // Coalesce sequences of special strings that would overwrite each other.
+  // The string is modified in-place.
+  static void Compact(char *str);
+
  protected:
-  const char *Black()   const { return ansi_ ? "\033[1m\033[30m" : ""; }
-  const char *Red()     const { return ansi_ ? "\033[1m\033[31m" : ""; }
-  const char *Green()   const { return ansi_ ? "\033[1m\033[32m" : ""; }
-  const char *Yellow()  const { return ansi_ ? "\033[1m\033[33m" : ""; }
-  const char *Blue()    const { return ansi_ ? "\033[1m\033[34m" : ""; }
-  const char *Magenta() const { return ansi_ ? "\033[1m\033[35m" : ""; }
-  const char *Cyan()    const { return ansi_ ? "\033[1m\033[36m" : ""; }
-  const char *White()   const { return ansi_ ? "\033[1m\033[37m" : ""; }
+  const char* Black()   const { return ansi_ ? ToStr(kBlack)   : ""; }
+  const char* Red()     const { return ansi_ ? ToStr(kRed)     : ""; }
+  const char* Green()   const { return ansi_ ? ToStr(kGreen)   : ""; }
+  const char* Yellow()  const { return ansi_ ? ToStr(kYellow)  : ""; }
+  const char* Blue()    const { return ansi_ ? ToStr(kBlue)    : ""; }
+  const char* Magenta() const { return ansi_ ? ToStr(kMagenta) : ""; }
+  const char* Cyan()    const { return ansi_ ? ToStr(kCyan)    : ""; }
+  const char* White()   const { return ansi_ ? ToStr(kWhite)   : ""; }
+
  private:
-  bool ansi_;
+  // The currently supported ANSI tags.
+  enum DecorationKind {
+    kUnknown,
+    kDefault,
+    kBold,
+    kBlack,
+    kRed,
+    kGreen,
+    kYellow,
+    kBlue,
+    kMagenta,
+    kCyan,
+    kWhite,
+  };
+  static DecorationKind ToKind(const char *str);
+  static const char *ToStr(DecorationKind kind);
+  static uptr StrLen(DecorationKind kind);
+
+  const bool ansi_;
 };
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_REPORT_DECORATOR_H

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc

+//===-- sanitizer_report_decorator.cc ---------------------------*- C++ -*-===//
+//
+// Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
+// See https://llvm.org/LICENSE.txt for license information.
+// SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
+//
+//===----------------------------------------------------------------------===//
+//
+// This file is shared between AddressSanitizer and ThreadSanitizer.
+//
+// Methods to manipulate supported ANSI tags.
+//
+//===----------------------------------------------------------------------===//
+
+#include "sanitizer_libc.h"
+#include "sanitizer_report_decorator.h"
+
+namespace __sanitizer {
+
+SanitizerCommonDecorator::DecorationKind
+SanitizerCommonDecorator::ToKind(const char *s) {
+  if (internal_strncmp(s, "\033[", 2) != 0 || s[2] == '\0' || s[3] != 'm')
+    return kUnknown;
+  switch (s[2]) {
+  case '0':
+    return kDefault;
+  case '1':
+    break;
+  default:
+    return kUnknown;
+  }
+  if (internal_strncmp(&s[4], "\033[3", 3) != 0 || s[7] == '\0' || s[8] != 'm')
+    return kBold;
+  switch(s[7]) {
+  case '0':
+    return kBlack;
+  case '1':
+    return kRed;
+  case '2':
+    return kGreen;
+  case '3':
+    return kYellow;
+  case '4':
+    return kBlue;
+  case '5':
+    return kMagenta;
+  case '6':
+    return kCyan;
+  case '7':
+    return kWhite;
+  default:
+    return kUnknown;
+  }
+}
+
+const char *SanitizerCommonDecorator::ToStr(DecorationKind kind) {
+  switch(kind) {
+  case kDefault:
+    return "\033[0m";
+  case kBold:
+    return "\033[1m";
+  case kBlack:
+    return "\033[1m\033[30m";
+  case kRed:
+    return "\033[1m\033[31m";
+  case kGreen:
+    return "\033[1m\033[32m";
+  case kYellow:
+    return "\033[1m\033[33m";
+  case kBlue:
+    return "\033[1m\033[34m";
+  case kMagenta:
+    return "\033[1m\033[35m";
+  case kCyan:
+    return "\033[1m\033[36m";
+  case kWhite:
+    return "\033[1m\033[37m";
+  default:
+    return "";
+  }
+}
+
+uptr SanitizerCommonDecorator::StrLen(DecorationKind kind) {
+  switch(kind) {
+  case kDefault:
+  case kBold:
+    return 4;
+  case kBlack:
+  case kRed:
+  case kGreen:
+  case kYellow:
+  case kBlue:
+  case kMagenta:
+  case kCyan:
+  case kWhite:
+    return 9;
+  default:
+    return 0;
+  }
+}
+
+void SanitizerCommonDecorator::Compact(char* str) {
+    if (!str)
+        return;
+    char* s = str;
+    char* z = str;
+
+    // We need to keep track of whatever decoration tag we last printed with, as
+    // well as whatever one is currently-active and where the string to print it
+    // is located in the buffer.
+    DecorationKind prev = kUnknown;
+    DecorationKind next = kUnknown;
+    uptr n = 0;
+
+    while (true) {
+        // Consume recognized tags in a loop. Tags are not immediately-printed,
+        // and each successive one replaces the previous, so that only-the last
+        // in a sequence is active.
+        for (auto kind = ToKind(s); kind != kUnknown; kind = ToKind(s)) {
+            next = kind;
+            s += SanitizerCommonDecorator::StrLen(kind);
+        }
+
+        if (*s == '\0')
+          break;
+
+        // The active tag(s) has changed since the last printable,
+        // non-whitespace character. Print the tag(s) before the character.
+        if (0x20 < *s && *s < 0x7f && prev != next) {
+            n = SanitizerCommonDecorator::StrLen(next);
+            internal_memcpy(z, ToStr(next), n);
+            prev = next;
+            z += n;
+            continue;
+        }
+
+        // We have actual characters; print them.
+        if (z != s)
+            *z = *s;
+        z++;
+        s++;
+    }
+    // Make sure we leave the output stream in the right state before
+    // terminating the string.
+    if (prev != next) {
+      n = SanitizerCommonDecorator::StrLen(next);
+      internal_strncpy(z, ToStr(next), n + 1);
+    } else {
+      *z = '\0';
+    }
+}
+
+}  // namespace __sanitizer

compiler-rt/lib/sanitizer_common/tests/sanitizer_printf_test.cc

TEST(Printf, Precision) {
   EXPECT_STREQ("12345 ", buf);
   // Check that width does not overflow the smaller buffer, although
   // 10 chars is requested, it stops at the buffer size, 8.
   len = internal_snprintf(buf, 8, "%-10s", "12345");
   EXPECT_EQ(10U, len);  // The required size reported.
   EXPECT_STREQ("12345  ", buf);
 }
 
+// The array below is used to test that Printf correctly collapses unneeded
+// decorations. Each element is a pair of strings that represent equivalent
+// decorated strings. The strings can be interpreted as follows:
+//   'R' and 'B':   Print the ANSI escape sequences for "Red" and "Blue"
+//   'N', 'S', 'T': Print newline, space, and tab, respectively.
+//   'x', 'y':      Print the character as-is.
+//   '-', others:   Ignore.
+struct DecorationTest {
+  char verbose[10];
+  char compact[7];
+} tests[] = {
+  {"S-xRRBy-", "S-xBy-"},
+  {"RSxRRBy-", "SRxBy-"},
+  {"BxSRRBy-", "BxS-y-"},
+  {"RxRSRByR", "RxSByR"},
+  {"BxRRSByR", "BxS-yR"},
+  {"RxRRBSyB", "RxSBy-"},
+  {"BxRRBySB", "Bx-yS-"},
+  {"-xRBRy-S", "-xRyS-"},
+  {"NRxRBRy-", "NRx-y-"},
+  {"BNxRBRy-", "NBxRy-"},
+  {"RxNRBRyR", "RxN-y-"},
+  {"BxRNBRyR", "BxNRy-"},
+  {"RxRBNRyB", "RxN-yB"},
+  {"BxRBRNyB", "BxNRyB"},
+  {"-xBRRyN-", "-xRyN-"},
+  {"RxBRRy-N", "Rx-yN-"},
+  {"TBxBRRy-", "TBxRy-"},
+  {"RTxBRRyR", "TRx-y-"},
+  {"BxTBRRyR", "BxTRy-"},
+  {"RxBTRRyB", "RxT-yB"},
+  {"BxBRTRyB", "BxTRyB"},
+};
+
+class TestDecorator : public SanitizerCommonDecorator {
+ public:
+  // Returns an ASNI escape sequence for 'R' or 'B' as described above.
+  const char* ToStr(char c) {
+    switch (c) {
+    case 'R':
+      return SanitizerCommonDecorator::Red();
+    case 'B':
+      return SanitizerCommonDecorator::Blue();
+    case 'N':
+      return "\n";
+    case 'S':
+      return " ";
+    case 'T':
+      return "\t";
+    case 'x':
+      return "x";
+    case 'y':
+      return "y";
+    case '-':
+    default:
+      return "";
+    }
+  }
+};
+
+TEST(Printf, Decorations) {
+  TestDecorator d;
+  char verbose[64];
+  char compact[64];
+  int n;
+  for (size_t i = 0; i < sizeof(tests) / sizeof(tests[0]); ++i) {
+    const char* v = tests[i].verbose;
+    size_t off = 0;
+    for (size_t j=0; j < strlen(v); j++) {
+      n = snprintf(verbose + off, sizeof(verbose) - off, "%s", d.ToStr(v[j]));
+      ASSERT_GE(n, 0);
+      off += static_cast<size_t>(n);
+      ASSERT_LT(off, sizeof(verbose));
+    }
+    SanitizerCommonDecorator::Compact(verbose);
+    const char* c = tests[i].compact;
+    off = 0;
+    for (size_t j=0; j < strlen(c); j++) {
+      n = snprintf(compact + off, sizeof(compact) - off, "%s", d.ToStr(c[j]));
+      ASSERT_GE(n, 0);
+      off += static_cast<size_t>(n);
+      ASSERT_LT(off, sizeof(compact));
+    }
+    EXPECT_STREQ(verbose, compact);
+  }
+}
+
 }  // namespace __sanitizer