Bug 1338374 - Use alignas/alignof to define Variant's internal raw storage. r=froydnj
authorJeff Walden <jwalden@mit.edu>
Mon, 30 Jan 2017 15:56:05 -0800
changeset 342887 81b228f0a7708c889f2d90627ff2199bc4284565
parent 342886 eb11f699091d1fa8ce5dc0ef671868feeb179778
child 342888 29d6afb1f91681a3f27549f5167e6dfc544546a0
push id31366
push usercbook@mozilla.com
push dateWed, 15 Feb 2017 11:25:19 +0000
treeherdermozilla-central@c0807d6938c1 [default view] [failures only]
perfherder[talos] [build metrics] [platform microbench] (compared to previous push)
reviewersfroydnj
bugs1338374
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 1338374 - Use alignas/alignof to define Variant's internal raw storage. r=froydnj
image/StreamingLexer.h
js/src/jscntxt.h
js/src/vm/HelperThreads.cpp
js/src/vm/HelperThreads.h
js/src/vm/SavedStacks.cpp
mfbt/Variant.h
xpcom/threads/nsTimerImpl.cpp
xpcom/threads/nsTimerImpl.h
--- a/image/StreamingLexer.h
+++ b/image/StreamingLexer.h
@@ -355,18 +355,18 @@ private:
  * XXX(seth): We should be able to get of the |State| stuff totally once bug
  * 1198451 lands, since we can then just return a function representing the next
  * state directly.
  */
 template <typename State, size_t InlineBufferSize = 16>
 class StreamingLexer
 {
 public:
-  StreamingLexer(LexerTransition<State> aStartState,
-                 LexerTransition<State> aTruncatedState)
+  StreamingLexer(const LexerTransition<State>& aStartState,
+                 const LexerTransition<State>& aTruncatedState)
     : mTransition(TerminalState::FAILURE)
     , mTruncatedTransition(aTruncatedState)
   {
     if (!aStartState.NextStateIsTerminal() &&
         aStartState.ControlFlow() == ControlFlowStrategy::YIELD) {
       // Allowing a StreamingLexer to start in a yield state doesn't make sense
       // semantically (since yield states are supposed to deliver the same data
       // as previous states, and there's no previous state here), but more
--- a/js/src/jscntxt.h
+++ b/js/src/jscntxt.h
@@ -306,22 +306,22 @@ struct JSContext : public JS::RootingCon
   public:
     inline JS::Result<> boolToResult(bool ok);
 
     /**
      * Intentionally awkward signpost method that is stationed on the
      * boundary between Result-using and non-Result-using code.
      */
     template <typename V, typename E>
-    bool resultToBool(JS::Result<V, E> result) {
+    bool resultToBool(const JS::Result<V, E>& result) {
         return result.isOk();
     }
 
     template <typename V, typename E>
-    V* resultToPtr(JS::Result<V*, E> result) {
+    V* resultToPtr(const JS::Result<V*, E>& result) {
         return result.isOk() ? result.unwrap() : nullptr;
     }
 
     mozilla::GenericErrorResult<JS::OOM&> alreadyReportedOOM();
     mozilla::GenericErrorResult<JS::Error&> alreadyReportedError();
 
     /*
      * Points to the most recent JitActivation pushed on the thread.
--- a/js/src/vm/HelperThreads.cpp
+++ b/js/src/vm/HelperThreads.cpp
@@ -117,47 +117,47 @@ static void
 FinishOffThreadIonCompile(jit::IonBuilder* builder, const AutoLockHelperThreadState& lock)
 {
     AutoEnterOOMUnsafeRegion oomUnsafe;
     if (!HelperThreadState().ionFinishedList(lock).append(builder))
         oomUnsafe.crash("FinishOffThreadIonCompile");
 }
 
 static JSRuntime*
-GetSelectorRuntime(CompilationSelector selector)
+GetSelectorRuntime(const CompilationSelector& selector)
 {
     struct Matcher
     {
         JSRuntime* match(JSScript* script)    { return script->runtimeFromActiveCooperatingThread(); }
         JSRuntime* match(JSCompartment* comp) { return comp->runtimeFromActiveCooperatingThread(); }
         JSRuntime* match(ZonesInState zbs)    { return zbs.runtime; }
         JSRuntime* match(JSRuntime* runtime)  { return runtime; }
         JSRuntime* match(AllCompilations all) { return nullptr; }
     };
 
     return selector.match(Matcher());
 }
 
 static bool
-JitDataStructuresExist(CompilationSelector selector)
+JitDataStructuresExist(const CompilationSelector& selector)
 {
     struct Matcher
     {
         bool match(JSScript* script)    { return !!script->compartment()->jitCompartment(); }
         bool match(JSCompartment* comp) { return !!comp->jitCompartment(); }
         bool match(ZonesInState zbs)    { return zbs.runtime->hasJitRuntime(); }
         bool match(JSRuntime* runtime)  { return runtime->hasJitRuntime(); }
         bool match(AllCompilations all) { return true; }
     };
 
     return selector.match(Matcher());
 }
 
 static bool
-CompiledScriptMatches(CompilationSelector selector, JSScript* target)
+CompiledScriptMatches(const CompilationSelector& selector, JSScript* target)
 {
     struct ScriptMatches
     {
         JSScript* target_;
 
         bool match(JSScript* script)    { return script == target_; }
         bool match(JSCompartment* comp) { return comp == target_->compartment(); }
         bool match(JSRuntime* runtime)  { return runtime == target_->runtimeFromAnyThread(); }
@@ -167,17 +167,17 @@ CompiledScriptMatches(CompilationSelecto
                    zbs.state == target_->zoneFromAnyThread()->gcState();
         }
     };
 
     return selector.match(ScriptMatches{target});
 }
 
 void
-js::CancelOffThreadIonCompile(CompilationSelector selector, bool discardLazyLinkList)
+js::CancelOffThreadIonCompile(const CompilationSelector& selector, bool discardLazyLinkList)
 {
     if (!JitDataStructuresExist(selector))
         return;
 
     AutoLockHelperThreadState lock;
 
     if (!HelperThreadState().threads)
         return;
--- a/js/src/vm/HelperThreads.h
+++ b/js/src/vm/HelperThreads.h
@@ -453,17 +453,17 @@ using CompilationSelector = mozilla::Var
                                              ZonesInState,
                                              JSRuntime*,
                                              AllCompilations>;
 
 /*
  * Cancel scheduled or in progress Ion compilations.
  */
 void
-CancelOffThreadIonCompile(CompilationSelector selector, bool discardLazyLinkList);
+CancelOffThreadIonCompile(const CompilationSelector& selector, bool discardLazyLinkList);
 
 inline void
 CancelOffThreadIonCompile(JSScript* script)
 {
     CancelOffThreadIonCompile(CompilationSelector(script), true);
 }
 
 inline void
--- a/js/src/vm/SavedStacks.cpp
+++ b/js/src/vm/SavedStacks.cpp
@@ -138,17 +138,18 @@ LiveSavedFrameCache::find(JSContext* cx,
     // Everything after the cached SavedFrame are stale younger frames we have
     // since popped.
     frames->shrinkBy(frames->length() - numberStillValid);
 }
 
 struct SavedFrame::Lookup {
     Lookup(JSAtom* source, uint32_t line, uint32_t column,
            JSAtom* functionDisplayName, JSAtom* asyncCause, SavedFrame* parent,
-           JSPrincipals* principals, Maybe<LiveSavedFrameCache::FramePtr> framePtr = Nothing(),
+           JSPrincipals* principals,
+           const Maybe<LiveSavedFrameCache::FramePtr>& framePtr = Nothing(),
            jsbytecode* pc = nullptr, Activation* activation = nullptr)
       : source(source),
         line(line),
         column(column),
         functionDisplayName(functionDisplayName),
         asyncCause(asyncCause),
         parent(parent),
         principals(principals),
--- a/mfbt/Variant.h
+++ b/mfbt/Variant.h
@@ -4,19 +4,19 @@
  * License, v. 2.0. If a copy of the MPL was not distributed with this
  * file, You can obtain one at http://mozilla.org/MPL/2.0/. */
 
 /* A template class for tagged unions. */
 
 #include <new>
 #include <stdint.h>
 
-#include "mozilla/Alignment.h"
 #include "mozilla/Assertions.h"
 #include "mozilla/Move.h"
+#include "mozilla/TemplateLib.h"
 #include "mozilla/TypeTraits.h"
 
 #ifndef mozilla_Variant_h
 #define mozilla_Variant_h
 
 namespace mozilla {
 
 template<typename... Ts>
@@ -47,32 +47,16 @@ struct TypesAreDistinct<> : TrueType { }
 template<typename First, typename... Rest>
 struct TypesAreDistinct<First, Rest...>
 {
   static constexpr bool value =
     !FirstTypeIsInRest<First, Rest...>::value &&
     TypesAreDistinct<Rest...>::value;
 };
 
-// MaxSizeOf computes the maximum sizeof(T) for each T in Ts.
-
-template<typename T, typename... Ts>
-struct MaxSizeOf
-{
-  static const size_t size = sizeof(T) > MaxSizeOf<Ts...>::size
-    ? sizeof(T)
-    : MaxSizeOf<Ts...>::size;
-};
-
-template<typename T>
-struct MaxSizeOf<T>
-{
-  static const size_t size = sizeof(T);
-};
-
 // The `IsVariant` helper is used in conjunction with static_assert and
 // `mozilla::EnableIf` to catch passing non-variant types to `Variant::is<T>()`
 // and friends at compile time, rather than at runtime. It ensures that the
 // given type `Needle` is one of the types in the set of types `Haystack`.
 
 template<typename Needle, typename... Haystack>
 struct IsVariant;
 
@@ -447,34 +431,52 @@ struct AsVariantTemporary
  * string, or an owning reference to our copy:
  *
  *     class CopyOnWriteString
  *     {
  *       Variant<const char*, UniquePtr<char[]>> string;
  *
  *       ...
  *     };
+ *
+ * Because Variant must be aligned suitable to hold any value stored within it,
+ * and because |alignas| requirements don't affect platform ABI with respect to
+ * how parameters are laid out in memory, Variant can't be used as the type of a
+ * function parameter.  Pass Variant to functions by pointer or reference
+ * instead.
  */
 template<typename... Ts>
-class MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS Variant
+class MOZ_INHERIT_TYPE_ANNOTATIONS_FROM_TEMPLATE_ARGS MOZ_NON_PARAM Variant
 {
-  static_assert(detail::TypesAreDistinct<Ts...>::value, "Variant with duplicate types is not supported");
+  static_assert(detail::TypesAreDistinct<Ts...>::value,
+                "Variant with duplicate types is not supported");
+
   using Tag = typename detail::VariantTag<Ts...>::Type;
   using Impl = detail::VariantImplementation<Tag, 0, Ts...>;
-  using RawData = AlignedStorage<detail::MaxSizeOf<Ts...>::size>;
+
+  static constexpr size_t RawDataAlignment = tl::Max<alignof(Ts)...>::value;
+  static constexpr size_t RawDataSize = tl::Max<sizeof(Ts)...>::value;
 
   // Raw storage for the contained variant value.
-  RawData raw;
+  alignas(RawDataAlignment) unsigned char rawData[RawDataSize];
 
   // Each type is given a unique tag value that lets us keep track of the
   // contained variant value's type.
   Tag tag;
 
+  // Some versions of GCC treat it as a -Wstrict-aliasing violation (ergo a
+  // -Werror compile error) to reinterpret_cast<> |rawData| to |T*|, even
+  // through |void*|.  Placing the latter cast in these separate functions
+  // breaks the chain such that affected GCC versions no longer warn/error.
   void* ptr() {
-    return reinterpret_cast<void*>(&raw);
+    return rawData;
+  }
+
+  const void* ptr() const {
+    return rawData;
   }
 
 public:
   /** Perfect forwarding construction for some variant type T. */
   template<typename RefT,
            // RefT captures both const& as well as && (as intended, to support
            // perfect forwarding), so we have to remove those qualifiers here
            // when ensuring that T is a variant of this type, and getting T's
@@ -571,26 +573,26 @@ public:
   // Accessors for working with the contained variant value.
 
   /** Mutable reference. */
   template<typename T>
   T& as() {
     static_assert(detail::IsVariant<T, Ts...>::value,
                   "provided a type not found in this Variant's type list");
     MOZ_RELEASE_ASSERT(is<T>());
-    return *reinterpret_cast<T*>(&raw);
+    return *static_cast<T*>(ptr());
   }
 
   /** Immutable const reference. */
   template<typename T>
   const T& as() const {
     static_assert(detail::IsVariant<T, Ts...>::value,
                   "provided a type not found in this Variant's type list");
     MOZ_RELEASE_ASSERT(is<T>());
-    return *reinterpret_cast<const T*>(&raw);
+    return *static_cast<const T*>(ptr());
   }
 
   /**
    * Extract the contained variant value from this container into a temporary
    * value.  On completion, the value in the variant will be in a
    * safely-destructible state, as determined by the behavior of T's move
    * constructor when provided the variant's internal value.
    */
--- a/xpcom/threads/nsTimerImpl.cpp
+++ b/xpcom/threads/nsTimerImpl.cpp
@@ -214,17 +214,17 @@ nsTimerImpl::InitCommon(uint32_t aDelay,
   return gThread->AddTimer(this);
 }
 
 nsresult
 nsTimerImpl::InitWithFuncCallbackCommon(nsTimerCallbackFunc aFunc,
                                         void* aClosure,
                                         uint32_t aDelay,
                                         uint32_t aType,
-                                        Callback::Name aName)
+                                        const Callback::Name& aName)
 {
   if (NS_WARN_IF(!aFunc)) {
     return NS_ERROR_INVALID_ARG;
   }
 
   Callback cb; // Goes out of scope after the unlock, prevents deadlock
   cb.mType = Callback::Type::Function;
   cb.mCallback.c = aFunc;
--- a/xpcom/threads/nsTimerImpl.h
+++ b/xpcom/threads/nsTimerImpl.h
@@ -153,17 +153,17 @@ public:
   nsCOMPtr<nsIEventTarget> mEventTarget;
 
   void LogFiring(const Callback& aCallback, uint8_t aType, uint32_t aDelay);
 
   nsresult InitWithFuncCallbackCommon(nsTimerCallbackFunc aFunc,
                                       void* aClosure,
                                       uint32_t aDelay,
                                       uint32_t aType,
-                                      Callback::Name aName);
+                                      const Callback::Name& aName);
 
   // These members are set by the initiating thread, when the timer's type is
   // changed and during the period where it fires on that thread.
   uint8_t               mType;
 
   // The generation number of this timer, re-generated each time the timer is
   // initialized so one-shot timers can be canceled and re-initialized by the
   // arming thread without any bad race conditions.