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 344758 7e72aa400561c7767ca4734f23ff3903e55d4e0a
parent 344678 6ef5dd99b45cf4b556e7204ba29c6446f550702a
child 344759 039475e399397ce1a79a0b31f2379a43eb52b894
push id31417
push userkwierso@gmail.com
push dateSat, 25 Feb 2017 00:46:36 +0000
treeherdermozilla-central@f36062d04d16 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersnbp
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 - 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;