Changeset 128203 in webkit

Timestamp:

Sep 11, 2012 10:39:08 AM (12 years ago)

Author:

enne@google.com

Message:

Clang doesn't optimize away undefined OwnPtr copy constructor
​https://bugs.webkit.org/show_bug.cgi?id=74625

Reviewed by Anders Carlsson.

Original patch by Anders Carlsson, with a minor edit.

The publicly declared-but-not-defined copy constructor is a compiler
optimization-dependent landmine. Clang often fails to optimize the use
of this function out, leading to internal linkage errors for the missing
definition. gcc doesn't have this problem and optimizes that function
out, leading to code that inconsistently fails to link across platforms.

As a partial fix for this problem, on any compiler that supports C++11
move semantics, replace the bogus copy constructor with the move
constructors. In the future, if all compilers support this, then the
copy constructor can be removed.

This still leaves other compilers that don't support move semantics
like Visual Studio vulnerable to linking inconsistencies.

• wtf/OwnPtr.h:

(OwnPtr):
(WTF):
(WTF::::OwnPtr):
(WTF::=):

Location:

trunk/Source/WTF

Files:

• ChangeLog (modified) (1 diff)
• wtf/OwnPtr.h (modified) (6 diffs)

trunk/Source/WTF/ChangeLog

@@ +1 @@
+2012-09-11  Adrienne Walker  <enne@google.com>
+
+        Clang doesn't optimize away undefined OwnPtr copy constructor
+        https://bugs.webkit.org/show_bug.cgi?id=74625
+
+        Reviewed by Anders Carlsson.
+
+        Original patch by Anders Carlsson, with a minor edit.
+
+        The publicly declared-but-not-defined copy constructor is a compiler
+        optimization-dependent landmine. Clang often fails to optimize the use
+        of this function out, leading to internal linkage errors for the missing
+        definition. gcc doesn't have this problem and optimizes that function
+        out, leading to code that inconsistently fails to link across platforms.
+
+        As a partial fix for this problem, on any compiler that supports C++11
+        move semantics, replace the bogus copy constructor with the move
+        constructors.  In the future, if all compilers support this, then the
+        copy constructor can be removed.
+
+        This still leaves other compilers that don't support move semantics
+        like Visual Studio vulnerable to linking inconsistencies.
+
+        * wtf/OwnPtr.h:
+        (OwnPtr):
+        (WTF):
+        (WTF::::OwnPtr):
+        (WTF::=):
+
 2012-09-11  Raphael Kubo da Costa  <rakuco@webkit.org>
 

trunk/Source/WTF/wtf/OwnPtr.h

@@ -23 +23 @@
 
 #include <wtf/Assertions.h>
+#include <wtf/Noncopyable.h>
 #include <wtf/NullPtr.h>
 #include <wtf/OwnPtrCommon.h>
@@ -37 +38 @@
 
     template<typename T> class OwnPtr {
+#if COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
+        // If rvalue references are not supported, the copy constructor is
+        // public so OwnPtr cannot be marked noncopyable. See note below.
+        WTF_MAKE_NONCOPYABLE(OwnPtr);
+#endif
     public:
         typedef typename RemovePointer<T>::Type ValueType;
@@ -47 +53 @@
         template<typename U> OwnPtr(const PassOwnPtr<U>& o);
 
+#if !COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
         // This copy constructor is used implicitly by gcc when it generates
         // transients for assigning a PassOwnPtr<T> object to a stack-allocated
@@ -52 +59 @@
         // should optimize away the constructor when generating code.
         OwnPtr(const OwnPtr<ValueType>&);
+#endif
 
         ~OwnPtr() { deleteOwnedPtr(m_ptr); }
@@ -74 +82 @@
         template<typename U> OwnPtr& operator=(const PassOwnPtr<U>&);
 
+#if COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
+        OwnPtr(OwnPtr&&);
+        template<typename U> OwnPtr(OwnPtr<U>&&);
+
+        OwnPtr& operator=(OwnPtr&&);
+        template<typename U> OwnPtr& operator=(OwnPtr<U>&&);
+#endif
+
         void swap(OwnPtr& o) { std::swap(m_ptr, o.m_ptr); }
 
     private:
-        OwnPtr& operator=(const OwnPtr<T>&);
+#if !COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
+        // If rvalue references are supported, noncopyable takes care of this.
+        OwnPtr& operator=(const OwnPtr&);
+#endif
 
         // We should never have two OwnPtrs for the same underlying object (otherwise we'll get
@@ -133 +152 @@
     }
 
+#if COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
+    template<typename T> inline OwnPtr<T>::OwnPtr(OwnPtr<T>&& o)
+        : m_ptr(o.leakPtr())
+    {
+    }
+
+    template<typename T> template<typename U> inline OwnPtr<T>::OwnPtr(OwnPtr<U>&& o)
+        : m_ptr(o.leakPtr())
+    {
+    }
+
+    template<typename T> inline OwnPtr<T>& OwnPtr<T>::operator=(OwnPtr<T>&& o)
+    {
+        PtrType ptr = m_ptr;
+        m_ptr = o.leakPtr();
+        ASSERT(!ptr || m_ptr != ptr);
+        deleteOwnedPtr(ptr);
+
+        return *this;
+    }
+
+    template<typename T> template<typename U> inline OwnPtr<T>& OwnPtr<T>::operator=(OwnPtr<U>&& o)
+    {
+        PtrType ptr = m_ptr;
+        m_ptr = o.leakPtr();
+        ASSERT(!ptr || m_ptr != ptr);
+        deleteOwnedPtr(ptr);
+
+        return *this;
+    }
+#endif
+
     template<typename T> inline void swap(OwnPtr<T>& a, OwnPtr<T>& b)
     {