base: make CHECK macros trap at distinct addresses in official builds

base/logging.h

Index: base/logging.h
diff --git a/base/logging.h b/base/logging.h
index 714545c02bb06ac7f1b31e8a007b671e6dc11d6d..7f6febbb05f3138f20a889f1f279afdcfe63a784 100644
--- a/base/logging.h
+++ b/base/logging.h
@@ -15,6 +15,7 @@
 #include <utility>
 
 #include "base/base_export.h"
+#include "base/compiler_specific.h"
 #include "base/debug/debugger.h"
 #include "base/macros.h"
 #include "base/template_util.h"
@@ -458,16 +459,43 @@ class CheckOpResult {
 // bloat, and improve performance, for official release builds.
 
 #if defined(COMPILER_GCC) || __clang__
-#define LOGGING_CRASH() __builtin_trap()
-#else
-#define LOGGING_CRASH() ((void)(*(volatile char*)0 = 0))
+
+// This is to make sure that the crash report can pinpoint to the right source
+// line even in presence of multiple CHECK()s in the same function. See
+// http://crbug.com/664209 for more context.
+// There are three things we need to guarantee here:
+// - The trap instructions, and hence the PC value at crash time, are distinct
+//   and don't get collapsed into the same op due to compiler optimizations.
+// - The debug line info for the trap instructions get attributed to the caller
+//   of CHECK(), to make sure that crash reports show actionable data. This in
+//   practice rules out the ability of using a inline function, at least as
+//   long as clang doesn't support attribute(artificial).
+// - The compiler should treat the CHECK as no-return instructions, so that the
+//   trap code can be efficiently packed in the prologue of the function and
+//   doesn't interfere with the main execution flow.
+// Code-wise:
+// - The asm volatile statement is a cross-architecture version of
+//   mov %eax, LINE. It guarantees that the assembly is unique for each line
+//   and the multiple crash instructions generated by CHECK() don't get folded.
+// - __builtin_trap() is the instruction that actually causes the crash,
+//   either a SIGTRAP or a SIGABRT depending on the platform.
+// - __builtin_unreachable() enables no-return optimizations.
+#define LOGGING_CRASH(LINE)                    \
+  ({                                           \
+    asm volatile("" : : "r"(LINE) : "memory"); \
+    __builtin_trap();                          \
+    __builtin_unreachable();                   \
+  })
+
+#else  // !(defined(COMPILER_GCC) || __clang__)
+#define LOGGING_CRASH(LINE) ((void)(*(volatile char*)0 = 0))

[Nico, 2016/12/02 02:04:03]

This doesn't help on Windows, does it. Would

  ((void)(*(volatile char*)0 = LINE))

be something that works everywhere? x86 has up to 32bit immediates and supports
immediate->mem moves, but I guess on arm it'd still clobber a reg (or several).

How does Torne's suggestion help? Aren't these ud2s mergable too? Or do
compilers not try to reason about inline asm? (LLVM probably doesn't.) 

(https://gcc.gnu.org/onlinedocs/gcc-4.6.4/gcc/Other-Builtins.html claims that
__builtin_unreachable would generate a return, but at least clang seems to not
do this.)

 #endif
 
 // This is not calling BreakDebugger since this is called frequently, and
 // calling an out-of-line function instead of a noreturn inline macro prevents
 // compiler optimizations.
-#define CHECK(condition)                                                \
-  !(condition) ? LOGGING_CRASH() : EAT_STREAM_PARAMETERS
+#define CHECK(condition) \
+  !(condition) ? LOGGING_CRASH(__LINE__) : EAT_STREAM_PARAMETERS
 
 #define PCHECK(condition) CHECK(condition)