Issue #2148 - Make Vector not use AlignedStorage for its inline element storage

See Bug 1338374 1/2

js/src/jit/RangeAnalysis.h

@@ -46,7 +46,8 @@ struct LoopIterationBound : public TempObject
     // of the loop header. This will use loop invariant terms and header phis.
     LinearSum currentSum;
 
-    LoopIterationBound(MBasicBlock* header, MTest* test, LinearSum boundSum, LinearSum currentSum)
+    LoopIterationBound(MBasicBlock* header, MTest* test,
+                       const LinearSum& boundSum, const LinearSum& currentSum)
       : header(header), test(test),
         boundSum(boundSum), currentSum(currentSum)
     {
@@ -59,7 +60,7 @@ typedef Vector<LoopIterationBound*, 0, SystemAllocPolicy> LoopIterationBoundVect
 struct SymbolicBound : public TempObject
 {
   private:
-    SymbolicBound(LoopIterationBound* loop, LinearSum sum)
+    SymbolicBound(LoopIterationBound* loop, const LinearSum& sum)
       : loop(loop), sum(sum)
     {
     }
@@ -73,7 +74,8 @@ struct SymbolicBound : public TempObject
     // If nullptr, then 'sum' is always valid.
     LoopIterationBound* loop;
 
-    static SymbolicBound* New(TempAllocator& alloc, LoopIterationBound* loop, LinearSum sum) {
+    static SymbolicBound*
+    New(TempAllocator& alloc, LoopIterationBound* loop, const LinearSum& sum) {
         return new(alloc) SymbolicBound(loop, sum);
     }
 

js/src/jsapi-tests/tests.h

@@ -23,6 +23,7 @@
 /* Note: Aborts on OOM. */
 class JSAPITestString {
     js::Vector<char, 0, js::SystemAllocPolicy> chars;
+
   public:
     JSAPITestString() {}
     explicit JSAPITestString(const char* s) { *this += s; }
@@ -32,21 +33,34 @@ class JSAPITestString {
     const char* end() const { return chars.end(); }
     size_t length() const { return chars.length(); }
 
-    JSAPITestString & operator +=(const char* s) {
+    JSAPITestString& operator +=(const char* s) {
         if (!chars.append(s, strlen(s)))
             abort();
         return *this;
     }
 
-    JSAPITestString & operator +=(const JSAPITestString& s) {
+    JSAPITestString& operator +=(const JSAPITestString& s) {
         if (!chars.append(s.begin(), s.length()))
             abort();
         return *this;
     }
 };
 
-inline JSAPITestString operator+(JSAPITestString a, const char* b) { return a += b; }
-inline JSAPITestString operator+(JSAPITestString a, const JSAPITestString& b) { return a += b; }
+inline JSAPITestString
+operator+(const JSAPITestString& a, const char* b)
+{
+    JSAPITestString result = a;
+    result += b;
+    return result;
+}
+
+inline JSAPITestString
+operator+(const JSAPITestString& a, const JSAPITestString& b)
+{
+    JSAPITestString result = a;
+    result += b;
+    return result;
+}
 
 class JSAPITest
 {
@@ -205,7 +219,11 @@ class JSAPITest
             return fail(JSAPITestString("CHECK failed: " #expr), __FILE__, __LINE__); \
     } while (false)
 
-    bool fail(JSAPITestString msg = JSAPITestString(), const char* filename = "-", int lineno = 0) {
+    bool fail(const JSAPITestString& msg = JSAPITestString(),
+              const char* filename = "-",
+              int lineno = 0)
+    {
+        JSAPITestString message = msg;
         if (JS_IsExceptionPending(cx)) {
             js::gc::AutoSuppressGC gcoff(cx);
             JS::RootedValue v(cx);
@@ -215,11 +233,12 @@ class JSAPITest
             if (s) {
                 JSAutoByteString bytes(cx, s);
                 if (!!bytes)
-                    msg += bytes.ptr();
+                    message += bytes.ptr();
             }
         }
-        fprintf(stderr, "%s:%d:%.*s\n", filename, lineno, (int) msg.length(), msg.begin());
-        msgs += msg;
+        fprintf(stderr, "%s:%d:%.*s\n",
+                filename, lineno, int(message.length()), message.begin());
+        msgs += message;
         return false;
     }
 

mfbt/Vector.h

@@ -277,7 +277,7 @@ struct VectorTesting;
 template<typename T,
          size_t MinInlineCapacity = 0,
          class AllocPolicy = MallocAllocPolicy>
-class Vector final : private AllocPolicy
+class MOZ_NON_PARAM Vector final : private AllocPolicy
 {
   /* utilities */
 
@@ -293,36 +293,39 @@ class Vector final : private AllocPolicy
 
   /* magic constants */
 
-  static const int kMaxInlineBytes = 1024;
-
-  /* compute constants */
-
-  /*
-   * Consider element size to be 1 for buffer sizing if there are 0 inline
-   * elements.  This allows us to compile when the definition of the element
-   * type is not visible here.
+/**
+   * The maximum space allocated for inline element storage.
    *
-   * Explicit specialization is only allowed at namespace scope, so in order
-   * to keep everything here, we use a dummy template parameter with partial
-   * specialization.
+   * We reduce space by what the AllocPolicy base class and prior Vector member
+   * fields likely consume to attempt to play well with binary size classes.
    */
-  template<int M, int Dummy>
-  struct ElemSize
-  {
-    static const size_t value = sizeof(T);
-  };
-  template<int Dummy>
-  struct ElemSize<0, Dummy>
-  {
-    static const size_t value = 1;
+  static constexpr size_t kMaxInlineBytes =
+      1024 -
+      (sizeof(AllocPolicy) + sizeof(T*) + sizeof(size_t) + sizeof(size_t));
+
+  /**
+   * The number of T elements of inline capacity built into this Vector.  This
+   * is usually |MinInlineCapacity|, but it may be less (or zero!) for large T.
+   *
+   * We use a partially-specialized template (not explicit specialization, which
+   * is only allowed at namespace scope) to compute this value.  The benefit is
+   * that |sizeof(T)| need not be computed, and |T| doesn't have to be fully
+   * defined at the time |Vector<T>| appears, if no inline storage is requested.
+   */
+  template <size_t MinimumInlineCapacity, size_t Dummy>
+  struct ComputeCapacity {
+    static constexpr size_t value =
+        tl::Min<MinimumInlineCapacity, kMaxInlineBytes / sizeof(T)>::value;
   };
 
-  static const size_t kInlineCapacity =
-    tl::Min<MinInlineCapacity, kMaxInlineBytes / ElemSize<MinInlineCapacity, 0>::value>::value;
+  template <size_t Dummy>
+  struct ComputeCapacity<0, Dummy> {
+    static constexpr size_t value = 0;
+  };
 
-  /* Calculate inline buffer size; avoid 0-sized array. */
-  static const size_t kInlineBytes =
-    tl::Max<1, kInlineCapacity * ElemSize<MinInlineCapacity, 0>::value>::value;
+  /** The actual inline capacity in number of elements T.  This may be zero! */
+  static constexpr size_t kInlineCapacity =
+      ComputeCapacity<MinInlineCapacity, 0>::value;
 
   /* member data */
 
@@ -346,8 +349,34 @@ class Vector final : private AllocPolicy
   size_t mReserved;
 #endif
 
-  /* Memory used for inline storage. */
-  AlignedStorage<kInlineBytes> mStorage;
+  /*
+   * Memory used for inline storage.  We want basically this:
+   *
+   *   alignas(T) unsigned char storage[kInlineCapacity * sizeof(T)];
+   *
+   * but C++ forbids zero-sized arrays that might result if we did this.  We fix
+   * this by (again) using partial specialization, defining an array only if
+   * contains at least one element.
+   */
+  template<size_t Capacity, size_t Dummy>
+  struct InlineStorage
+  {
+    alignas(T) unsigned char mBytes[Capacity * sizeof(T)];
+
+    // GCC fails due to -Werror=strict-aliasing if |mBytes| is directly cast to
+    // T*.  Indirecting through this function addresses the problem.
+    void* data() { return mBytes; }
+
+    T* addr() { return static_cast<T*>(data()); }
+  };
+
+  template<size_t Dummy>
+  struct InlineStorage<0, Dummy>
+  {
+    T* addr() { return nullptr; }
+  };
+
+  InlineStorage<kInlineCapacity, 0> mStorage;
 
 #ifdef DEBUG
   friend class ReentrancyGuard;
@@ -363,7 +392,7 @@ class Vector final : private AllocPolicy
 
   T* inlineStorage()
   {
-    return static_cast<T*>(mStorage.addr());
+    return mStorage.addr();
   }
 
   T* beginNoCheck() const
@@ -771,7 +800,7 @@ Vector<T, N, AP>::Vector(AP aAP)
   , mEntered(false)
 #endif
 {
-  mBegin = static_cast<T*>(mStorage.addr());
+  mBegin = inlineStorage();
 }
 
 /* Move constructor. */
@@ -791,7 +820,7 @@ Vector<T, N, AllocPolicy>::Vector(Vector&& aRhs)
 
   if (aRhs.usingInlineStorage()) {
     /* We can't move the buffer over in this case, so copy elements. */
-    mBegin = static_cast<T*>(mStorage.addr());
+    mBegin = inlineStorage();
     Impl::moveConstruct(mBegin, aRhs.beginNoCheck(), aRhs.endNoCheck());
     /*
      * Leave aRhs's mLength, mBegin, mCapacity, and mReserved as they are.
@@ -803,7 +832,7 @@ Vector<T, N, AllocPolicy>::Vector(Vector&& aRhs)
      * in-line storage.
      */
     mBegin = aRhs.mBegin;
-    aRhs.mBegin = static_cast<T*>(aRhs.mStorage.addr());
+    aRhs.mBegin = aRhs.inlineStorage();
     aRhs.mCapacity = kInlineCapacity;
     aRhs.mLength = 0;
 #ifdef DEBUG
@@ -1142,7 +1171,7 @@ Vector<T, N, AP>::clearAndFree()
     return;
   }
   this->free_(beginNoCheck());
-  mBegin = static_cast<T*>(mStorage.addr());
+  mBegin = inlineStorage();
   mCapacity = kInlineCapacity;
 #ifdef DEBUG
   mReserved = 0;
@@ -1370,7 +1399,7 @@ Vector<T, N, AP>::extractRawBuffer()
   }
 
   T* ret = mBegin;
-  mBegin = static_cast<T*>(mStorage.addr());
+  mBegin = inlineStorage();
   mLength = 0;
   mCapacity = kInlineCapacity;
 #ifdef DEBUG
@@ -1396,7 +1425,7 @@ Vector<T, N, AP>::extractOrCopyRawBuffer()
 
   Impl::moveConstruct(copy, beginNoCheck(), endNoCheck());
   Impl::destroy(beginNoCheck(), endNoCheck());
-  mBegin = static_cast<T*>(mStorage.addr());
+  mBegin = inlineStorage();
   mLength = 0;
   mCapacity = kInlineCapacity;
 #ifdef DEBUG
@@ -1424,7 +1453,7 @@ Vector<T, N, AP>::replaceRawBuffer(T* aP, size_t aLength)
      * otherwise be acceptable.  Maybe this behaviour should be
      * specifiable with an argument to this function.
      */
-    mBegin = static_cast<T*>(mStorage.addr());
+    mBegin = inlineStorage();
     mLength = aLength;
     mCapacity = kInlineCapacity;
     Impl::moveConstruct(mBegin, aP, aP + aLength);

security/certverifier/CTLogVerifier.cpp

@@ -172,10 +172,17 @@ CTLogVerifier::Verify(const LogEntry& entry,
   if (rv != Success) {
     return rv;
   }
+
+  // sct.extensions may be empty.  If it is, sctExtensionsInput will remain in
+  // its default state, which is valid but of length 0.
   Input sctExtensionsInput;
   rv = BufferToInput(sct.extensions, sctExtensionsInput);
-  if (rv != Success) {
-    return rv;
+  if (sct.extensions.length() > 0) {
+    rv = sctExtensionsInput.Init(sct.extensions.begin(),
+                                 sct.extensions.length());
+    if (rv != Success) {
+      return rv;
+    }
   }
 
   Buffer serializedData;

security/certverifier/SignedCertificateTimestamp.h

@@ -115,6 +115,9 @@ struct SignedCertificateTimestamp
 
 inline pkix::Result BufferToInput(const Buffer& buffer, pkix::Input& input)
 {
+  if (buffer.length() == 0) {
+    return pkix::Result::FATAL_ERROR_LIBRARY_FAILURE;
+  }
   return input.Init(buffer.begin(), buffer.length());
 }