Make v8 build with -Wmicrosoft-cast under clang-cl.

gcc and clang (and the standard) don't allow implicit conversion of
function pointers to object pointers. MSVC does allow that, and since
system headers require this to work, clang-cl allows it too -- but
it emits a -Wmicrosoft-cast warning (which we currently suppress in
the Chromium build, but which we want to enable.)

As a side effect, when printing a function pointer to a stream, MSVC
(and clang-cl) will pick the operator<<(void*) overload, while gcc
and clang will pick operator<<(bool) since the best allowed conversion
they find is from function pointer to bool.

To prevent the clang-cl warning, we need to make sure that we never
directly print a function pointer to a stream. In v8, this requires
two changes:

1. Give PrintCheckOperand() an explicit specialization for function
   pointers and explicitly cast to void* there.  This ports
   https://codereview.chromium.org/2515283002/ to V8, and also fixes a
   bug on non-Windows where DCHECK() of function pointers would print
   "(1 vs 1)" instead of the function's addresses.
   (The bug remains with member function pointers,
   where it's not clear what to print instead of the 1.)

2. has_output_operator<T> must not use operator<< on its argument
   in an evaluated context if T is a function pointer.  This patch
   modifies has_output_operator<> to use an unevaluated context instead,
   which is simpler than the current approach (and matches what Chromium's
   base does), but changes behavior    in minor (boring) ways
   (see template-utils-unittest.cc), since operator<<() is now
   called with a temporary and only operator<<() implementations callable
   with a temporary are considered.
   A more complicated but behavior-preserving alternative would be to
   add an explicit specialization for function pointers. You can see
   this variant in patch set 1 on gerrit.

Bug: chromium:550065
Change-Id: Idc2854d6c258b7fc0b959604006d8952a79eca3d
Reviewed-on: https://chromium-review.googlesource.com/940004
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Clemens Hammacher <clemensh@chromium.org>
Cr-Commit-Position: refs/heads/master@{#51636}

src/base/logging.h

@@ -106,11 +106,25 @@ V8_BASE_EXPORT void SetDcheckFunction(void (*dcheck_Function)(const char*, int,
 
 // Define PrintCheckOperand<T> for each T which defines operator<< for ostream.
 template <typename T>
-typename std::enable_if<has_output_operator<T>::value>::type PrintCheckOperand(
-    std::ostream& os, T val) {
+typename std::enable_if<
+    !std::is_function<typename std::remove_pointer<T>::type>::value &&
+    has_output_operator<T>::value>::type
+PrintCheckOperand(std::ostream& os, T val) {
   os << std::forward<T>(val);
 }
 
+// Provide an overload for functions and function pointers. Function pointers
+// don't implicitly convert to void* but do implicitly convert to bool, so
+// without this function pointers are always printed as 1 or 0. (MSVC isn't
+// standards-conforming here and converts function pointers to regular
+// pointers, so this is a no-op for MSVC.)
+template <typename T>
+typename std::enable_if<
+    std::is_function<typename std::remove_pointer<T>::type>::value>::type
+PrintCheckOperand(std::ostream& os, T val) {
+  os << reinterpret_cast<const void*>(val);
+}
+
 // Define PrintCheckOperand<T> for enums which have no operator<<.
 template <typename T>
 typename std::enable_if<std::is_enum<T>::value &&

src/base/template-utils.h

@@ -79,20 +79,13 @@ struct pass_value_or_ref {
                                          decay_t, const decay_t&>::type;
 };
 
+// Uses expression SFINAE to detect whether using operator<< would work.
+template <typename T, typename = void>
+struct has_output_operator : std::false_type {};
 template <typename T>
-struct has_output_operator {
-  // This template is only instantiable if U provides operator<< with ostream.
-  // Its return type is uint8_t.
-  template <typename U>
-  static auto __check_operator(U u)
-      -> decltype(*(std::ostream*)nullptr << *u, uint8_t{0});
-  // This is a fallback implementation, returning uint16_t. If the template
-  // above is instantiable, is has precedence over this varargs function.
-  static uint16_t __check_operator(...);
-
-  using ptr_t = typename std::add_pointer<T>::type;
-  static constexpr bool value = sizeof(__check_operator(ptr_t{nullptr})) == 1;
-};
+struct has_output_operator<T, decltype(void(std::declval<std::ostream&>()
+                                            << std::declval<T>()))>
+    : std::true_type {};
 
 namespace detail {
 

test/unittests/base/logging-unittest.cc

@@ -248,6 +248,42 @@ TEST(LoggingDeathTest, V8_DcheckCanBeOverridden) {
       "Dread pirate");
 }
 
+#if defined(DEBUG)
+namespace {
+int g_log_sink_call_count = 0;
+void DcheckCountFunction(const char* file, int line, const char* message) {
+  ++g_log_sink_call_count;
+}
+
+void DcheckEmptyFunction1() {
+  // Provide a body so that Release builds do not cause the compiler to
+  // optimize DcheckEmptyFunction1 and DcheckEmptyFunction2 as a single
+  // function, which breaks the Dcheck tests below.
+  // Note that this function is never actually called.
+  g_log_sink_call_count += 42;
+}
+void DcheckEmptyFunction2() {}
+
+}  // namespace
+
+TEST(LoggingTest, LogFunctionPointers) {
+  v8::base::SetDcheckFunction(&DcheckCountFunction);
+  g_log_sink_call_count = 0;
+  void (*fp1)() = DcheckEmptyFunction1;
+  void (*fp2)() = DcheckEmptyFunction2;
+  void (*fp3)() = DcheckEmptyFunction1;
+  DCHECK_EQ(fp1, DcheckEmptyFunction1);
+  DCHECK_EQ(fp1, fp3);
+  EXPECT_EQ(0, g_log_sink_call_count);
+  DCHECK_EQ(fp1, fp2);
+  EXPECT_EQ(1, g_log_sink_call_count);
+  std::string* error_message =
+      CheckEQImpl<decltype(fp1), decltype(fp2)>(fp1, fp2, "");
+  EXPECT_NE(*error_message, "(1 vs 1)");
+  delete error_message;
+}
+#endif  // defined(DEBUG)
+
 }  // namespace logging_unittest
 }  // namespace base
 }  // namespace v8

test/unittests/base/template-utils-unittest.cc

@@ -93,13 +93,19 @@ static_assert(has_output_operator<uint64_t>::value, "int can be output");
 // Classes:
 class TestClass1 {};
 class TestClass2 {};
-extern std::ostream& operator<<(std::ostream& str, TestClass2&);
+extern std::ostream& operator<<(std::ostream& str, const TestClass2&);
+class TestClass3 {};
+extern std::ostream& operator<<(std::ostream& str, TestClass3);
 static_assert(!has_output_operator<TestClass1>::value,
               "TestClass1 can not be output");
 static_assert(has_output_operator<TestClass2>::value,
               "non-const TestClass2 can be output");
-static_assert(!has_output_operator<const TestClass2>::value,
-              "const TestClass2 can not be output");
+static_assert(has_output_operator<const TestClass2>::value,
+              "const TestClass2 can be output");
+static_assert(has_output_operator<TestClass3>::value,
+              "non-const TestClass3 can be output");
+static_assert(has_output_operator<const TestClass3>::value,
+              "const TestClass3 can be output");
 
 //////////////////////////////
 // Test fold.