[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

22 ↗

(On Diff #204195)

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

119 ↗

(On Diff #204195)

nit: space between for and (

148 ↗

(On Diff #204195)

Can you use braces for else for consistency?

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

192 ↗

(On Diff #204195)

nit: separate with empty line from the class declaration

[aarongreen]

Addresses phosek's comments.

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cc

22 ↗

(On Diff #204195)

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 ↗

(On Diff #212462)

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 ↗

(On Diff #212462)

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

107 ↗

(On Diff #212462)

z->out?

129 ↗

(On Diff #212462)

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

138 ↗

(On Diff #212462)

if (z != s) can be omitted

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

196 ↗

(On Diff #212462)

ASNI -> ANSI

225 ↗

(On Diff #212462)

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 ↗

(On Diff #212462)

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 ↗

(On Diff #212462)

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 ↗

(On Diff #212462)

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 ↗

(On Diff #212462)

seplling is hrad.

[vitalybuka]

compiler-rt/lib/sanitizer_common/sanitizer_common.cpp

99

INLINE is not necessary

can this be?

return SANITIZER_FUCHSIA || report_file.SupportsColors();

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cpp

58

should these be kDefault

59

DecorationKind prev = kUnknown;
DecorationKind next = kUnknown;

69

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

why next must be kBold?

91

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

99

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.cpp
   sanitizer_posix.cpp
   sanitizer_printf.cpp
   sanitizer_procmaps_common.cpp
   sanitizer_procmaps_bsd.cpp
   sanitizer_procmaps_linux.cpp
   sanitizer_procmaps_mac.cpp
   sanitizer_procmaps_solaris.cpp
+  sanitizer_report_decorator.cpp
   sanitizer_rtems.cpp
   sanitizer_solaris.cpp
   sanitizer_stoptheworld_mac.cpp
   sanitizer_suppressions.cpp
   sanitizer_tls_get_addr.cpp
   sanitizer_thread_registry.cpp
   sanitizer_type_traits.cpp
   sanitizer_win.cpp

compiler-rt/lib/sanitizer_common/sanitizer_common.cpp

 // run-time libraries.
 //===----------------------------------------------------------------------===//
 
 #include "sanitizer_common.h"
 #include "sanitizer_allocator_interface.h"
 #include "sanitizer_allocator_internal.h"
 #include "sanitizer_atomic.h"
 #include "sanitizer_flags.h"
+#include "sanitizer_file.h"
 #include "sanitizer_libc.h"
 #include "sanitizer_placement_new.h"
 
 namespace __sanitizer {
 
 const char *SanitizerToolName = "SanitizerTool";
 
 atomic_uint32_t current_verbosity;
void ReportErrorSummary(const char *error_message, const char *alt_tool_name) {
   if (!common_flags()->print_summary)
     return;
   InternalScopedString buff(kMaxSummaryLength);
   buff.append("SUMMARY: %s: %s",
               alt_tool_name ? alt_tool_name : SanitizerToolName, error_message);
   __sanitizer_report_error_summary(buff.data());
 }
 
+#if !SANITIZER_FUCHSIA
+
+static INLINE bool ReportSupportsColors() {
+  return report_file.SupportsColors();
+}
+
+#else  // SANITIZER_FUCHSIA
+
+// Fuchsia's logs always go through post-processing that handles colorization.
+static INLINE bool ReportSupportsColors() { return true; }
+
+#endif  // !SANITIZER_FUCHSIA
+
+bool ColorizeReports() {
+  // FIXME: Add proper Windows support to AnsiColorDecorator and re-enable color
+  // printing on Windows.
+  if (SANITIZER_WINDOWS)
+    return false;
+
+  const char *flag = common_flags()->color;
+  return internal_strcmp(flag, "always") == 0 ||
+         (internal_strcmp(flag, "auto") == 0 && ReportSupportsColors());
+}
+
 // Removes the ANSI escape sequences from the input string (in-place).
 void RemoveANSIEscapeSequencesFromString(char *str) {
   if (!str)
     return;
 
   // We are going to remove the escape sequences in place.
   char *s = str;
   char *z = str;

compiler-rt/lib/sanitizer_common/sanitizer_printf.cpp

 // 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 ToStr(kBold); }
+  const char *Default() const { return 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.
+  void Compact(char* str) const;
+
  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 ToStr(kBlack); }
+  const char *Red() const { return ToStr(kRed); }
+  const char *Green() const { return ToStr(kGreen); }
+  const char *Yellow() const { return ToStr(kYellow); }
+  const char *Blue() const { return ToStr(kBlue); }
+  const char *Magenta() const { return ToStr(kMagenta); }
+  const char *Cyan() const { return ToStr(kCyan); }
+  const char *White() const { return ToStr(kWhite); }
+
  private:
-  bool ansi_;
+  // The currently supported ANSI tags.
+  enum DecorationKind {
+    kUnknown = -1,
+    kDefault = 0,
+    kBold = 1,
+    kBlack = 30,
+    kRed = 31,
+    kGreen = 32,
+    kYellow = 33,
+    kBlue = 34,
+    kMagenta = 35,
+    kCyan = 36,
+    kWhite = 37,
+  };
+
+  const char* ToStr(DecorationKind kind) const;
+
+  const bool ansi_;
 };
 
 }  // namespace __sanitizer
 
 #endif  // SANITIZER_REPORT_DECORATOR_H

compiler-rt/lib/sanitizer_common/sanitizer_report_decorator.cpp

+//===-- 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_report_decorator.h"
+
+#include "sanitizer_libc.h"
+
+namespace __sanitizer {
+
+const char* SanitizerCommonDecorator::ToStr(DecorationKind kind) const {
+  if (!ansi_)
+    return "";
+  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 "";
+  }
+}
+
+void SanitizerCommonDecorator::Compact(char* str) const {
+  if (!str)
+    return;
+  const char *in = str;
+  char* out = 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, next = kUnknown;
+  do {
+    // 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.
+    while (true) {
+      s64 code = kUnknown;
+      const char* endptr = in;
+      if (internal_strncmp(in, "\033[", 2) == 0)
+        code = internal_simple_strtoll(in + 2, &endptr, 10);
+      if (endptr <= in + 2 || *endptr != 'm')
+        code = kUnknown;
+      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)
+            code = kUnknown;
+          break;
+        default:
+          code = kUnknown;
+      }
+      if (code == kUnknown)
+        break;
+      in = endptr + 1;
+      next = static_cast<DecorationKind>(code);
+    }
+
+    // If the current character is printable or the end of the string, and the
+    // active tag has changed, print the tag.
+    if (((0x20 < *in && *in < 0x7f) || *in == '\0') && prev != next) {
+      auto tag = ToStr(next);
+      auto tag_len = internal_strlen(tag);  // Not null-terminated!
+      internal_strncpy(out, tag, tag_len);
+      prev = next;
+      out += tag_len;
+    }
+  } while((*out++ = *in++) != '\0');
+}
+
+}  // namespace __sanitizer

compiler-rt/lib/sanitizer_common/sanitizer_symbolizer_report.cpp

void ReportErrorSummary(const char *error_type, const AddressInfo &info,
   InternalScopedString buff(kMaxSummaryLength);
   buff.append("%s ", error_type);
   RenderFrame(&buff, "%L %F", 0, info, common_flags()->symbolize_vs_style,
               common_flags()->strip_path_prefix);
   ReportErrorSummary(buff.data(), alt_tool_name);
 }
 #endif
 
-#if !SANITIZER_FUCHSIA
-
-bool ReportFile::SupportsColors() {
-  SpinMutexLock l(mu);
-  ReopenIfNecessary();
-  return SupportsColoredOutput(fd);
-}
-
-static INLINE bool ReportSupportsColors() {
-  return report_file.SupportsColors();
-}
-
-#else  // SANITIZER_FUCHSIA
-
-// Fuchsia's logs always go through post-processing that handles colorization.
-static INLINE bool ReportSupportsColors() { return true; }
-
-#endif  // !SANITIZER_FUCHSIA
-
-bool ColorizeReports() {
-  // FIXME: Add proper Windows support to AnsiColorDecorator and re-enable color
-  // printing on Windows.
-  if (SANITIZER_WINDOWS)
-    return false;
-
-  const char *flag = common_flags()->color;
-  return internal_strcmp(flag, "always") == 0 ||
-         (internal_strcmp(flag, "auto") == 0 && ReportSupportsColors());
-}
-
 void ReportErrorSummary(const char *error_type, const StackTrace *stack,
                         const char *alt_tool_name) {
 #if !SANITIZER_GO
   if (!common_flags()->print_summary)
     return;
   if (stack->size == 0) {
     ReportErrorSummary(error_type);
     return;

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

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);
 }
 
+// Value parameterized test for compacting ANSI color sequences.
+struct CompactParam {
+  const char *Input;
+  const char *WithoutColor;
+  const char *WithColor;
+};
+
+class CompactTest : public testing::TestWithParam<CompactParam> {};
+
+// Derived decorator that allows setting color support manualy.
+class TestDecorator : public SanitizerCommonDecorator {
+  TestDecorator(bool ansi) { ansi_ = ansi; }
+};
+
+TEST_P(CompactTest, WithoutColor) {
+  auto Param = GetParam();
+  char *Actual = strdup(Param.Input);
+  ASSERT_NOT_NULL(Actual);
+  TestDecorator(false).Compact(Actual);
+  EXPECT_STREQ(Actual, Param.WithoutColor);
+  free(Actual);
+};
+
+TEST_P(CompactTest, WithColor) {
+  auto Param = GetParam();
+  char *Actual = strdup(Param.Input);
+  ASSERT_NOT_NULL(Actual);
+  TestDecorator(true).Compact(Actual);
+  EXPECT_STREQ(Actual, Param.WithColor);
+  free(Actual);
+}
+
+INSTANTIATE_TEST_SUITE_P(
+    WithSpaces, CompactTest,
+    testing::Values(
+        {
+            " x\033[1m\033[31m\033[1m\033[31m\033[1m\033[34my",
+            " xy",
+            " x\033[1m\033[34my",
+        },
+        {
+            "\033[1m\033[31m x\033[1m\033[31m\033[1m\033[31m\033[1m\033[34my",
+            " xy",
+            " \033[1m\033[31mx\033[1m\033[34my",
+        },
+        {
+            "\033[1m\033[34mx \033[1m\033[31m\033[1m\033[31m\033[1m\033[34my",
+            "x y",
+            "\033[1m\033[34mx y",
+        },
+        {
+            "\033[1m\033[31mx\033[1m\033[31m "
+            "\033[1m\033[31m\033[1m\033[34my\033[1m\033[31m",
+            "x y",
+            "\033[1m\033[31mx \033[1m\033[34my\033[1m\033[31m",
+        },
+        {
+            "\033[1m\033[34mx\033[1m\033[31m\033[1m\033[31m "
+            "\033[1m\033[34my\033[1m\033[31m",
+            "x y",
+            "\033[1m\033[34mx y\033[1m\033[31m",
+        },
+        {
+            "\033[1m\033[31mx\033[1m\033[31m\033[1m\033[31m\033[1m\033[34m "
+            "y\033[1m\033[34m",
+            "x y",
+            "\033[1m\033[31mx \033[1m\033[34my",
+        },
+        {
+            "\033[1m\033[34mx\033[1m\033[31m\033[1m\033[31m\033[1m\033[34my "
+            "\033[1m\033[34m",
+            "xy ",
+            "\033[1m\033[34mxy ",
+        },
+        {
+            "x\033[1m\033[31m\033[1m\033[34m\033[1m\033[31my ",
+            "xy ",
+            "x\033[1m\033[31my ",
+        }));
+
+INSTANTIATE_TEST_SUITE_P(
+    WithNewlines, CompactTest,
+    testing::Values(
+        {
+            "\n\033[1m\033[31mx\033[1m\033[31m\033[1m\033[34m\033[1m\033[31my",
+            "\nxy",
+            "\n\033[1m\033[31mxy",
+        },
+        {
+            "\033[1m\033[34m\nx\033[1m\033[31m\033[1m\033[34m\033[1m\033[31my",
+            "\nxy",
+            "\n\033[1m\033[34mx\033[1m\033[31my",
+        },
+        {
+            "\033[1m\033[31mx\n\033[1m\033[31m\033[1m\033[34m\033[1m\033["
+            "31my\033[1m\033[31m",
+            "x\ny",
+            "\033[1m\033[31mx\ny",
+        },
+        {
+            "\033[1m\033[34mx\033[1m\033[31m\n\033[1m\033[34m\033[1m\033["
+            "31my\033[1m\033[31m",
+            "x\ny",
+            "\033[1m\033[34mx\n\033[1m\033[31my",
+        },
+        {
+            "\033[1m\033[31mx\033[1m\033[31m\033[1m\033[34m\n\033[1m\033["
+            "31my\033[1m\033[34m",
+            "x\ny",
+            "\033[1m\033[31mx\ny\033[1m\033[34m",
+        },
+        {
+            "\033[1m\033[34mx\033[1m\033[31m\033[1m\033[34m\033[1m\033["
+            "31m\ny\033[1m\033[34m",
+            "x\ny",
+            "\033[1m\033[34mx\n\033[1m\033[31my\033[1m\033[34m",
+        },
+        {
+            "x\033[1m\033[34m\033[1m\033[31m\033[1m\033[31my\n",
+            "xy\n",
+            "x\033[1m\033[31my\n",
+        },
+        {
+            "\033[1m\033[31mx\033[1m\033[34m\033[1m\033[31m\033[1m\033[31my\n",
+            "xy\n",
+            "\033[1m\033[31mxy\n",
+        }));
+
+INSTANTIATE_TEST_SUITE_P(
+    WithTabs, CompactTest,
+    testing::Values(
+        {
+            "\t\033[1m\033[34mx\033[1m\033[34m\033[1m\033[31m\033[1m\033[31my",
+            "\txy",
+            "\t\033[1m\033[34mx\033[1m\033[31my",
+        },
+        {
+            "\033[1m\033[31m\tx\033[1m\033[34m\033[1m\033[31m\033[1m\033["
+            "31my\033[1m\033[31m",
+            "\txy",
+            "\t\033[1m\033[31mxy",
+        },
+        {
+            "\033[1m\033[34mx\t\033[1m\033[34m\033[1m\033[31m\033[1m\033["
+            "31my\033[1m\033[31m",
+            "x\ty",
+            "\033[1m\033[34mx\t\033[1m\033[31my",
+        },
+        {
+            "\033[1m\033[31mx\033[1m\033[34m\t\033[1m\033[31m\033[1m\033["
+            "31my\033[1m\033[34m",
+            "x\ty",
+            "\033[1m\033[31mx\ty\033[1m\033[34m",
+        },
+        {
+            "\033[1m\033[34mx\033[1m\033[34m\033[1m\033[31m\t\033[1m\033["
+            "31my\033[1m\033[34m",
+            "x\ty",
+            "\033[1m\033[34mx\t\033[1m\033[31my\033[1m\033[34m",
+        }));
+
 }  // namespace __sanitizer