Improve EAT_STREAM_PARAMETERS for Windows x86

Dumps of check_example.exe Current: ?DoBlinkReleaseAssert@@YAX_N@Z: 00404EDC: 55 push ebp 00404EDD: 8B EC mov ebp,esp 00404EDF: 80 7D 08 00 cmp byte ptr [ebp+8],0 00404EE3: 75 07 jne 00404EEC 00404EE5: C6 05 00 00 00 00 mov byte ptr ds:[0],0 00 00404EEC: 5D pop ebp 00404EED: C3 ret ?DoCheck@@YAX_N@Z: 00404EEE: 55 push ebp 00404EEF: 8B EC mov ebp,esp 00404EF1: 51 push ecx 00404EF2: 83 65 FC 00 and dword ptr [ebp-4],0 00404EF6: 80 7D 08 00 cmp byte ptr [ebp+8],0 00404EFA: 75 07 jne 00404F03 00404EFC: C6 05 00 00 00 00 mov byte ptr ds:[0],0 00 00404F03: 8B E5 mov esp,ebp 00404F05: 5D pop ebp 00404F06: C3 ret _main: 00404F07: 55 push ebp 00404F08: 8B EC mov ebp,esp 00404F0A: 83 7D 08 02 cmp dword ptr [ebp+8],2 00404F0E: 53 push ebx 00404F0F: 0F 9F C3 setg bl 00404F12: 53 push ebx 00404F13: E8 D6 FF FF FF call ?DoCheck@@YAX_N@Z 00404F18: 53 push ebx 00404F19: E8 BE FF FF FF call ?DoBlinkReleaseAssert@@YAX_N@Z 00404F1E: 59 pop ecx 00404F1F: 59 pop ecx 00404F20: 33 C0 xor eax,eax 00404F22: 5B pop ebx 00404F23: 5D pop ebp 00404F24: C3 ret After this CL: ?DoBlinkReleaseAssert@@YAX_N@Z: 00404EAC: 55 push ebp 00404EAD: 8B EC mov ebp,esp 00404EAF: 80 7D 08 00 cmp byte ptr [ebp+8],0 00404EB3: 75 07 jne 00404EBC 00404EB5: C6 05 00 00 00 00 mov byte ptr ds:[0],0 00 00404EBC: 5D pop ebp 00404EBD: C3 ret _main: 00404EBE: 55 push ebp 00404EBF: 8B EC mov ebp,esp 00404EC1: 83 7D 08 02 cmp dword ptr [ebp+8],2 00404EC5: 53 push ebx 00404EC6: 0F 9F C3 setg bl 00404EC9: 53 push ebx 00404ECA: E8 DD FF FF FF call ?DoBlinkReleaseAssert@@YAX_N@Z 00404ECF: 53 push ebx 00404ED0: E8 D7 FF FF FF call ?DoBlinkReleaseAssert@@YAX_N@Z 00404ED5: 59 pop ecx 00404ED6: 59 pop ecx 00404ED7: 33 C0 xor eax,eax 00404ED9: 5B pop ebx 00404EDA: 5D pop ebp 00404EDB: C3 ret Amusingly, I was confused because I thought I was going crazy when DoCheck wasn't showing up in the /disasm. But of course, it's because it got COMDAT'd with the Blink one, as we want. :) R=primiano@chromium.org BUG=672699 Review-Url: https://codereview.chromium.org/2559323007 Review-Url: https://codereview.chromium.org/2559323007 Cr-Commit-Position: refs/heads/master@{#437777}
Denger-Network · Dec 10, 2016 · 3c957a5 · 3c957a5
1 parent b8b2bfc
commit 3c957a5
Show file tree

Hide file tree

Showing 3 changed files with 35 additions and 4 deletions.
diff --git a/base/check_example.cc b/base/check_example.cc
@@ -5,21 +5,33 @@
 // This file is meant for analyzing the code generated by the CHECK
 // macros in a small executable file that's easy to disassemble.
 
+#include "base/compiler_specific.h"
 #include "base/logging.h"
 
 // An official build shouldn't generate code to print out messages for
 // the CHECK* macros, nor should it have the strings in the
-// executable.
+// executable. It is also important that the CHECK() function collapse to the
+// same implementation as RELEASE_ASSERT(), in particular on Windows x86.
+// Historically, the stream eating caused additional unnecessary instructions.
+// See https://crbug.com/672699.
+
+#define BLINK_RELEASE_ASSERT_EQUIVALENT(assertion) \
+  (UNLIKELY(!(assertion)) ? (IMMEDIATE_CRASH()) : (void)0)
 
 void DoCheck(bool b) {
   CHECK(b) << "DoCheck " << b;
 }
 
+void DoBlinkReleaseAssert(bool b) {
+  BLINK_RELEASE_ASSERT_EQUIVALENT(b);
+}
+
 void DoCheckEq(int x, int y) {
   CHECK_EQ(x, y);
 }
 
 int main(int argc, const char* argv[]) {
   DoCheck(argc > 1);
   DoCheckEq(argc, 1);
+  DoBlinkReleaseAssert(argc > 1);
 }
diff --git a/base/logging.cc b/base/logging.cc
@@ -343,6 +343,11 @@ void CloseLogFileUnlocked() {
 
 }  // namespace
 
+// This is never instantiated, it's just used for EAT_STREAM_PARAMETERS to have
+// an object of the correct type on the LHS of the unused part of the ternary
+// operator.
+std::ostream* g_swallow_stream;
+
 LoggingSettings::LoggingSettings()
     : logging_dest(LOG_DEFAULT),
       log_file(nullptr),

diff --git a/base/logging.h b/base/logging.h
@@ -426,9 +426,23 @@ const LogSeverity LOG_0 = LOG_ERROR;
 #define PLOG_IF(severity, condition) \
   LAZY_STREAM(PLOG_STREAM(severity), LOG_IS_ON(severity) && (condition))
 
-// The actual stream used isn't important.
-#define EAT_STREAM_PARAMETERS                                           \
-  true ? (void) 0 : ::logging::LogMessageVoidify() & LOG_STREAM(FATAL)
+BASE_EXPORT extern std::ostream* g_swallow_stream;
+
+// Note that g_swallow_stream is used instead of an arbitrary LOG() stream to
+// avoid the creation of an object with a non-trivial destructor (LogMessage).
+// On MSVC x86 (checked on 2015 Update 3), this causes a few additional
+// pointless instructions to be emitted even at full optimization level, even
+// though the : arm of the ternary operator is clearly never executed. Using a
+// simpler object to be &'d with Voidify() avoids these extra instructions.
+// Using a simpler POD object with a templated operator<< also works to avoid
+// these instructions. However, this causes warnings on statically defined
+// implementations of operator<<(std::ostream, ...) in some .cc files, because
+// they become defined-but-unreferenced functions. A reinterpret_cast of 0 to an
+// ostream* also is not suitable, because some compilers warn of undefined
+// behavior.
+#define EAT_STREAM_PARAMETERS \
+  true ? (void)0              \
+       : ::logging::LogMessageVoidify() & (*::logging::g_swallow_stream)
 
 // Captures the result of a CHECK_EQ (for example) and facilitates testing as a
 // boolean.