Reland "Add test to verify the opcodes generated by IMMEDIATE_CRASH()."

This is a reland of 28eb992f

Fixes:
- Disable sanitizers in the test helper. The test is intended to test
  baseline behavior with max optimizations/undefined behavior, and the
  assumption is that sanitized builds will, in fact, be saner.
- Relax the nonce check, as the compiler can reorder IMMEDIATE_CRASH()
  blocks in the function epilogue as it wants.

Original change's description:
> Add test to verify the opcodes generated by IMMEDIATE_CRASH().
>
> Bug: 958675
> Change-Id: Ica6b5bab7849f227567d8016354a39383533799f
> Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1595585
> Commit-Queue: Daniel Cheng <dcheng@chromium.org>
> Reviewed-by: Nico Weber <thakis@chromium.org>
> Cr-Commit-Position: refs/heads/master@{#658344}

Bug: 958675
Change-Id: Ic1af1994909d58427cc12f1f9aee2765aa5b449c
Reviewed-on: https://chromium-review.googlesource.com/c/chromium/src/+/1604487
Commit-Queue: Nico Weber <thakis@chromium.org>
Reviewed-by: Nico Weber <thakis@chromium.org>
Cr-Commit-Position: refs/heads/master@{#658523}

base/BUILD.gn

@@ -311,6 +311,7 @@ jumbo_component("base") {
     "hash/md5.h",
     "hash/sha1.cc",
     "hash/sha1.h",
+    "immediate_crash.h",
     "ios/block_types.h",
     "ios/crb_protocol_observers.h",
     "ios/crb_protocol_observers.mm",
@@ -2472,6 +2473,7 @@ test("base_unittests") {
     "i18n/string_search_unittest.cc",
     "i18n/time_formatting_unittest.cc",
     "i18n/timezone_unittest.cc",
+    "immediate_crash_unittest.cc",
     "ios/crb_protocol_observers_unittest.mm",
     "ios/device_util_unittest.mm",
     "ios/weak_nsobject_unittest.mm",
@@ -2774,6 +2776,7 @@ test("base_unittests") {
   ]
 
   data_deps = [
+    "//base/test:immediate_crash_test_helper",
     "//base/test:test_child_process",
     "//base/test:test_shared_library",
   ]

base/immediate_crash.h

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#ifndef BASE_IMMEDIATE_CRASH_H_
+#define BASE_IMMEDIATE_CRASH_H_
+
+#include "build/build_config.h"
+
+// 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)
+
+#if defined(ARCH_CPU_X86_FAMILY) && !defined(OS_NACL)
+// 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) && !defined(OS_NACL)
+// 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) && !defined(OS_NACL)
+// 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  // ARCH_CPU_*
+
+#elif defined(COMPILER_MSVC)
+
+// Clang is cleverer about coalescing int3s, so we need to add a unique-ish
+// instruction following the __debugbreak() to have it emit distinct locations
+// for CHECKs rather than collapsing them all together. It would be nice to use
+// a short intrinsic to do this (and perhaps have only one implementation for
+// both clang and MSVC), however clang-cl currently does not support intrinsics.
+// On the flip side, MSVC x64 doesn't support inline asm. So, we have to have
+// two implementations. Normally clang-cl's version will be 5 bytes (1 for
+// `int3`, 2 for `ud2`, 2 for `push byte imm`, however, TODO(scottmg):
+// https://crbug.com/694670 clang-cl doesn't currently support %'ing
+// __COUNTER__, so eventually it will emit the dword form of push.
+// TODO(scottmg): Reinvestigate a short sequence that will work on both
+// compilers once clang supports more intrinsics. See https://crbug.com/693713.
+#if !defined(__clang__)
+#define TRAP_SEQUENCE() __debugbreak()
+#elif defined(ARCH_CPU_ARM64)
+#define TRAP_SEQUENCE() \
+  __asm volatile("brk #0\n hlt %0\n" ::"i"(__COUNTER__ % 65536));
+#else
+#define TRAP_SEQUENCE() ({ {__asm int 3 __asm ud2 __asm push __COUNTER__}; })
+#endif  // __clang__
+
+#else
+#error Port
+#endif  // COMPILER_GCC
+
+// CHECK() and the trap sequence can be invoked from a constexpr function.
+// This could make compilation fail on GCC, as it forbids directly using inline
+// asm inside a constexpr function. However, it allows calling a lambda
+// expression including the same asm.
+// The side effect is that the top of the stacktrace will not point to the
+// calling function, but to this anonymous lambda. This is still useful as the
+// full name of the lambda will typically include the name of the function that
+// calls CHECK() and the debugger will still break at the right line of code.
+#if !defined(COMPILER_GCC)
+#define WRAPPED_TRAP_SEQUENCE() TRAP_SEQUENCE()
+#else
+#define WRAPPED_TRAP_SEQUENCE() \
+  do {                          \
+    [] { TRAP_SEQUENCE(); }();  \
+  } while (false)
+#endif
+
+#if defined(__clang__) || defined(COMPILER_GCC)
+#define IMMEDIATE_CRASH()    \
+  ({                         \
+    WRAPPED_TRAP_SEQUENCE(); \
+    __builtin_unreachable(); \
+  })
+#else
+// This is supporting non-chromium user of logging.h to build with MSVC, like
+// pdfium. On MSVC there is no __builtin_unreachable().
+#define IMMEDIATE_CRASH() WRAPPED_TRAP_SEQUENCE()
+#endif
+
+#endif  // BASE_IMMEDIATE_CRASH_H_

base/immediate_crash_unittest.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/immediate_crash.h"
+
+#include <stdint.h>
+
+#include <algorithm>
+
+#include "base/containers/span.h"
+#include "base/files/file_path.h"
+#include "base/native_library.h"
+#include "base/optional.h"
+#include "base/strings/string_number_conversions.h"
+#include "build/build_config.h"
+#include "testing/gtest/include/gtest/gtest.h"
+
+namespace base {
+
+// iOS is excluded, since it doesn't support loading shared libraries.
+#if defined(OS_WIN) || (defined(OS_MACOSX) && !defined(OS_IOS)) ||      \
+    defined(OS_LINUX) || defined(OS_ANDROID) || defined(OS_CHROMEOS) || \
+    defined(OS_FUCHSIA)
+
+// Checks that the IMMEDIATE_CRASH() macro produces specific instructions; see
+// comments in immediate_crash.h for the requirements.
+TEST(ImmediateCrashTest, ExpectedOpcodeSequence) {
+  // TestFunction1() and TestFunction2() are defined in a shared library in an
+  // attempt to guarantee that they are located next to each other.
+  NativeLibraryLoadError load_error;
+  // TODO(dcheng): Shouldn't GetNativeLibraryName just return a FilePath?
+  NativeLibrary helper_library = LoadNativeLibrary(
+      FilePath::FromUTF8Unsafe(
+          GetNativeLibraryName("immediate_crash_test_helper")),
+      &load_error);
+  ASSERT_TRUE(helper_library)
+      << "shared library load failed: " << load_error.ToString();
+
+  // TestFunction1() and TestFunction2() each contain two IMMEDIATE_CRASH()
+  // invocations. IMMEDIATE_CRASH() should be treated as a noreturn sequence and
+  // optimized into the function epilogue. The general strategy is to find the
+  // return opcode, then scan the following bytes for the opcodes for two
+  // consecutive IMMEDIATE_CRASH() sequences.
+  void* a =
+      GetFunctionPointerFromNativeLibrary(helper_library, "TestFunction1");
+  ASSERT_TRUE(a);
+  void* b =
+      GetFunctionPointerFromNativeLibrary(helper_library, "TestFunction2");
+  ASSERT_TRUE(b);
+
+#if defined(ARCH_CPU_X86_FAMILY)
+
+  // X86 opcode reference:
+  // https://software.intel.com/en-us/download/intel-64-and-ia-32-architectures-sdm-combined-volumes-1-2a-2b-2c-2d-3a-3b-3c-3d-and-4
+  span<const uint8_t> function_body =
+      a < b ? make_span(static_cast<const uint8_t*>(a),
+                        static_cast<const uint8_t*>(b))
+            : make_span(static_cast<const uint8_t*>(b),
+                        static_cast<const uint8_t*>(a));
+  SCOPED_TRACE(HexEncode(function_body.data(), function_body.size_bytes()));
+
+  // Look for RETN opcode (0xC3). Note that 0xC3 is a substring of several
+  // other opcodes (VMRESUME, MOVNTI), and can also be encoded as part of an
+  // argument to another opcode. None of these other cases are expected to be
+  // present, so a simple byte scan should be Good Enough™.
+  auto it = std::find(function_body.begin(), function_body.end(), 0xC3);
+  ASSERT_NE(function_body.end(), it) << "Failed to find return! ";
+
+  // Look for two IMMEDIATE_CRASH() opcode sequences.
+  base::Optional<uint8_t> nonce;
+  for (int i = 0; i < 2; ++i) {
+    // INT 3
+    EXPECT_EQ(0xCC, *++it);
+    // UD2
+    EXPECT_EQ(0x0F, *++it);
+    EXPECT_EQ(0x0B, *++it);
+    // PUSH
+    EXPECT_EQ(0x6A, *++it);
+    // Immediate nonce argument to PUSH
+    if (!nonce) {
+      nonce = *++it;
+    } else {
+      EXPECT_NE(*nonce, *++it);
+    }
+#if (defined(OS_WIN) && defined(ARCH_CPU_64_BITS)) || defined(OS_MACOSX)
+    // On Windows x64 and Mac, __builtin_unreachable() generates UD2. See
+    // https://crbug.com/958373.
+    EXPECT_EQ(0x0F, *++it);
+    EXPECT_EQ(0x0B, *++it);
+#endif  // defined(OS_WIN) || defined(OS_MACOSX)
+  }
+
+#elif defined(ARCH_CPU_ARMEL)
+
+  // Routines loaded from a shared library will have the LSB in the pointer set
+  // if encoded as T32 instructions. The rest of this test assumes T32.
+  ASSERT_TRUE(reinterpret_cast<uintptr_t>(a) & 0x1)
+      << "Expected T32 opcodes but found A32 opcodes instead.";
+  ASSERT_TRUE(reinterpret_cast<uintptr_t>(b) & 0x1)
+      << "Expected T32 opcodes but found A32 opcodes instead.";
+
+  // Mask off the lowest bit.
+  a = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(a) & ~uintptr_t{0x1});
+  b = reinterpret_cast<void*>(reinterpret_cast<uintptr_t>(b) & ~uintptr_t{0x1});
+
+  // T32 opcode reference: https://developer.arm.com/docs/ddi0487/latest
+  span<const uint16_t> function_body =
+      a < b ? make_span(static_cast<const uint16_t*>(a),
+                        static_cast<const uint16_t*>(b))
+            : make_span(static_cast<const uint16_t*>(b),
+                        static_cast<const uint16_t*>(a));
+  SCOPED_TRACE(HexEncode(function_body.data(), function_body.size_bytes()));
+
+  // Look for the standard return opcode sequence (BX LR).
+  auto it = std::find(function_body.begin(), function_body.end(), 0x4770);
+  ASSERT_NE(function_body.end(), it) << "Failed to find return! ";
+
+  // Look for two IMMEDIATE_CRASH() opcode sequences.
+  base::Optional<uint8_t> nonce;
+  for (int i = 0; i < 2; ++i) {
+    // BKPT #0
+    EXPECT_EQ(0xBE00, *++it);
+    // UDF #<nonce>
+    EXPECT_EQ(0xDE00, *++it & 0xFF00);
+    if (!nonce) {
+      nonce = *it & 0x00FF;
+    } else {
+      EXPECT_NE(*nonce, *it & 0x00FF);
+    }
+  }
+
+#elif defined(ARCH_CPU_ARM64)
+
+  // A64 opcode reference: https://developer.arm.com/docs/ddi0487/latest
+  span<const uint32_t> function_body =
+      a < b ? make_span(static_cast<const uint32_t*>(a),
+                        static_cast<const uint32_t*>(b))
+            : make_span(static_cast<const uint32_t*>(b),
+                        static_cast<const uint32_t*>(a));
+  SCOPED_TRACE(HexEncode(function_body.data(), function_body.size_bytes()));
+
+  // Look for RET. There appears to be multiple valid encodings, so this is
+  // hardcoded to whatever clang currently emits...
+  auto it = std::find(function_body.begin(), function_body.end(), 0XD65F03C0);
+  ASSERT_NE(function_body.end(), it) << "Failed to find return! ";
+
+  // Look for two IMMEDIATE_CRASH() opcode sequences.
+  base::Optional<uint16_t> nonce;
+  for (int i = 0; i < 2; ++i) {
+    // BRK #0
+    EXPECT_EQ(0XD4200000, *++it);
+    // HLT #<nonce>
+    EXPECT_EQ(0xD4400000, *++it & 0xFFE00000);
+    if (!nonce) {
+      nonce = (*it >> 5) & 0xFFFF;
+    } else {
+      EXPECT_NE(*nonce, (*it >> 5) & 0xFFFF);
+    }
+  }
+
+#endif  // defined(ARCH_CPU_X86_FAMILY)
+
+  UnloadNativeLibrary(helper_library);
+}
+
+#endif
+
+}  // namespace base

base/logging.h

@@ -18,6 +18,7 @@
 #include "base/callback_forward.h"
 #include "base/compiler_specific.h"
 #include "base/debug/debugger.h"
+#include "base/immediate_crash.h"
 #include "base/logging_buildflags.h"
 #include "base/macros.h"
 #include "base/scoped_clear_last_error.h"
@@ -526,110 +527,6 @@ class CheckOpResult {
   std::string* message_;
 };
 
-// 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)
-
-#if defined(ARCH_CPU_X86_FAMILY) && !defined(OS_NACL)
-// 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) && !defined(OS_NACL)
-// 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) && !defined(OS_NACL)
-// 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  // ARCH_CPU_*
-
-#elif defined(COMPILER_MSVC)
-
-// Clang is cleverer about coalescing int3s, so we need to add a unique-ish
-// instruction following the __debugbreak() to have it emit distinct locations
-// for CHECKs rather than collapsing them all together. It would be nice to use
-// a short intrinsic to do this (and perhaps have only one implementation for
-// both clang and MSVC), however clang-cl currently does not support intrinsics.
-// On the flip side, MSVC x64 doesn't support inline asm. So, we have to have
-// two implementations. Normally clang-cl's version will be 5 bytes (1 for
-// `int3`, 2 for `ud2`, 2 for `push byte imm`, however, TODO(scottmg):
-// https://crbug.com/694670 clang-cl doesn't currently support %'ing
-// __COUNTER__, so eventually it will emit the dword form of push.
-// TODO(scottmg): Reinvestigate a short sequence that will work on both
-// compilers once clang supports more intrinsics. See https://crbug.com/693713.
-#if !defined(__clang__)
-#define TRAP_SEQUENCE() __debugbreak()
-#elif defined(ARCH_CPU_ARM64)
-#define TRAP_SEQUENCE() \
-  __asm volatile("brk #0\n hlt %0\n" ::"i"(__COUNTER__ % 65536));
-#else
-#define TRAP_SEQUENCE() ({ {__asm int 3 __asm ud2 __asm push __COUNTER__}; })
-#endif  // __clang__
-
-#else
-#error Port
-#endif  // COMPILER_GCC
-
-// CHECK() and the trap sequence can be invoked from a constexpr function.
-// This could make compilation fail on GCC, as it forbids directly using inline
-// asm inside a constexpr function. However, it allows calling a lambda
-// expression including the same asm.
-// The side effect is that the top of the stacktrace will not point to the
-// calling function, but to this anonymous lambda. This is still useful as the
-// full name of the lambda will typically include the name of the function that
-// calls CHECK() and the debugger will still break at the right line of code.
-#if !defined(COMPILER_GCC)
-#define WRAPPED_TRAP_SEQUENCE() TRAP_SEQUENCE()
-#else
-#define WRAPPED_TRAP_SEQUENCE() \
-  do {                          \
-    [] { TRAP_SEQUENCE(); }();  \
-  } while (false)
-#endif
-
-#if defined(__clang__) || defined(COMPILER_GCC)
-#define IMMEDIATE_CRASH()    \
-  ({                         \
-    WRAPPED_TRAP_SEQUENCE(); \
-    __builtin_unreachable(); \
-  })
-#else
-// This is supporting non-chromium user of logging.h to build with MSVC, like
-// pdfium. On MSVC there is no __builtin_unreachable().
-#define IMMEDIATE_CRASH() WRAPPED_TRAP_SEQUENCE()
-#endif
-
 // CHECK dies with a fatal error if condition is not true.  It is *not*
 // controlled by NDEBUG, so the check will be executed regardless of
 // compilation mode.

base/test/BUILD.gn

@@ -349,6 +349,31 @@ source_set("native_library_test_utils") {
   ]
 }
 
+# This shared library is dynamically loaded by ImmediateCrash unittests.
+shared_library("immediate_crash_test_helper") {
+  sources = [
+    "immediate_crash_test_helper.cc",
+  ]
+
+  # Note: the helper has a header-only dependency on //base/immediate_helper.h.
+  # However, the build rule intentionally omits an explicit //base dependency
+  # to avoid potential ODR violations and minimize the amount of code linked in.
+
+  # Try to minimize the risk of non-official builds generating different code.
+  if (!is_official_build) {
+    configs -= [ "//build/config/compiler:default_optimization" ]
+    configs += [ "//build/config/compiler:optimize_max" ]
+  }
+
+  # Disable sanitization: sanitized builds are assumed to be saner than normal,
+  # and can affect codegen in surprising ways, which breaks the tests.
+  configs -= [ "//build/config/sanitizers:default_sanitizer_flags" ]
+
+  if (is_android) {
+    configs -= [ "//build/config/android:hide_all_but_jni_onload" ]
+  }
+}
+
 # This shared library is dynamically loaded by NativeLibrary unittests.
 shared_library("test_shared_library") {
   testonly = true

base/test/immediate_crash_test_helper.cc

+// Copyright 2019 The Chromium Authors. All rights reserved.
+// Use of this source code is governed by a BSD-style license that can be
+// found in the LICENSE file.
+
+#include "base/immediate_crash.h"  // nogncheck
+
+#if defined(WIN32)
+#define IMMEDIATE_CRASH_TEST_HELPER_EXPORT __declspec(dllexport)
+#else  // defined(WIN32)
+#define IMMEDIATE_CRASH_TEST_HELPER_EXPORT \
+  __attribute__((visibility("default")))
+#endif  // defined(WIN32)
+
+extern "C" {
+
+IMMEDIATE_CRASH_TEST_HELPER_EXPORT int TestFunction1(int x, int y) {
+  if (x < 1)
+    IMMEDIATE_CRASH();
+  if (y < 1)
+    IMMEDIATE_CRASH();
+  return x + y;
+}
+
+IMMEDIATE_CRASH_TEST_HELPER_EXPORT int TestFunction2(int x, int y) {
+  if (x < 2)
+    IMMEDIATE_CRASH();
+  if (y < 2)
+    IMMEDIATE_CRASH();
+  return x * y;
+}
+
+}  // extern "C"