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 374331 06b3c4468a687e83a8425cd702297b23b3c37f18
parent 374330 c7ab28e722427c3bb74a16e33e4ebc3c26770cfe
child 374332 109aaf987508596bea32fce8dabf671b2075b7b9
push id10863
push userjlorenzo@mozilla.com
push dateMon, 06 Mar 2017 23:02:23 +0000
treeherdermozilla-aurora@0931190cd725 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersWaldo
bugs1341951
milestone54.0a1
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_;
     }