Bug 1341951 - Replace RInstructionStorage copy by a cloneInto function on every RInstruction. r=Waldo
authorNicolas B. Pierron <nicolas.b.pierron@mozilla.com>
Wed, 01 Mar 2017 13:36:50 +0000
changeset 345304 06b3c4468a687e83a8425cd702297b23b3c37f18
parent 345303 c7ab28e722427c3bb74a16e33e4ebc3c26770cfe
child 345305 109aaf987508596bea32fce8dabf671b2075b7b9
push id31436
push userkwierso@gmail.com
push dateThu, 02 Mar 2017 01:18:52 +0000
treeherdermozilla-central@e91de6fb2b3d [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersWaldo
bugs1341951
milestone54.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 1341951 - Replace RInstructionStorage copy by a cloneInto function on every RInstruction. r=Waldo
js/src/jit/Recover.h
js/src/jit/Snapshots.cpp
js/src/jit/Snapshots.h
--- a/js/src/jit/Recover.h
+++ b/js/src/jit/Recover.h
@@ -129,16 +129,20 @@ class MOZ_NON_PARAM RInstruction
 
     // As opposed to the MIR, there is no need to add more methods as every
     // other instruction is well abstracted under the "recover" method.
     bool isResumePoint() const {
         return opcode() == Recover_ResumePoint;
     }
     inline const RResumePoint* toResumePoint() const;
 
+    // Call the copy constructor of a specific RInstruction, to do a copy of the
+    // RInstruction content.
+    virtual void cloneInto(RInstructionStorage* raw) const = 0;
+
     // Number of allocations which are encoded in the Snapshot for recovering
     // the current instruction.
     virtual uint32_t numOperands() const = 0;
 
     // Function used to recover the value computed by this instruction. This
     // function reads its arguments from the allocations listed on the snapshot
     // iterator and stores its returned value on the snapshot iterator too.
     virtual MOZ_MUST_USE bool recover(JSContext* cx, SnapshotIterator& iter) const = 0;
@@ -148,20 +152,24 @@ class MOZ_NON_PARAM RInstruction
     // instruction.
     static void readRecoverData(CompactBufferReader& reader, RInstructionStorage* raw);
 };
 
 #define RINSTRUCTION_HEADER_(op)                                        \
   private:                                                              \
     friend class RInstruction;                                          \
     explicit R##op(CompactBufferReader& reader);                        \
+    explicit R##op(const R##op& src) = default;                         \
                                                                         \
   public:                                                               \
     Opcode opcode() const {                                             \
         return RInstruction::Recover_##op;                              \
+    }                                                                   \
+    virtual void cloneInto(RInstructionStorage* raw) const {            \
+        new (raw->addr()) R##op(*this);                                 \
     }
 
 #define RINSTRUCTION_HEADER_NUM_OP_MAIN(op, numOp)                      \
     RINSTRUCTION_HEADER_(op)                                            \
     virtual uint32_t numOperands() const {                              \
         return numOp;                                                   \
     }
 
--- a/js/src/jit/Snapshots.cpp
+++ b/js/src/jit/Snapshots.cpp
@@ -585,25 +585,48 @@ SnapshotWriter::init()
     // enough to prevent the reallocation of the hash table for at least half of
     // the compilations.
     return allocMap_.init(32);
 }
 
 RecoverReader::RecoverReader(SnapshotReader& snapshot, const uint8_t* recovers, uint32_t size)
   : reader_(nullptr, nullptr),
     numInstructions_(0),
-    numInstructionsRead_(0)
+    numInstructionsRead_(0),
+    resumeAfter_(false)
 {
     if (!recovers)
         return;
     reader_ = CompactBufferReader(recovers + snapshot.recoverOffset(), recovers + size);
     readRecoverHeader();
     readInstruction();
 }
 
+RecoverReader::RecoverReader(const RecoverReader& rr)
+  : reader_(rr.reader_),
+    numInstructions_(rr.numInstructions_),
+    numInstructionsRead_(rr.numInstructionsRead_),
+    resumeAfter_(rr.resumeAfter_)
+{
+    if (reader_.currentPosition())
+        rr.instruction()->cloneInto(&rawData_);
+}
+
+RecoverReader&
+RecoverReader::operator=(const RecoverReader& rr)
+{
+    reader_ = rr.reader_;
+    numInstructions_ = rr.numInstructions_;
+    numInstructionsRead_ = rr.numInstructionsRead_;
+    resumeAfter_ = rr.resumeAfter_;
+    if (reader_.currentPosition())
+        rr.instruction()->cloneInto(&rawData_);
+    return *this;
+}
+
 void
 RecoverReader::readRecoverHeader()
 {
     uint32_t bits = reader_.readUnsigned();
 
     numInstructions_ = (bits & RECOVER_RINSCOUNT_MASK) >> RECOVER_RINSCOUNT_SHIFT;
     resumeAfter_ = (bits & RECOVER_RESUMEAFTER_MASK) >> RECOVER_RESUMEAFTER_SHIFT;
     MOZ_ASSERT(numInstructions_);
--- a/js/src/jit/Snapshots.h
+++ b/js/src/jit/Snapshots.h
@@ -509,44 +509,29 @@ class MOZ_NON_PARAM RInstructionStorage
 {
     static constexpr size_t Size = 4 * sizeof(uint32_t);
 
     // This presumes all RInstructionStorage are safely void*-alignable.
     // RInstruction::readRecoverData asserts that no RInstruction subclass
     // has stricter alignment requirements than RInstructionStorage.
     static constexpr size_t Alignment = alignof(void*);
 
-    /*
-     * DO NOT REUSE THIS IMPLEMENTATION TACTIC.
-     *
-     * It is undefined behavior to bytewise-copy the bytes of one T, into a
-     * location that isn't a T -- and because the target |mem| often wasn't
-     * already constructed as a T, it doesn't always count as such -- and then
-     * interpret that second location as T.  (This is *possibly* okay if T is
-     * POD or standard layout or is trivial.  None of those are true here.)
-     * The C++ spec considers this a strict aliasing violation.  We've hit
-     * crashes before when we've done this: for example, bug 1269319.
-     *
-     * This tactic is used here *only* because it was mistakenly added long
-     * ago, before it was recognized to be buggy, and we haven't yet struggled
-     * through the effort to remove it.
-     */
     alignas(Alignment) unsigned char mem[Size];
 
   public:
     const void* addr() const { return mem; }
     void* addr() { return mem; }
 
     RInstructionStorage() = default;
 
-    // FIXME (bug 1341951): These are strict aliasing violations: see the
-    // comment above, and see bug 1269319 for an instance of bytewise copying
-    // having caused crashes.
-    RInstructionStorage(const RInstructionStorage& other) = default;
-    RInstructionStorage& operator=(const RInstructionStorage& other) = default;
+    // Making a copy of raw bytes holding a RInstruction instance would be a
+    // strict aliasing violation: see bug 1269319 for an instance of bytewise
+    // copying having caused crashes.
+    RInstructionStorage(const RInstructionStorage&) = delete;
+    RInstructionStorage& operator=(const RInstructionStorage& other) = delete;
 };
 
 class RInstruction;
 
 class RecoverReader
 {
     CompactBufferReader reader_;
 
@@ -565,16 +550,18 @@ class RecoverReader
     RInstructionStorage rawData_;
 
   private:
     void readRecoverHeader();
     void readInstruction();
 
   public:
     RecoverReader(SnapshotReader& snapshot, const uint8_t* recovers, uint32_t size);
+    explicit RecoverReader(const RecoverReader& rr);
+    RecoverReader& operator=(const RecoverReader& rr);
 
     uint32_t numInstructions() const {
         return numInstructions_;
     }
     uint32_t numInstructionsRead() const {
         return numInstructionsRead_;
     }