[logging] Fix printing of single-byte enums

We still get e.g. ClusterFuzz reports with enums printed as
non-printable single-character strings (see linked bug).
This CL fixes this, and also includes the integral enum value for enum
that come with their own output operator.

This makes error messages strictly better, at the cost of some more code
per enum which is being used in a CHECK/DCHECK.
Note that binary size of release builds is not affected, since we do not
print the values there.

R=nicohartmann@chromium.org

Bug: v8:11384, chromium:1187484
Change-Id: I066b32f68440096babed9b629c7ffe3f2285cba8
Reviewed-on: https://chromium-review.googlesource.com/c/v8/v8/+/2756226Reviewed-by: Nico Hartmann <nicohartmann@chromium.org>
Commit-Queue: Clemens Backes <clemensb@chromium.org>
Cr-Commit-Position: refs/heads/master@{#73373}

src/base/logging.h

@@ -140,16 +140,36 @@ V8_BASE_EXPORT void SetDcheckFunction(void (*dcheck_Function)(const char*, int,
 #define CONSTEXPR_DCHECK(cond)
 #endif
 
+namespace detail {
+template <typename... Ts>
+std::string PrintToString(Ts&&... ts) {
+  std::ostringstream oss;
+  int unused_results[]{((oss << std::forward<Ts>(ts)), 0)...};
+  (void)unused_results;  // Avoid "unused variable" warning.
+  return oss.str();
+}
+
+template <typename T>
+auto GetUnderlyingEnumTypeForPrinting(T val) {
+  using underlying_t = typename std::underlying_type<T>::type;
+  // For single-byte enums, return a 16-bit integer to avoid printing the value
+  // as a character.
+  using int_t = typename std::conditional_t<
+      sizeof(underlying_t) != 1, underlying_t,
+      std::conditional_t<std::is_signed<underlying_t>::value, int16_t,
+                         uint16_t> >;
+  return static_cast<int_t>(static_cast<underlying_t>(val));
+}
+}  // namespace detail
+
 // Define PrintCheckOperand<T> for each T which defines operator<< for ostream.
 template <typename T>
 typename std::enable_if<
     !std::is_function<typename std::remove_pointer<T>::type>::value &&
-        has_output_operator<T>::value,
+        !std::is_enum<T>::value && has_output_operator<T>::value,
     std::string>::type
 PrintCheckOperand(T val) {
-  std::ostringstream oss;
-  oss << std::forward<T>(val);
-  return oss.str();
+  return detail::PrintToString(std::forward<T>(val));
 }
 
 // Provide an overload for functions and function pointers. Function pointers
@@ -165,18 +185,32 @@ PrintCheckOperand(T val) {
   return PrintCheckOperand(reinterpret_cast<const void*>(val));
 }
 
-// Define PrintCheckOperand<T> for enums which have no operator<<.
+// Define PrintCheckOperand<T> for enums with an output operator.
+template <typename T>
+typename std::enable_if<std::is_enum<T>::value && has_output_operator<T>::value,
+                        std::string>::type
+PrintCheckOperand(T val) {
+  std::string val_str = detail::PrintToString(val);
+  std::string int_str =
+      detail::PrintToString(detail::GetUnderlyingEnumTypeForPrinting(val));
+  // Printing the original enum might have printed a single non-printable
+  // character. Ignore it in that case. Also ignore if it printed the same as
+  // the integral representation.
+  // TODO(clemensb): Can we somehow statically find out if the output operator
+  // is the default one, printing the integral value?
+  if ((val_str.length() == 1 && !std::isprint(val_str[0])) ||
+      val_str == int_str) {
+    return int_str;
+  }
+  return detail::PrintToString(val_str, " (", int_str, ")");
+}
+
+// Define PrintCheckOperand<T> for enums without an output operator.
 template <typename T>
 typename std::enable_if<
     std::is_enum<T>::value && !has_output_operator<T>::value, std::string>::type
 PrintCheckOperand(T val) {
-  using underlying_t = typename std::underlying_type<T>::type;
-  // 8-bit types are not printed as number, so extend them to 16 bit.
-  using int_t = typename std::conditional<
-      std::is_same<underlying_t, uint8_t>::value, uint16_t,
-      typename std::conditional<std::is_same<underlying_t, int8_t>::value,
-                                int16_t, underlying_t>::type>::type;
-  return PrintCheckOperand(static_cast<int_t>(static_cast<underlying_t>(val)));
+  return detail::PrintToString(detail::GetUnderlyingEnumTypeForPrinting(val));
 }
 
 // Define default PrintCheckOperand<T> for non-printable types.

test/unittests/base/logging-unittest.cc

@@ -230,11 +230,26 @@ void operator<<(std::ostream& str, TestEnum6 val) {
 TEST(LoggingDeathTest, OutputEnumWithOutputOperator) {
   ASSERT_DEATH_IF_SUPPORTED(
       ([&] { CHECK_EQ(TEST_A, TEST_B); })(),
-      FailureMessage("Check failed: TEST_A == TEST_B", "A", "B"));
+      FailureMessage("Check failed: TEST_A == TEST_B", "A (0)", "B (1)"));
   ASSERT_DEATH_IF_SUPPORTED(
       ([&] { CHECK_GE(TestEnum6::TEST_C, TestEnum6::TEST_D); })(),
       FailureMessage("Check failed: TestEnum6::TEST_C >= TestEnum6::TEST_D",
-                     "C", "D"));
+                     "C (0)", "D (1)"));
+}
+
+enum TestEnum7 : uint8_t { A = 2, B = 7 };
+enum class TestEnum8 : int8_t { A, B };
+
+TEST(LoggingDeathTest, OutputSingleCharEnum) {
+  ASSERT_DEATH_IF_SUPPORTED(
+      ([&] { CHECK_EQ(TestEnum7::A, TestEnum7::B); })(),
+      FailureMessage("Check failed: TestEnum7::A == TestEnum7::B", "2", "7"));
+  ASSERT_DEATH_IF_SUPPORTED(
+      ([&] { CHECK_GT(TestEnum7::A, TestEnum7::B); })(),
+      FailureMessage("Check failed: TestEnum7::A > TestEnum7::B", "2", "7"));
+  ASSERT_DEATH_IF_SUPPORTED(
+      ([&] { CHECK_GE(TestEnum8::A, TestEnum8::B); })(),
+      FailureMessage("Check failed: TestEnum8::A >= TestEnum8::B", "0", "1"));
 }
 
 TEST(LoggingDeathTest, OutputLongValues) {
@@ -328,6 +343,12 @@ TEST(LoggingTest, LogFunctionPointers) {
 }
 #endif  // defined(DEBUG)
 
+TEST(LoggingDeathTest, CheckChars) {
+  ASSERT_DEATH_IF_SUPPORTED(
+      ([&] { CHECK_EQ('a', 'b'); })(),
+      FailureMessage("Check failed: 'a' == 'b'", "'97'", "'98'"));
+}
+
 }  // namespace logging_unittest
 }  // namespace base
 }  // namespace v8