Bug 1574388 - Implement PHC on Mac. r=gsvelto,glandium
authorNicholas Nethercote <nnethercote@mozilla.com>
Sun, 25 Aug 2019 23:16:05 +0000
changeset 553566 1429658f80ca4dc9d2e02fbfa9fdafe7e10b1393
parent 553565 b72d1d6131cce6845af6ffd6f83de7f993930961
child 553567 8a7e48b3e117939cab046dac642c601e8b2eb4ef
push id2165
push userffxbld-merge
push dateMon, 14 Oct 2019 16:30:58 +0000
treeherdermozilla-release@0eae18af659f [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersgsvelto, glandium
bugs1574388, 1576515
milestone70.0a1
first release with
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
last release without
nightly linux32
nightly linux64
nightly mac
nightly win32
nightly win64
Bug 1574388 - Implement PHC on Mac. r=gsvelto,glandium But it is not yet enabled; bug 1576515 will do that. Differential Revision: https://phabricator.services.mozilla.com/D42264
build/moz.configure/memory.configure
memory/replace/phc/PHC.cpp
toolkit/crashreporter/breakpad-client/mac/handler/exception_handler.cc
toolkit/crashreporter/breakpad-client/mac/handler/moz.build
toolkit/crashreporter/test/unit/test_crash_phc.js
toolkit/crashreporter/test/unit_ipc/test_content_phc.js
toolkit/crashreporter/test/unit_ipc/test_content_phc2.js
--- a/build/moz.configure/memory.configure
+++ b/build/moz.configure/memory.configure
@@ -57,16 +57,19 @@ def phc_default(milestone, target, repla
     if not replace_malloc_default or \
        (replace_malloc.origin != 'default' and not replace_malloc):
         return False
     # Nightly only because PHC has a non-negligible performance cost.
     if not milestone.is_nightly:
         return False
     # Both Linux32 and Win32 have frequent crashes when stack tracing (for
     # unclear reasons), so PHC is enabled only on 64-bit only in both cases.
+    #
+    # XXX: PHC is implemented but not yet enabled on Mac. Bug 1576515 is about
+    #      enabling it on Mac, but it is blocked by bug 1035892.
     return (target.os == 'GNU' and target.kernel == 'Linux' and
             target.bitness == 64) or \
            (target.kernel == 'WINNT' and target.bitness == 64)
 
 
 option('--enable-phc', env='MOZ_PHC', default=phc_default,
        when='--enable-jemalloc',
        help='{Enable|Disable} PHC (Probabilistic Memory Checker). '
--- a/memory/replace/phc/PHC.cpp
+++ b/memory/replace/phc/PHC.cpp
@@ -86,16 +86,17 @@
 #include "mozilla/Attributes.h"
 #include "mozilla/CheckedInt.h"
 #include "mozilla/Maybe.h"
 #include "mozilla/StackWalk.h"
 #include "mozilla/ThreadLocal.h"
 #include "mozilla/XorShift128PlusRNG.h"
 
 using namespace mozilla;
+using namespace mozilla::recordreplay;
 
 //---------------------------------------------------------------------------
 // Utilities
 //---------------------------------------------------------------------------
 
 #ifdef ANDROID
 // Android doesn't have pthread_atfork defined in pthread.h.
 extern "C" MOZ_EXPORT int pthread_atfork(void (*)(void), void (*)(void),
@@ -321,26 +322,26 @@ class GAtomic {
   static int32_t DecrementDelay() { return --sAllocDelay; }
 
   static void SetAllocDelay(Delay aAllocDelay) { sAllocDelay = aAllocDelay; }
 
  private:
   // The current time. Relaxed semantics because it's primarily used for
   // determining if an allocation can be recycled yet and therefore it doesn't
   // need to be exact.
-  static Atomic<Time, Relaxed> sNow;
+  static Atomic<Time, Relaxed, Behavior::DontPreserve> sNow;
 
   // Delay until the next attempt at a page allocation. See the comment in
   // MaybePageAlloc() for an explanation of why it is a signed integer, and why
   // it uses ReleaseAcquire semantics.
-  static Atomic<Delay, ReleaseAcquire> sAllocDelay;
+  static Atomic<Delay, ReleaseAcquire, Behavior::DontPreserve> sAllocDelay;
 };
 
-Atomic<Time, Relaxed> GAtomic::sNow;
-Atomic<Delay, ReleaseAcquire> GAtomic::sAllocDelay;
+Atomic<Time, Relaxed, Behavior::DontPreserve> GAtomic::sNow;
+Atomic<Delay, ReleaseAcquire, Behavior::DontPreserve> GAtomic::sAllocDelay;
 
 // Shared, immutable global state. Initialized by replace_init() and never
 // changed after that. replace_init() runs early enough that no synchronization
 // is needed.
 class GConst {
  private:
   // The bounds of the allocated pages.
   const uintptr_t mPagesStart;
--- a/toolkit/crashreporter/breakpad-client/mac/handler/exception_handler.cc
+++ b/toolkit/crashreporter/breakpad-client/mac/handler/exception_handler.cc
@@ -36,16 +36,20 @@
 #include <map>
 
 #include "mac/handler/exception_handler.h"
 #include "mac/handler/minidump_generator.h"
 #include "common/mac/macho_utilities.h"
 #include "common/mac/scoped_task_suspend-inl.h"
 #include "google_breakpad/common/minidump_exception_mac.h"
 
+#ifdef MOZ_PHC
+#include "replace_malloc_bridge.h"
+#endif
+
 #include "mozilla/RecordReplay.h"
 
 #ifndef __EXCEPTIONS
 // This file uses C++ try/catch (but shouldn't). Duplicate the macros from
 // <c++/4.2.1/exception_defines.h> allowing this file to work properly with
 // exceptions disabled even when other C++ libraries are used. #undef the try
 // and catch macros first in case libstdc++ is in use and has already provided
 // its own definitions.
@@ -343,46 +347,64 @@ bool ExceptionHandler::WriteMinidumpForC
 
   if (callback) {
     return callback(dump_path.c_str(), dump_id.c_str(),
                     callback_context, nullptr, result);
   }
   return result;
 }
 
+#ifdef MOZ_PHC
+static void GetPHCAddrInfo(int64_t exception_subcode,
+                           mozilla::phc::AddrInfo* addr_info) {
+  // Is this a crash involving a PHC allocation?
+  if (exception_subcode) {
+    // `exception_subcode` is only non-zero when it's a bad access, in which
+    // case it holds the address of the bad access.
+    char* addr = reinterpret_cast<char*>(exception_subcode);
+    ReplaceMalloc::IsPHCAllocation(addr, addr_info);
+  }
+}
+#endif
+
 bool ExceptionHandler::WriteMinidumpWithException(
     int exception_type,
     int exception_code,
     int exception_subcode,
     breakpad_ucontext_t* task_context,
     mach_port_t thread_name,
     bool exit_after_write,
     bool report_current_thread) {
   bool result = false;
 
 #if TARGET_OS_IPHONE
   // _exit() should never be called on iOS.
   exit_after_write = false;
 #endif
 
+    mozilla::phc::AddrInfo addr_info;
+#ifdef MOZ_PHC
+    GetPHCAddrInfo(exception_subcode, &addr_info);
+#endif
+
   if (directCallback_) {
     if (directCallback_(callback_context_,
                         exception_type,
                         exception_code,
                         exception_subcode,
                         thread_name) ) {
       if (exit_after_write)
         _exit(exception_type);
     }
 #if !TARGET_OS_IPHONE
   } else if (IsOutOfProcess()) {
     if (exception_type && exception_code) {
       // If this is a real exception, give the filter (if any) a chance to
       // decide if this should be sent.
-      if (filter_ && !filter_(callback_context_, nullptr))
+      if (filter_ && !filter_(callback_context_, &addr_info))
         return false;
       result = crash_generation_client_->RequestDumpForException(
           exception_type,
           exception_code,
           exception_subcode,
           thread_name);
       if (result && exit_after_write) {
         _exit(exception_type);
@@ -413,17 +435,17 @@ bool ExceptionHandler::WriteMinidumpWith
     }
 
     // Call user specified callback (if any)
     if (callback_) {
       // If the user callback returned true and we're handling an exception
       // (rather than just writing out the file), then we should exit without
       // forwarding the exception to the next handler.
       if (callback_(dump_path_c_, next_minidump_id_c_, callback_context_,
-                    nullptr, result)) {
+                    &addr_info, result)) {
         if (exit_after_write)
           _exit(exception_type);
       }
     }
   }
 
   return result;
 }
--- a/toolkit/crashreporter/breakpad-client/mac/handler/moz.build
+++ b/toolkit/crashreporter/breakpad-client/mac/handler/moz.build
@@ -13,8 +13,10 @@ UNIFIED_SOURCES += [
 
 FINAL_LIBRARY = 'breakpad_client'
 
 LOCAL_INCLUDES += [
     '/toolkit/crashreporter/breakpad-client',
     '/toolkit/crashreporter/google-breakpad/src',
 ]
 
+if CONFIG['MOZ_PHC']:
+    DEFINES['MOZ_PHC'] = True
--- a/toolkit/crashreporter/test/unit/test_crash_phc.js
+++ b/toolkit/crashreporter/test/unit/test_crash_phc.js
@@ -2,18 +2,19 @@ function check(extra, size) {
   Assert.equal(extra.PHCKind, "FreedPage");
 
   // This is a string holding a decimal address.
   Assert.ok(/^\d+$/.test(extra.PHCBaseAddress));
 
   Assert.equal(extra.PHCUsableSize, size);
 
   // These are strings holding comma-separated lists of decimal addresses.
-  Assert.ok(/^(\d+,)+\d+$/.test(extra.PHCAllocStack));
-  Assert.ok(/^(\d+,)+\d+$/.test(extra.PHCFreeStack));
+  // Sometimes on Mac they have a single entry.
+  Assert.ok(/^(\d+,)*\d+$/.test(extra.PHCAllocStack));
+  Assert.ok(/^(\d+,)*\d+$/.test(extra.PHCFreeStack));
 }
 
 function run_test() {
   if (!("@mozilla.org/toolkit/crash-reporter;1" in Cc)) {
     dump(
       "INFO | test_crash_phc.js | Can't test crashreporter in a non-libxul build.\n"
     );
     return;
--- a/toolkit/crashreporter/test/unit_ipc/test_content_phc.js
+++ b/toolkit/crashreporter/test/unit_ipc/test_content_phc.js
@@ -18,13 +18,14 @@ function run_test() {
 
       // This is a string holding a decimal address.
       Assert.ok(/^\d+$/.test(extra.PHCBaseAddress));
 
       // CRASH_PHC_USE_AFTER_FREE uses 32 for the size.
       Assert.equal(extra.PHCUsableSize, 32);
 
       // These are strings holding comma-separated lists of decimal addresses.
-      Assert.ok(/^(\d+,)+\d+$/.test(extra.PHCAllocStack));
-      Assert.ok(/^(\d+,)+\d+$/.test(extra.PHCFreeStack));
+      // Sometimes on Mac they have a single entry.
+      Assert.ok(/^(\d+,)*\d+$/.test(extra.PHCAllocStack));
+      Assert.ok(/^(\d+,)*\d+$/.test(extra.PHCFreeStack));
     }
   );
 }
--- a/toolkit/crashreporter/test/unit_ipc/test_content_phc2.js
+++ b/toolkit/crashreporter/test/unit_ipc/test_content_phc2.js
@@ -21,13 +21,14 @@ function run_test() {
 
       // This is a string holding a decimal address.
       Assert.ok(/^\d+$/.test(extra.PHCBaseAddress));
 
       // CRASH_PHC_DOUBLE_FREE uses 64 for the size.
       Assert.equal(extra.PHCUsableSize, 64);
 
       // These are strings holding comma-separated lists of decimal addresses.
-      Assert.ok(/^(\d+,)+\d+$/.test(extra.PHCAllocStack));
-      Assert.ok(/^(\d+,)+\d+$/.test(extra.PHCFreeStack));
+      // Sometimes on Mac they have a single entry.
+      Assert.ok(/^(\d+,)*\d+$/.test(extra.PHCAllocStack));
+      Assert.ok(/^(\d+,)*\d+$/.test(extra.PHCFreeStack));
     }
   );
 }