Commit ab022b52 authored by Iain Ireland's avatar Iain Ireland
Browse files

Bug 1761947: Visit stolen resume points in MNodeIterator r=jandem 

`EmulateStateOf<T>::run` uses `MNodeIterator` to iterate over the instructions in a block. `MNodeIterator` has some internal cleverness to visit the resume point attached to an instruction, unless that instruction has been discarded. In `visitApplyArray`, we steal the resume point from the applyArray, attach it to a new call, and discard the applyArray. Because the applyArray was discarded, MNodeIterator skips the resume point. We don't update the list of stores, so the recovery code allocates an empty array instead of an initialized array, causing us to get the wrong answer if we invalidate due to GC inside the replaced spread call.

This patch changes MNodeIterator to store the resume point instead of the instruction, which simplifies the code somewhat and seems more robust.

Differential Revision: https://phabricator.services.mozilla.com/D142579
parent 06487ec5

js/src/jit-test/tests/warp/bug1761947.js

0 → 100644
+28 −0
Original line number Diff line number Diff line 
let trigger = false;



function bar(x) {

  with ({}) {}

  if (trigger) {

    gc(foo, "shrinking");

    trigger = false;

  }

  return Object(x);

}



function foo() {

  let result = undefined;

  const arr = [8];

  for (var i = 0; i < 10; i++) {

    result = bar(...arr);

    assertEq(Number(result), 8);

  }

  return result;

}



with ({}) {}

for (var i = 0; i < 100; i++) {

  foo();

}



trigger = true;

foo();

js/src/jit/MIR.h

+4 −0
Original line number Diff line number Diff line
@@ -8449,6 +8449,7 @@ class MResumePoint final : public MNode 
  jsbytecode* pc_;

  MInstruction* instruction_;

  ResumeMode mode_;

  bool isDiscarded_ = false;



  MResumePoint(MBasicBlock* block, jsbytecode* pc, ResumeMode mode);

  void inherit(MBasicBlock* state);
@@ -8542,6 +8543,9 @@ class MResumePoint final : public MNode 
  MStoresToRecoverList::iterator storesBegin() const { return stores_.begin(); }

  MStoresToRecoverList::iterator storesEnd() const { return stores_.end(); }



  void setDiscarded() { isDiscarded_ = true; }

  bool isDiscarded() const { return isDiscarded_; }



#ifdef JS_JITSPEW

  virtual void dump(GenericPrinter& out) const override;

  virtual void dump() const override;

js/src/jit/MIRGraph.cpp

+1 −0
Original line number Diff line number Diff line
@@ -616,6 +616,7 @@ void MBasicBlock::discardResumePoint( 
  if (refType & RefType_DiscardOperands) {

    rp->releaseUses();

  }

  rp->setDiscarded();

#ifdef DEBUG

  MResumePointIterator iter = resumePointsBegin();

  while (*iter != rp) {

js/src/jit/MIRGraph.h

+21 −32
Original line number Diff line number Diff line
@@ -800,14 +800,15 @@ class MDefinitionIterator { 
};



// Iterates on all resume points, phis, and instructions of a MBasicBlock.

// Resume points are visited as long as the instruction which holds it is not

// discarded.

// Resume points are visited as long as they have not been discarded.

class MNodeIterator {

 private:

  // Last instruction which holds a resume point. To handle the entry point

  // resume point, it is set to the last instruction, assuming that the last

  // instruction is not discarded before we visit it.

  MInstruction* last_;

  // If this is non-null, the resume point that we will visit next (unless

  // it has been discarded). Initialized to the entry resume point.

  // Otherwise, resume point of the most recently visited instruction.

  MResumePoint* resumePoint_;



  mozilla::DebugOnly<MInstruction*> lastInstruction_ = nullptr;



  // Definition iterator which is one step ahead when visiting resume points.

  // This is in order to avoid incrementing the iterator while it is settled
@@ -816,35 +817,29 @@ class MNodeIterator { 


  MBasicBlock* block() const { return defIter_.block_; }



  bool atResumePoint() const { return last_ && !last_->isDiscarded(); }



  MNode* getNode() {

    if (!atResumePoint()) {

      return *defIter_;

  bool atResumePoint() const {

    MOZ_ASSERT_IF(lastInstruction_ && !lastInstruction_->isDiscarded(),

                  lastInstruction_->resumePoint() == resumePoint_);

    return resumePoint_ && !resumePoint_->isDiscarded();

  }



    // We use the last instruction as a sentinelle to iterate over the entry

    // resume point of the basic block, before even starting to iterate on

    // the instruction list.  Otherwise, the last_ corresponds to the

    // previous instruction.

    if (last_ != block()->lastIns()) {

      return last_->resumePoint();

  MNode* getNode() {

    if (atResumePoint()) {

      return resumePoint_;

    }

    return block()->entryResumePoint();

    return *defIter_;

  }



  void next() {

    if (!atResumePoint()) {

      if (defIter_->isInstruction() &&

          defIter_->toInstruction()->resumePoint()) {

        // In theory, we could but in practice this does not happen.

        MOZ_ASSERT(*defIter_ != block()->lastIns());

        last_ = defIter_->toInstruction();

      if (defIter_->isInstruction()) {

        resumePoint_ = defIter_->toInstruction()->resumePoint();

        lastInstruction_ = defIter_->toInstruction();

      }



      defIter_++;

    } else {

      last_ = nullptr;

      resumePoint_ = nullptr;

      lastInstruction_ = nullptr;

    }

  }
@@ -852,14 +847,8 @@ class MNodeIterator { 


 public:

  explicit MNodeIterator(MBasicBlock* block)

      : last_(block->entryResumePoint() ? block->lastIns() : nullptr),

        defIter_(block) {

      : resumePoint_(block->entryResumePoint()), defIter_(block) {

    MOZ_ASSERT(bool(block->entryResumePoint()) == atResumePoint());



    // We use the last instruction to check for the entry resume point,

    // assert that no control instruction has any resume point.  If so, then

    // we need to handle this case in this iterator.

    MOZ_ASSERT(!block->lastIns()->resumePoint());

  }



  MNodeIterator operator++(int) {