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 6f30bcbe4f736f989bad178ac3d96ed70bf08943..df3d246609bbff0d7a8e22f4cf94626a9e5891d1 100644
--- a/base/logging.h
+++ b/base/logging.h
@@ -496,9 +496,59 @@ class CheckOpResult {
   std::string* message_;
 };
 
-// Crashes in the fastest, simplest possible way with no attempt at logging.
+// Crashes in the fastest possible way with no attempt at logging.
+// There are different constraints to satisfy here, see http://crbug.com/664209
+// for more context:
+// - The trap instructions, and hence the PC value at crash time, have to be
+//   distinct and not get folded into the same opcode by the compiler.
+//   On Linux/Android this is tricky because GCC still folds identical
+//   asm volatile blocks. The workaround is generating distinct opcodes for
+//   each CHECK using the __COUNTER__ macro.
+// - The debug info for the trap instruction has to be attributed to the source
+//   line that has the CHECK(), to make crash reports actionable. This rules
+//   out the ability of using a inline function, at least as long as clang
+//   doesn't support attribute(artificial).
+// - Failed CHECKs should produce a signal that is distinguishable from an
+//   invalid memory access, to improve the actionability of crash reports.
+// - 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.
+// - When debugging, developers shouldn't be able to accidentally step over a
+//   CHECK. This is achieved by putting opcodes that will cause a non
+//   continuable exception after the actual trap instruction.
+// - Don't cause too much binary bloat.
 #if defined(COMPILER_GCC)
-#define IMMEDIATE_CRASH() __builtin_trap()
+
+#if defined(ARCH_CPU_X86_FAMILY)
+// int 3 will generate a SIGTRAP.
+#define TRAP_INSTRUCTION() \
+  asm volatile("int $3; push %0; ud2;" ::"i"(__COUNTER__ % 128))

[Mark Mentovai, 2017/02/17 14:24:28]

The push imm8 form of this instruction dedicates a whole byte to imm8, you
shouldn’t need to take it mod 128. I bet you did this because "i"(128) (when
__COUNTER__ reaches that) gives you (int)128 which overflows as an 8-bit
quantity, so you’d wind up with a fatter variant of push immediate.

Instead of "i"(__COUNTER__ % 128), take "i"((signed char)__COUNTER__).

Now you can use twice as many of these in the same file without the compiler
deciding that any two instances are identical!

For that matter, you can do "i"(__COUNTER__ - 128) and you’ll get the thin imm8
form for the first 256 occurrences in a file, and then you’ll get fatter forms
for subsequent occurrences, but there’s a much stronger guarantee that there
won’t be any overlap.

[Mark Mentovai, 2017/02/17 14:24:28]

Can the push go behind the ud2?

[Mark Mentovai, 2017/02/17 14:24:28]

You should write this as int3, which is a distinct mnemonic that always
corresponds to a single-byte form of the instruction. I don’t trust every
assembler to emit the one-byte form upon seeing int $3.

[Primiano Tucci (use gerrit), 2017/02/17 14:51:04]

Ah right, I realized only now that there is a 1byte specialization of int3. 
FYI gcc was doing the right think, using the 1byte version even with int $3, but
make sense to be explicit.

[Primiano Tucci (use gerrit), 2017/02/17 14:51:04]

Spot on. I did % 128 because I observed that values > 127 did fall back on the 5
byte instruction (rather than 2).
However your suggestion does the trick. if I just ((uint8_t)__COUNTER__) I get
double the space and still the 2 bytes variant.
Hmm given that the overlapping problem seems bounded to  functions scope, I
think it is quite unlikely that a function will have > 255 CHECKs. Binary size
inflation is already in the ~hundreds kb ballpark.

[Primiano Tucci (use gerrit), 2017/02/17 14:51:04]

Done.

[Mark Mentovai, 2017/02/17 15:04:33]

Primiano Tucci wrote:

+
+#elif defined(ARCH_CPU_ARMEL)
+// bkpt will generate a SIGBUS when running on armv7 and a SIGTRAP when running
+// as a 32 bit userspace app on arm64. There doesn't seem to be any way to
+// cause a SIGTRAP from userspace without using a syscall (which would be a
+// problem for sandboxing).
+#define TRAP_INSTRUCTION() \
+  asm volatile("bkpt #0; udf %0;" ::"i"(__COUNTER__ % 256))
+
+#elif defined(ARCH_CPU_ARM64)
+
+// This will always generate a SIGTRAP on arm64.
+#define TRAP_INSTRUCTION() \
+  asm volatile("brk #0; hlt %0;" ::"i"(__COUNTER__ % 65536))
+#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();      \

[Mark Mentovai, 2017/02/17 14:24:28]

nit: these are more TRAP_SEQUENCE now that none are really single instructions.

[Primiano Tucci (use gerrit), 2017/02/17 14:51:04]

good point, renamed.

+    __builtin_unreachable(); \
+  })
+
 #elif defined(COMPILER_MSVC)
 #define IMMEDIATE_CRASH() __debugbreak()
 #else