Chromium Code Reviews
chromiumcodereview-hr@appspot.gserviceaccount.com (chromiumcodereview-hr) | Please choose your nickname with Settings | Help | Chromium Project | Gerrit Changes | Sign out
(853)

Unified Diff: base/logging.h

Issue 2502953003: base: make CHECK macros trap at distinct addresses in official builds (Closed)
Patch Set: use static_cast + primitive type Created 3 years, 10 months ago
Use n/p to move between diff chunks; N/P to move between comments. Draft comments are only viewable by you.
Jump to:
View side-by-side diff with in-line comments
Download patch
« no previous file with comments | « no previous file | base/logging_unittest.cc » ('j') | no next file with comments »
Expand Comments ('e') | Collapse Comments ('c') | Show Comments Hide Comments ('s')
Index: base/logging.h
diff --git a/base/logging.h b/base/logging.h
index 6f30bcbe4f736f989bad178ac3d96ed70bf08943..0417aae18c9740cb4f3bb218063e5f502e01c2e4 100644
--- a/base/logging.h
+++ b/base/logging.h
@@ -496,9 +496,60 @@ 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_SEQUENCE() \
+ asm volatile( \
+ "int3; ud2; push %0;" ::"i"(static_cast<unsigned char>(__COUNTER__)))
+
+#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_SEQUENCE() \
+ asm volatile("bkpt #0; udf %0;" ::"i"(__COUNTER__ % 256))
+
+#elif defined(ARCH_CPU_ARM64)
+
+// This will always generate a SIGTRAP on arm64.
+#define TRAP_SEQUENCE() \
+ 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_SEQUENCE() __builtin_trap()
+#endif
+
+#define IMMEDIATE_CRASH() \
+ ({ \
+ TRAP_SEQUENCE(); \
+ __builtin_unreachable(); \
+ })
+
#elif defined(COMPILER_MSVC)
#define IMMEDIATE_CRASH() __debugbreak()
#else
« no previous file with comments | « no previous file | base/logging_unittest.cc » ('j') | no next file with comments »

Powered by Google App Engine
This is Rietveld 408576698