Bug 1341951 - Remove mozilla::AlignedStorage, and inline its sole use into js::jit::RInstructionStorage. r=nbp
authorJeff Walden <jwalden@mit.edu>
Wed, 22 Feb 2017 18:27:51 -0800
changeset 373785 7e72aa400561c7767ca4734f23ff3903e55d4e0a
parent 373705 6ef5dd99b45cf4b556e7204ba29c6446f550702a
child 373786 039475e399397ce1a79a0b31f2379a43eb52b894
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)
reviewersnbp
bugs1341951
milestone54.0a1
Bug 1341951 - Remove mozilla::AlignedStorage, and inline its sole use into js::jit::RInstructionStorage. r=nbp
js/src/jit/Recover.cpp
js/src/jit/Snapshots.h
mfbt/Alignment.h
--- a/js/src/jit/Recover.cpp
+++ b/js/src/jit/Recover.cpp
@@ -43,17 +43,19 @@ MNode::writeRecoverData(CompactBufferWri
 void
 RInstruction::readRecoverData(CompactBufferReader& reader, RInstructionStorage* raw)
 {
     uint32_t op = reader.readUnsigned();
     switch (Opcode(op)) {
 #   define MATCH_OPCODES_(op)                                           \
       case Recover_##op:                                                \
         static_assert(sizeof(R##op) <= sizeof(RInstructionStorage),     \
-                      "Storage space is too small to decode R" #op " instructions."); \
+                      "storage space must be big enough to store R" #op); \
+        static_assert(alignof(R##op) <= alignof(RInstructionStorage),   \
+                      "storage space must be aligned adequate to store R" #op); \
         new (raw->addr()) R##op(reader);                                \
         break;
 
         RECOVER_OPCODE_LIST(MATCH_OPCODES_)
 #   undef MATCH_OPCODES_
 
       case Recover_Invalid:
       default:
--- a/js/src/jit/Snapshots.h
+++ b/js/src/jit/Snapshots.h
@@ -501,29 +501,58 @@ class SnapshotReader
     }
     void resetNumAllocationsRead() {
         allocRead_ = 0;
     }
 };
 
 class RInstructionStorage
 {
-    static const size_t Size = 4 * sizeof(uint32_t);
-    mozilla::AlignedStorage<Size> mem;
+    /*
+     * 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 |mBytes| often wasn't
+     * previously 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.
+     */
+    static constexpr size_t Size = 4 * sizeof(uint32_t);
+    union U
+    {
+        char mBytes[Size];
+
+        // This presumes all RInstructionStorage are safely uint64_t-alignable.
+        // RInstruction::readRecoverData asserts that no RInstruction subclass
+        // has stricter alignment requirements than RInstructionStorage.
+        uint64_t mDummy;
+    } u;
 
   public:
-    const void* addr() const { return mem.addr(); }
-    void* addr() { return mem.addr(); }
+    const void* addr() const { return u.mBytes; }
+    void* addr() { return u.mBytes; }
 
     RInstructionStorage() = default;
 
     RInstructionStorage(const RInstructionStorage& other) {
+        // FIXME: This is a strict aliasing violation: see the comment above,
+        //        and see bug 1269319 for an instance of this idea having
+        //        caused crashes.
         memcpy(addr(), other.addr(), Size);
     }
     void operator=(const RInstructionStorage& other) {
+        // FIXME: This is a strict aliasing violation: see the comment above,
+        //        and see bug 1269319 for an instance of this idea having
+        //        caused crashes.
         memcpy(addr(), other.addr(), Size);
     }
 };
 
 class RInstruction;
 
 class RecoverReader
 {
--- a/mfbt/Alignment.h
+++ b/mfbt/Alignment.h
@@ -94,46 +94,16 @@ struct AlignedElem<8>
 };
 
 template<>
 struct AlignedElem<16>
 {
   MOZ_ALIGNED_DECL(uint8_t elem, 16);
 };
 
-/*
- * This utility pales in comparison to Boost's aligned_storage. The utility
- * simply assumes that uint64_t is enough alignment for anyone. This may need
- * to be extended one day...
- *
- * As an important side effect, pulling the storage into this template is
- * enough obfuscation to confuse gcc's strict-aliasing analysis into not giving
- * false negatives when we cast from the char buffer to whatever type we've
- * constructed using the bytes.
- */
-template<size_t Nbytes>
-struct AlignedStorage
-{
-  union U
-  {
-    char mBytes[Nbytes];
-    uint64_t mDummy;
-  } u;
-
-  const void* addr() const { return u.mBytes; }
-  void* addr() { return u.mBytes; }
-
-  AlignedStorage() = default;
-
-  // AlignedStorage is non-copyable: the default copy constructor violates
-  // strict aliasing rules, per bug 1269319.
-  AlignedStorage(const AlignedStorage&) = delete;
-  void operator=(const AlignedStorage&) = delete;
-};
-
 template<typename T>
 struct MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS AlignedStorage2
 {
   union U
   {
     char mBytes[sizeof(T)];
     uint64_t mDummy;
   } u;