Bug 1451268: RematerializedFrames may not be cached, even when younger frames are. r=jorendorff
authorJim Blandy <jimb@mozilla.com>
Sat, 11 May 2019 02:17:04 +0000
changeset 532317 5b35987c1ac477956899f2cb929218e019db20e0
parent 532316 af805180a1e6329754d626a62d17bdca87ffc2b0
child 532318 a07d7f8f53ca2bf81e5650a499481793c9b89fce
push id11265
push userffxbld-merge
push dateMon, 13 May 2019 10:53:39 +0000
treeherdermozilla-beta@77e0fe8dbdd3 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersjorendorff
bugs1451268
milestone68.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 1451268: RematerializedFrames may not be cached, even when younger frames are. r=jorendorff If the Debugger API tries to inspect or modify an IonMonkey frame, much of the information it expects to find in a frame is missing: function calls may have been inlined, variables may have been optimized out, and so on. So when this happens, SpiderMonkey builds one or more Rematerialized frames from the IonMonkey frame, using metadata built by Ion to reconstruct the missing parts. The Rematerialized frames are now the authority on the state of those frames, and the Ion frame is ignored: stack iterators ignore the Ion frame, producing the Rematerialized frames in their stead; and when control returns to the Ion frame, we pop it, rebuild Baseline frames from the Rematerialized frames, and resume execution in Baseline. Thus, Rematerialized frames are always created with their hasCachedSavedFrame bits clear: although there may be extant SavedFrames built from the original IonMonkey frame, the Rematerialized frames will not have cache entries for them until they are traversed in a capture themselves. This means that, oddly, it is not always true that, once we reach a frame with its hasCachedSavedFrame bit set, all its parents will have the bit set as well. However, clear bits under younger set bits will only occur on Rematerialized frames. Differential Revision: https://phabricator.services.mozilla.com/D29785
js/src/jit-test/tests/saved-stacks/bug-1451268.js
js/src/jit/RematerializedFrame.h
js/src/vm/SavedStacks.cpp
js/src/vm/Stack.h
new file mode 100644
--- /dev/null
+++ b/js/src/jit-test/tests/saved-stacks/bug-1451268.js
@@ -0,0 +1,23 @@
+// |jit-test| --no-threads; --ion-eager
+// Walking into Rematerialized frames under ordinary frames with their
+// hasCachedSavedFrame bits set shouldn't cause an assertion.
+
+enableTrackAllocations();
+var g = newGlobal({ newCompartment: true });
+var dbg = new Debugger;
+g.toggle = function toggle(x, d) {
+    if (d) {
+        dbg.addDebuggee(g);
+        var frame = dbg.getNewestFrame().older;
+    }
+};
+g.eval("" + function f(x, d) {
+    g(x, d);
+});
+g.eval("" + function g(x, d) {
+    toggle(x, d);
+});
+g.eval("(" + function test() {
+    for (var i = 0; i < 5; i++) f(42, false);
+    f(42, true);
+} + ")();");
--- a/js/src/jit/RematerializedFrame.h
+++ b/js/src/jit/RematerializedFrame.h
@@ -14,20 +14,29 @@
 #include "js/UniquePtr.h"
 #include "vm/EnvironmentObject.h"
 #include "vm/JSFunction.h"
 #include "vm/Stack.h"
 
 namespace js {
 namespace jit {
 
+// RematerializedFrame: An optimized frame that has been rematerialized with
+// values read out of Snapshots.
 //
-// An optimized frame that has been rematerialized with values read out of
-// Snapshots.
-//
+// If the Debugger API tries to inspect or modify an IonMonkey frame, much of
+// the information it expects to find in a frame is missing: function calls may
+// have been inlined, variables may have been optimized out, and so on. So when
+// this happens, SpiderMonkey builds one or more Rematerialized frames from the
+// IonMonkey frame, using the snapshot metadata built by Ion to reconstruct the
+// missing parts. The Rematerialized frames are now the authority on the state
+// of those frames, and the Ion frame is ignored: stack iterators ignore the Ion
+// frame, producing the Rematerialized frames in their stead; and when control
+// returns to the Ion frame, we pop it, rebuild Baseline frames from the
+// Rematerialized frames, and resume execution in Baseline.
 class RematerializedFrame {
   // See DebugScopes::updateLiveScopes.
   bool prevUpToDate_;
 
   // Propagated to the Baseline frame once this is popped.
   bool isDebuggee_;
 
   // Has an initial environment has been pushed on the environment chain for
--- a/js/src/vm/SavedStacks.cpp
+++ b/js/src/vm/SavedStacks.cpp
@@ -1417,17 +1417,20 @@ bool SavedStacks::insertFrames(JSContext
   DebugOnly<bool> seenCached = false;
 
   while (!iter.done()) {
     Activation& activation = *iter.activation();
     Maybe<LiveSavedFrameCache::FramePtr> framePtr =
         LiveSavedFrameCache::FramePtr::create(iter);
 
     if (framePtr) {
-      MOZ_ASSERT_IF(seenCached, framePtr->hasCachedSavedFrame());
+      // See the comment in Stack.h for why RematerializedFrames
+      // are a special case here.
+      MOZ_ASSERT_IF(seenCached, framePtr->hasCachedSavedFrame() ||
+                                    framePtr->isRematerializedFrame());
       seenCached |= framePtr->hasCachedSavedFrame();
     }
 
     if (capture.is<JS::AllFrames>() && framePtr &&
         framePtr->hasCachedSavedFrame()) {
       auto* cache = activation.getLiveSavedFrameCache(cx);
       if (!cache) {
         return false;
--- a/js/src/vm/Stack.h
+++ b/js/src/vm/Stack.h
@@ -1134,17 +1134,17 @@ namespace js {
 // has bits set on all its frames:
 //
 //     P* > Q* > T* > U*         All frames are now in the cache.
 //
 // And the cache again holds entries for the entire stack:
 //
 //     P  > Q  > T  > U
 //
-// Some details:
+// Details:
 //
 // - When we find a cache entry whose frame address matches our frame F, we know
 //   that F has never left the stack, but it may certainly be the case that
 //   execution took place in that frame, and that the current source position
 //   within F's function has changed. This means that the entry's SavedFrame,
 //   which records the source line and column as well as the function, is not
 //   correct. To detect this case, when we push a cache entry, we record the
 //   frame's pc. When consulting the cache, if a frame's address matches but its
@@ -1216,16 +1216,27 @@ namespace js {
 //   stack-like fashion: if a frame has its bit set, it has never left the
 //   stack. To support this assumption, when the cache is in use, we do not skip
 //   the frames between a debugger eval frame an its target; we always traverse
 //   the entire stack, invalidating and populating the cache in the usual way.
 //   Instead, when we construct a SavedFrame for a debugger eval frame, we
 //   select the appropriate parent at that point: rather than the next-older
 //   frame, we find the SavedFrame for the eval's target frame. The skip appears
 //   in the SavedFrame chains, even as the traversal covers all the frames.
+//
+// - Rematerialized frames (see ../jit/RematerializedFrame.h) are always created
+//   with their hasCachedSavedFrame bits clear: although there may be extant
+//   SavedFrames built from the original IonMonkey frame, the Rematerialized
+//   frames will not have cache entries for them until they are traversed in a
+//   capture themselves.
+//
+//   This means that, oddly, it is not always true that, once we reach a frame
+//   with its hasCachedSavedFrame bit set, all its parents will have the bit set
+//   as well. However, clear bits under younger set bits will only occur on
+//   Rematerialized frames.
 class LiveSavedFrameCache {
  public:
   // The address of a live frame for which we can cache SavedFrames: it has a
   // 'hasCachedSavedFrame' bit we can examine and set, and can be converted to
   // a Key to index the cache.
   class FramePtr {
     // We use jit::CommonFrameLayout for both Baseline frames and Ion
     // physical frames.
@@ -1258,16 +1269,21 @@ class LiveSavedFrameCache {
       return ptr.is<InterpreterFrame*>();
     }
 
     // If this FramePtr is an interpreter frame, return a pointer to it.
     inline InterpreterFrame& asInterpreterFrame() const {
       return *ptr.as<InterpreterFrame*>();
     }
 
+    // Return true if this FramePtr refers to a rematerialized frame.
+    inline bool isRematerializedFrame() const {
+      return ptr.is<jit::RematerializedFrame*>();
+    }
+
     bool operator==(const FramePtr& rhs) const { return rhs.ptr == this->ptr; }
     bool operator!=(const FramePtr& rhs) const { return !(rhs == *this); }
   };
 
  private:
   // A key in the cache: the address of a frame, live or dead, for which we
   // can cache SavedFrames. Since the pointer may not be live, the only
   // operation this type permits is comparison.