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 5174e6d375540cd0f0bf355034c17812a66bf37b..986eeba88b80261118807ef4d1ff8beb574a22f6 100644
--- a/base/logging.h
+++ b/base/logging.h
@@ -462,8 +462,47 @@ class CheckOpResult {
 
 // Crashes in the fastest, simplest possible way with no attempt at logging.
 #if defined(COMPILER_GCC) || defined(__clang__)
-#define IMMEDIATE_CRASH() __builtin_trap()
+
+// 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:
+// - asm volatile seems to be enough to make the compiler emit distinct
+//   instructions for each CHECK() and not fold them together.
+// - the actual instructions (e.g., "ud2") are borrowed from implementation of
+//   __builtin_trap() in the various toolchain.
+//   either a SIGTRAP or a SIGABRT depending on the platform.

[Mark Mentovai, 2017/02/13 16:22:09]

On x86 and x86_64, ud2 will be SIGILL. That’s good, but it’s not distinct in
that true bad codegen will also be SIGILL. SIGABRT or SIGTRAP, as this comment
says, would be much better.

On the Windows equivalent (https://codereview.chromium.org/2676483002/), we’re
discussing int3 with a ud2 backstop. int3 will be SIGTRAP which is somewhat
distinct. The ud2 backstop is there to provide a non-continuable barrier to
further execution, otherwise it’s not really noreturn because it’s trivial to
continue past int3.

The reserved illegal opcode backstop could also work for ARM. I don’t know that
it’ll solve multiple CHECK()s jumping to a single target that you’re seeing, but
it’s a further refinement.

+// - __builtin_unreachable() enables no-return optimizations.
+
+#if defined(ARCH_CPU_X86_FAMILY)
+#define TRAP_INSTRUCTION() asm volatile("ud2" ::: "memory")
+#elif defined(ARCH_CPU_ARMEL)
+#define TRAP_INSTRUCTION() asm volatile("udf $0xff" ::: "memory")
+#elif defined(ARCH_CPU_ARM64)
+#define TRAP_INSTRUCTION() asm volatile("brk #0x3e8" ::: "memory")
 #else
+// Crash report accuracy will not be guaranteed on other architectures, but at
+// least this will crash as expected.
+#define TRAP_INSTRUCTION() __builtin_trap()
+#endif
+
+#define IMMEDIATE_CRASH()    \
+  ({                         \
+    TRAP_INSTRUCTION();      \
+    __builtin_unreachable(); \
+  })
+
+#else  // defined(COMPILER_GCC) || defined(__clang__)
 #define IMMEDIATE_CRASH() ((void)(*(volatile char*)0 = 0))
 #endif