Bug 74625 – Clang doesn't optimize away undefined OwnPtr copy constructor

• Component: Web Template Framework
• Status: RESOLVED FIXED

[Adrienne Walker]

Clang doesn't optimize away undefined OwnPtr copy constructor

[Adrienne Walker]

Created attachment 119460 [details]
Patch

[Nico Weber]

Comment on attachment 119460 [details]
Patch

This won't work, clang sets GCC.

Declaring but not defining this destructor is a bug, and from what I can tell not necessary for recent-ish gccs. Can you try just removing that whole block?

[Adrienne Walker]

(In reply to comment #2)
> (From update of attachment 119460 [details])
> This won't work, clang sets GCC.
> 
> Declaring but not defining this destructor is a bug, and from what I can tell not necessary for recent-ish gccs. Can you try just removing that whole block?

Removing the whole block works fine and fixes the Clang errors.

However, I think it's still useful to have this public declared-but-not-defined copy constructor for gcc, because it makes it impossible to call it explicitly without getting link errors.

Ideally, you'd make OwnPtr WTF_MAKE_NONCOPYABLE, but because the copy constructor gets called implicitly you can't do that.  I think having that function there is the next best thing.

[Adrienne Walker]

Created attachment 119465 [details]
Change #if to !Clang

[Anders Carlsson]

Adding an OwnPtr copy constructor is bad and will lead to memory corruption - do we know where clang is generating calls to the copy constructor?

[Anders Carlsson]

Comment on attachment 119465 [details]
Change #if to !Clang

I'll r- this so it won't be landed by accident and possibly break the world :)

[Nico Weber]

> However, I think it's still useful to have this public declared-but-not-defined copy constructor for gcc, because it makes it impossible to call it explicitly without getting link errors.

If you remove it, it's impossible to call it without getting compile errors. I don't understand why this can't be removed. It was added as a workaround to make this compile with some old gcc (I'd guess pre 4.0?)

[Adrienne Walker]

(In reply to comment #5)
> Adding an OwnPtr copy constructor is bad and will lead to memory corruption - do we know where clang is generating calls to the copy constructor?

I'm not adding--I'm compiling out its declaration on Clang.

Both gcc and Clang generate implicit calls to the OwnPtr copy constructor when you assign and initialize a stack-based OwnPtr, e.g.

    OwnPtr<T> local = adoptPtr(SomeClass::create());

You can see this locally if you move the existing copy constructor behind a private visibility modifier, build with gcc, and watch the compiler errors fly.    gcc optimizes these function calls out but Clang doesn't, so Clang's build fails with link errors because the copy constructor only has a declaration and not a definition.

I would love if somebody could make this function call not be transitively generated, but doing so myself is beyond my ken.

(In reply to comment #7)
> > However, I think it's still useful to have this public declared-but-not-defined copy constructor for gcc, because it makes it impossible to call it explicitly without getting link errors.
>
> If you remove it, it's impossible to call it without getting compile errors. I don't understand why this can't be removed. It was added as a workaround to make this compile with some old gcc (I'd guess pre 4.0?)

Not true.  Copy constructors are one of the special member functions that C++ will happily provide public versions of for you if you don't declare them in some other way.

Here's some example code:

    OwnPtr<T> ptr1;
    OwnPtr<T> ptr2(ptr1);

I believe this code should be invalid and should ideally cause a compiler error.  I suspect this is also andersca's worry.

With the copy constructor still defined as in the patch above, the above code continues to be a link error for gcc.  With it removed as you propose, the above code compiles and links, with ptr1.get() == ptr2.get().

[Darin Adler]

(In reply to comment #2)
> Declaring but not defining this constructor is a bug, and from what I can tell not necessary for recent-ish gccs. Can you try just removing that whole block?

We need to make sure that the OwnPtr copy constructor is never actually called. It’s impossible to copy one of these safely. Declaring and not defining it is not a bug, but rather a well-known and understood technique.

We need to carefully study any specific places where the call to the copy constructor is being generated to see what possible safe solutions to the problem are. Anders is well suited to helping us diagnose this since he has done quite a bit of work on clang and is a C++ expert.

[Adrienne Walker]

(In reply to comment #9)
> (In reply to comment #2)
> > Declaring but not defining this constructor is a bug, and from what I can tell not necessary for recent-ish gccs. Can you try just removing that whole block?
> 
> We need to make sure that the OwnPtr copy constructor is never actually called. It’s impossible to copy one of these safely. Declaring and not defining it is not a bug, but rather a well-known and understood technique.

"Declaring and not defining" also usually involves that declaration having a private visibility modifier and doesn't involve that function being implicitly defined but then compiled away so that explicit uses fail at link time.  Usually the function doesn't get used, period.

I'm not saying we shouldn't use this approach, but it's not fair to paint this as any sort of common case.

> We need to carefully study any specific places where the call to the copy constructor is being generated to see what possible safe solutions to the problem are. Anders is well suited to helping us diagnose this since he has done quite a bit of work on clang and is a C++ expert.

I apologize for the length of this comment, but I spent a long time staring at this and there seems to be a lot of confusion on this issue.

Here's my analysis: the copy constructor is primarily being generated as a part of copy-initialization, often in a codesite that looks something like this:

    OwnPtr<SomeClass> local = adoptPtr(SomeClass::create());

The C++ standard requires the copy constructor to be optimized away during copy-initialization by calling a constructor instead in some special cases, such as when both types are identical or the source is derived from the destination.  Unfortunately, PassOwnPtr and OwnPtr have no such relationship, so the non-explicit OwnPtr<T>(const PassOwnPtr<U>&) constructor is used as an implicit conversion followed by the copy constructor in order to copy-initialize this object.

More specifically, the reason this was failing in my hash table (see bug 74154) is because I was using a std::pair<int, int> as the key, but with emptyValueIsZero = false, because (0, 0) is a valid index for my use case.  However, the storage in a hash table is a std::pair<key, value>.  Since OwnPtr's emptyValue() gets passed as a full OwnPtr to the std::pair<key, value> constructor, the copy constructor gets called implicitly.  Oops.

In both of these cases, gcc optimizes these copy constructor usages away, but Clang doesn't manage to in as many cases (although it clearly does in some), so we need to sort out how to accommodate them both compilers.  Personally, I find failing at link time on only one compiler for perfectly safe code to be a really poor experience as a developer.

In my opinion, it would be excellent to have all compilers fail only on improper OwnPtr usage at compile time.  I think we can do this.


Small Question #1: What to do about my HashTable problem?

I can easily work around it in this specific case by offsetting all indices by one so that the combined key/value hash trait can be memset to zero.  There may be some more elegant way to fix this at the hash table level, but I'm willing to punt.


Big Question #2: What to do about other implicit copy constructor uses?

The quick solution is my original patch, which would let gcc continue to catch explicit use of the copy constructor at link time, even if Clang doesn't. This would prevent future issues where Clang failed to compile on code that gcc was happy with, while still being safe.  However, this still leaves us in a bad state where developers building with Clang might break gcc code unknowingly.

A better solution would be to have PassOwnPtr (non-publicly) inherit from OwnPtr.  This sounds awkward at first blush, but they have the same underlying storage and behave identically during destruction, so it's not as strange as it first seems.

This approach would allow all copy-initialization of OwnPtr via PassOwnPtr to call the OwnPtr(PassOwnPtr) constructor rather than the OwnPtr(OwnPtr) copy constructor.  And, in doing so, we could properly make the OwnPtr copy constructor private, just like WTF_MAKE_NONCOPYABLE does.  Sounds great so far, right?

Unfortunately, there's a catch--in doing this, you lose the ability to copy-initialize an OwnPtr with a PassOwnPtr of a derived type:

    class Base;
    class Derived : public Base;
    OwnPtr<Base> local = adoptPtr(new Derived);
    // Note: "OwnPtr<Base> local(adoptPtr(new Derived));" will still compile

Because PassOwnPtr<Derived> and OwnPtr<Base> are of different types, copy-initialize still uses OwnPtr<T>(PassOwnPtr<U>) for implicit conversion and so we're right back into the original problem with the OwnPtr<T> copy constructor getting called.  As I understand it, the PassOwnPtr type is already resolved prior to considering OwnPtr constructors, so a conversion operator on PassOwnPtr can't fix this problem.

However, in building Chromium's chrome, webkit_unit_tests, and DumpRenderTree targets I only found 6 places with this derived copy-initialization behavior.  All of these codesites are easily fixed by changing "T x = y" copy-initialization to "T x(y)" direct-initialization.  There might be more in other ports or targets, but I think that's a reasonable estimate for how small this problem is.

Having PassOwnPtr derive from OwnPtr isn't a perfect solution, but I find the minor tradeoff of not being able to do derived copy-initialization an acceptable loss for being able to have all compilers fail at compile time on unsafe code and none fail at link time on safe code.

In summary, my proposal is:

* have PassOwnPtr non-publicly derive from OwnPtr
* make OwnPtr truly noncopyable by making its copy constructor private
* fix up these six codesites using derived copy-initialization
* fix up any other breakages on other ports or targets
* add a comment to OwnPtr's now-private copy constructor explaining to developers why their compile fails and that they should switch copy-initialization to direct-initialization

If this sounds palatable, I'll clean up my work-in-progress patch and put it up for review.

[Darin Adler]

(In reply to comment #10)
> A better solution would be to have PassOwnPtr (non-publicly) inherit from OwnPtr.

That sounds fine if we can make it work.

> Unfortunately, there's a catch--in doing this, you lose the ability to copy-initialize an OwnPtr with a PassOwnPtr of a derived type:
> 
>     class Base;
>     class Derived : public Base;
>     OwnPtr<Base> local = adoptPtr(new Derived);
>     // Note: "OwnPtr<Base> local(adoptPtr(new Derived));" will still compile

We don’t want to lose that.

> Having PassOwnPtr derive from OwnPtr isn't a perfect solution, but I find the minor tradeoff of not being able to do derived copy-initialization an acceptable loss for being able to have all compilers fail at compile time on unsafe code and none fail at link time on safe code.

I’m not sure I agree about that tradeoff. You mentioned the 6 places where we use polymorphic types with OwnPtr, but I think that’s an important idiom. Probably more important than hash maps with non-zero key values and OwnPtr mapped values. I think we may want to keep looking for even better solution before doing anything here.

Anders, what do you think?

[Adrienne Walker]

Created attachment 120413 [details]
Make PassOwnPtr derive from OwnPtr

[Early Warning System Bot]

Comment on attachment 120413 [details]
Make PassOwnPtr derive from OwnPtr

Attachment 120413 [details] did not pass qt-ews (qt):
Output: http://queues.webkit.org/results/11018103

[Adrienne Walker]

(In reply to comment #12)
> Created an attachment (id=120413) [details]
> Make PassOwnPtr derive from OwnPtr

I just wanted to put up a concrete patch for further discussion.

I continue to think that the OwnPtr code as-is is problematic in that safe code can cause link errors for compilers that don't implement the exact same optimizations as gcc.

The hashmap issue is entirely secondary--this patch wouldn't solve it, although it would have helped catch its errors before it was committed.

[Gyuyoung Kim]

Comment on attachment 120413 [details]
Make PassOwnPtr derive from OwnPtr

Attachment 120413 [details] did not pass efl-ews (efl):
Output: http://queues.webkit.org/results/11019101

[Gustavo Noronha (kov)]

Comment on attachment 120413 [details]
Make PassOwnPtr derive from OwnPtr

Attachment 120413 [details] did not pass gtk-ews (gtk):
Output: http://queues.webkit.org/results/10939113

[Anders Carlsson]

I don't think we need to/should make PassOwnPtr derive from OwnPtr. 

Since this error only happens with clang, and since we know clang supports rvalue references as a C++98 extension, it should be possible to add move constructors and move assignment operators to OwnPtr.h. Clang should choose the move constructor instead of the undefined copy constructor when possible. I'll attach a test patch that does this.

[Anders Carlsson]

Created attachment 121166 [details]
Patch

[Adrienne Walker]

(In reply to comment #18)
> Created an attachment (id=121166) [details]
> Patch

Hmm.  This doesn't seem to do the trick for my HashMap case.  It still fails with a link error for the copy constructor on Clang, but builds on gcc.  It generates the copy constructor in a different way.

Here's a log where I made the copy constructor private (along with your patch and the patch from 74154), to demonstrate where it is being implicitly generated: http://build.chromium.org/p/tryserver.chromium/builders/linux_clang/builds/5644/steps/compile/logs/stdio

There's also another example I just ran into of Visual Studio failing to perform RVO and requiring an OwnPtr copy constructor.  GCC built fine, but Visual Studio failed with a link error.  See bug 75557.

I think I just want OwnPtr usage to fail consistently across compilers, independent of the optimizations that they perform.

[James Robinson]

Compile error (that log won't live forever):

In file included from third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:31:
In file included from third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.h:32:
In file included from third_party/WebKit/Source/WebCore/platform/graphics/IntRect.h:29:
In file included from third_party/WebKit/Source/WebCore/platform/graphics/IntPoint.h:30:
In file included from third_party/WebKit/Source/JavaScriptCore/wtf/MathExtras.h:29:
In file included from /usr/lib/gcc/x86_64-linux-gnu/4.4/../../../../include/c++/4.4/algorithm:60:
In file included from /usr/lib/gcc/x86_64-linux-gnu/4.4/../../../../include/c++/4.4/bits/stl_algobase.h:66:
/usr/lib/gcc/x86_64-linux-gnu/4.4/../../../../include/c++/4.4/bits/stl_pair.h:67:12:error: field of type 'WTF::OwnPtr<WebCore::CCLayerTilingData::Tile>' has private copy constructor
    struct pair
           ^
third_party/WebKit/Source/JavaScriptCore/wtf/HashTable.h:661:9: note: in instantiation of function template specialization 'WTF::HashTableBucketInitializer<false>::initialize<WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >' requested here
        HashTableBucketInitializer<Traits::emptyValueIsZero>::template initialize<Traits>(bucket);
        ^
third_party/WebKit/Source/JavaScriptCore/wtf/HashTable.h:724:13: note: in instantiation of member function 'WTF::HashTable<std::pair<int, int>, std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >, WTF::PairFirstExtractor<std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WTF::PairHash<int, int>, WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WebCore::CCLayerTilingData::TileMapKeyTraits>::initializeBucket' requested here
            initializeBucket(*deletedEntry);
            ^
third_party/WebKit/Source/JavaScriptCore/wtf/HashMap.h:325:32: note: in instantiation of function template specialization 'WTF::HashTable<std::pair<int, int>, std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >, WTF::PairFirstExtractor<std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WTF::PairHash<int, int>, WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WebCore::CCLayerTilingData::TileMapKeyTraits>::add<WTF::HashMapTranslator<WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WTF::PairHash<int, int> >, std::pair<int, int>, WTF::PassOwnPtr<WebCore::CCLayerTilingData::Tile> >' requested here
        return m_impl.template add<HashMapTranslator<ValueTraits, HashFunctions> >(key, mapped);
                               ^
third_party/WebKit/Source/JavaScriptCore/wtf/HashMap.h:352:16: note: in instantiation of member function 'WTF::HashMap<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile>, WTF::PairHash<int, int>, WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >::inlineAdd' requested here
        return inlineAdd(key, mapped);
               ^
third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:81:13: note: in instantiation of member function 'WTF::HashMap<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile>, WTF::PairHash<int, int>, WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >::add' requested here
    m_tiles.add(make_pair(i, j), tile);
            ^
third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtr.h:90:9: note: declared private here
        OwnPtr(const OwnPtr<ValueType>&);
        ^
third_party/WebKit/Source/JavaScriptCore/wtf/HashTable.h:644:36: note: implicit default copy constructor for 'std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >' first required here
            new (NotNull, &bucket) Value(Traits::emptyValue());
                                   ^
In file included from third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:31:
In file included from third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.h:32:
In file included from third_party/WebKit/Source/WebCore/platform/graphics/IntRect.h:29:
In file included from third_party/WebKit/Source/WebCore/platform/graphics/IntPoint.h:30:
In file included from third_party/WebKit/Source/JavaScriptCore/wtf/MathExtras.h:29:
In file included from /usr/lib/gcc/x86_64-linux-gnu/4.4/../../../../include/c++/4.4/algorithm:60:
In file included from /usr/lib/gcc/x86_64-linux-gnu/4.4/../../../../include/c++/4.4/bits/stl_algobase.h:66:
/usr/lib/gcc/x86_64-linux-gnu/4.4/../../../../include/c++/4.4/bits/stl_pair.h:101:4:error: field of type 'WTF::OwnPtr<WebCore::CCLayerTilingData::Tile>' has private copy constructor
          second(__p.second) { }
          ^
third_party/WebKit/Source/JavaScriptCore/wtf/HashTraits.h:149:41: note: in instantiation of function template specialization 'std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >' requested here
        static TraitType emptyValue() { return make_pair(FirstTraits::emptyValue(), SecondTraits::emptyValue()); }
                                        ^
third_party/WebKit/Source/JavaScriptCore/wtf/HashTable.h:644:50: note: in instantiation of member function 'WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >::emptyValue' requested here
            new (NotNull, &bucket) Value(Traits::emptyValue());

                                                 ^
third_party/WebKit/Source/JavaScriptCore/wtf/HashTable.h:661:9: note: in instantiation of function template specialization 'WTF::HashTableBucketInitializer<false>::initialize<WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >' requested here
        HashTableBucketInitializer<Traits::emptyValueIsZero>::template initialize<Traits>(bucket);
        ^
third_party/WebKit/Source/JavaScriptCore/wtf/HashTable.h:724:13: note: in instantiation of member function 'WTF::HashTable<std::pair<int, int>, std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >, WTF::PairFirstExtractor<std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WTF::PairHash<int, int>, WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WebCore::CCLayerTilingData::TileMapKeyTraits>::initializeBucket' requested here
            initializeBucket(*deletedEntry);
            ^
third_party/WebKit/Source/JavaScriptCore/wtf/HashMap.h:325:32: note: in instantiation of function template specialization 'WTF::HashTable<std::pair<int, int>, std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> >, WTF::PairFirstExtractor<std::pair<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WTF::PairHash<int, int>, WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WebCore::CCLayerTilingData::TileMapKeyTraits>::add<WTF::HashMapTranslator<WTF::PairHashTraits<WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >, WTF::PairHash<int, int> >, std::pair<int, int>, WTF::PassOwnPtr<WebCore::CCLayerTilingData::Tile> >' requested here
        return m_impl.template add<HashMapTranslator<ValueTraits, HashFunctions> >(key, mapped);
                               ^

third_party/WebKit/Source/JavaScriptCore/wtf/HashMap.h:352:16: note: in instantiation of member function 'WTF::HashMap<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile>, WTF::PairHash<int, int>, WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >::inlineAdd' requested here
        return inlineAdd(key, mapped);
               ^
third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.cpp:81:13: note: in instantiation of member function 'WTF::HashMap<std::pair<int, int>, WTF::OwnPtr<WebCore::CCLayerTilingData::Tile>, WTF::PairHash<int, int>, WebCore::CCLayerTilingData::TileMapKeyTraits, WTF::HashTraits<WTF::OwnPtr<WebCore::CCLayerTilingData::Tile> > >::add' requested here
    m_tiles.add(make_pair(i, j), tile);
            ^
third_party/WebKit/Source/JavaScriptCore/wtf/OwnPtr.h:90:9: note: declared private here
        OwnPtr(const OwnPtr<ValueType>&);
        ^
2 errors generated.
make: *** [out/Debug/obj.target/webcore_platform/third_party/WebKit/Source/WebCore/platform/graphics/chromium/cc/CCLayerTilingData.o] Error 1

[Adrienne Walker]

A belated follow-up.

As long as we have a public declared-but-not-defined copy constructor, we are always going to depend on inconsistent compiler optimizations, where different platforms may or may not be able to compile and link the same code.  It's not just Clang, although Clang seems to be more different more often.

andersca's patch to add rvalue reference functions to OwnPtr is great but it only hits one of these optimizations. I think that will be true of any solution that doesn't involve the copy constructor being made private or "= delete".

Hypothetically speaking, if every compiler supported rvalue references, then andersca's patch with a follow-up of making the copy constructor private could be workable.  However, since they don't, this can't be done.

I continue to think that the PassOwnPtr-derives-from-OwnPtr patch has less smell than the current behavior.  It may disallow one rarely-used case that should be allowed, but it will always fail all bad cases early and it will always fail consistently across all compilers.

darin, andersca: Do you have any thoughts or other proposals? Do you feel like that solution is workable or do you feel like the cure is worse than the disease?

[Darin Adler]

(In reply to comment #21)
> andersca's patch to add rvalue reference functions to OwnPtr is great but it only hits one of these optimizations. I think that will be true of any solution that doesn't involve the copy constructor being made private or "= delete".
>
> Hypothetically speaking, if every compiler supported rvalue references, then andersca's patch with a follow-up of making the copy constructor private could be workable.  However, since they don't, this can't be done.

We can make the copy constructor private on any compiler that supports rvalue references. Since clang is such a compiler it will catch mistakes for us. And we haven’t had problems with compilers other than clang yet, so lets not make changes based on fear for the future.

That’s what I think we should do.

[Adrienne Walker]

(In reply to comment #22)
> (In reply to comment #21)
> > andersca's patch to add rvalue reference functions to OwnPtr is great but it only hits one of these optimizations. I think that will be true of any solution that doesn't involve the copy constructor being made private or "= delete".
> >
> > Hypothetically speaking, if every compiler supported rvalue references, then andersca's patch with a follow-up of making the copy constructor private could be workable.  However, since they don't, this can't be done.
> 
> We can make the copy constructor private on any compiler that supports rvalue references. Since clang is such a compiler it will catch mistakes for us. And we haven’t had problems with compilers other than clang yet, so lets not make changes based on fear for the future.

Unfortunately, that will do nothing for the status quo.  Other compilers will continue to successfully compile and link code that will not work for Clang, causing unexpected breakages because that particular compiler doesn't have a private OwnPtr copy constructor.  The problem is not improper use of the copy constructor, but the dependency on inconsistent compiler optimizations to remove transient but proper usage.

Also, it's not just Clang.  In bug 75557, Visual Studio failed to perform named return value optimization and generated an OwnPtr copy constructor.  gcc built fine, but Visual Studio failed with a link error.

[Darin Adler]

(In reply to comment #23)
> The problem is not improper use of the copy constructor, but the dependency on inconsistent compiler optimizations to remove transient but proper usage.

I don’t agree with this analysis.

[Adrienne Walker]

(In reply to comment #24)
> (In reply to comment #23)
> > The problem is not improper use of the copy constructor, but the dependency on inconsistent compiler optimizations to remove transient but proper usage.
> 
> I don’t agree with this analysis.

How would you state the problem?

[Anders Carlsson]

I think that we should treat the current behavior as the legacy behavior, and not spend more time on trying to fix it given that it seems to work well enough.

The C++11 way with a move constructor and move assignment operator has well-defined semantics and works on most modern compilers (including VS2010, which we'd have to adopt on Windows).

I think the problem in https://bugs.webkit.org/show_bug.cgi?id=74154 is that the traits for the key in HashMap< TileMapKey, OwnPtr<Tile>> specifies emptyValueIsZero = false.

This means that the traits for the underlying HashTable will have emptyValueIsZero = false set as well, which causes the emptyValue to be constructed using

        static TraitType emptyValue() { return make_pair(FirstTraits::emptyValue(), SecondTraits::emptyValue()); }

and that's what ends up trying to copy the OwnPtr.

[Adrienne Walker]

(In reply to comment #26)
> I think that we should treat the current behavior as the legacy behavior, and not spend more time on trying to fix it given that it seems to work well enough.

I don't think the current behavior *does* work well enough, because I keep running into cases where different compilers behave differently.  However, it's pretty clear that nobody else feels this way.

> The C++11 way with a move constructor and move assignment operator has well-defined semantics and works on most modern compilers (including VS2010, which we'd have to adopt on Windows).

Is the path to move forward to first land that patch for rvalue references and Clang? Then, make the OwnPtr copy constructor private if that feature is enabled (and fix the build errors that causes.)  Then, consider enabling that feature for other compilers on an individual basis?

[Adrienne Walker]

The public OwnPtr copy constructor continues to cause ongoing build failures for Chromium:

http://build.webkit.org/builders/Chromium%20Mac%20Release/builds/40233/steps/compile-webkit/logs/stdio

[Darin Adler]

Comment on attachment 120413 [details]
Make PassOwnPtr derive from OwnPtr

We should fix this using a move constructor. Not by making PassOwnPtr derive from OwnPtr. I suggest starting with Anders’s patch and making it work.

[Adrienne Walker]

(In reply to comment #29)
> (From update of attachment 120413 [details])
> We should fix this using a move constructor. Not by making PassOwnPtr derive from OwnPtr. I suggest starting with Anders’s patch and making it work.

How does the move constructor fix the Visual Studio error mentioned in comment #19?

The source of the problem is the public copy constructor.  If all WebKit platforms supported the move constructor, then we could remove it, but this doesn't seem to be the case.

[Adrienne Walker]

Created attachment 160019 [details]
andersca's patch + #if'd out copy constructor

[WebKit Review Bot]

Attachment 160019 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ow..." exit_code: 1
Source/WTF/wtf/OwnPtr.h:79:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/OwnPtr.h:82:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Nico Weber]

Comment on attachment 160019 [details]
andersca's patch + #if'd out copy constructor

View in context: https://bugs.webkit.org/attachment.cgi?id=160019&action=review

Not sure if this is something the project cares about:

> Source/WTF/wtf/OwnPtr.h:49
> +#if !COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)

Note: All except very recent gdb's (xcode 4.4 on mac, bug not yet fixed for official gdb afaik) can't handle the symbols that recent compilers produce for rvalue references. That's already a bit of a problem and makes it impossible to debug parts of webkit with many toolchains. Making this change would probably mean that all of webkit becomes undebuggable, as OwnPtr is used so widely.

[Adrienne Walker]

(In reply to comment #32)
> Attachment 160019 [details] did not pass style-queue:
> 
> Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ow..." exit_code: 1
> Source/WTF/wtf/OwnPtr.h:79:  Missing spaces around &&  [whitespace/operators] [3]
> Source/WTF/wtf/OwnPtr.h:82:  Missing spaces around &&  [whitespace/operators] [3]
> Total errors found: 2 in 2 files
> 
> 
> If any of these errors are false positives, please file a bug against check-webkit-style.

Filed bug 94753.

[Adrienne Walker]

(In reply to comment #33)
> (From update of attachment 160019 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=160019&action=review
> 
> Not sure if this is something the project cares about:
> 
> > Source/WTF/wtf/OwnPtr.h:49
> > +#if !COMPILER_SUPPORTS(CXX_RVALUE_REFERENCES)
> 
> Note: All except very recent gdb's (xcode 4.4 on mac, bug not yet fixed for official gdb afaik) can't handle the symbols that recent compilers produce for rvalue references. That's already a bit of a problem and makes it impossible to debug parts of webkit with many toolchains. Making this change would probably mean that all of webkit becomes undebuggable, as OwnPtr is used so widely.

In my own experience, this is already a pervasive issue even before this patch is applied given that rvalue references are already used by RetainPtr, Vector, AtomicString, and WTFString.

I wonder if maybe we should not say that the CXX_RVALUE_REFERENCES are supported when compiling with older versions of Xcode, but I also think that it's a separate bug from this one.

[Adrienne Walker]

Created attachment 161011 [details]
Also make copy constructor private

[WebKit Review Bot]

Attachment 161011 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ow..." exit_code: 1
Source/WTF/wtf/OwnPtr.h:79:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/OwnPtr.h:82:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Adrienne Walker]

darin, andersca: review?

[Anders Carlsson]

Comment on attachment 161011 [details]
Also make copy constructor private

View in context: https://bugs.webkit.org/attachment.cgi?id=161011&action=review

> Source/WTF/wtf/OwnPtr.h:91
> +        OwnPtr(const OwnPtr<T>&);

I don't think the <T> is needed here.

We should really add a WTF_MARK_DELETED macro in Compiler.h that expands to "= delete" for compilers that support deleted functions.

> Source/WTF/wtf/OwnPtr.h:93
>          OwnPtr& operator=(const OwnPtr<T>&);

Same comment here (sorta).

[Adrienne Walker]

(In reply to comment #39)
> (From update of attachment 161011 [details])
> View in context: https://bugs.webkit.org/attachment.cgi?id=161011&action=review
> 
> > Source/WTF/wtf/OwnPtr.h:91
> > +        OwnPtr(const OwnPtr<T>&);
> 
> I don't think the <T> is needed here.
> 
> We should really add a WTF_MARK_DELETED macro in Compiler.h that expands to "= delete" for compilers that support deleted functions.

WTF_MAKE_NONCOPYABLE already does that.  It makes the #ifdefs look a little weird, but I can do that if rvalue references are supported.

I'll put up another patch.  Please take a look.  :)

[Adrienne Walker]

Created attachment 163220 [details]
Use Noncopyable

[WebKit Review Bot]

Attachment 163220 [details] did not pass style-queue:

Failed to run "['Tools/Scripts/check-webkit-style', '--diff-files', u'Source/WTF/ChangeLog', u'Source/WTF/wtf/Ow..." exit_code: 1
Source/WTF/wtf/OwnPtr.h:85:  Missing spaces around &&  [whitespace/operators] [3]
Source/WTF/wtf/OwnPtr.h:88:  Missing spaces around &&  [whitespace/operators] [3]
Total errors found: 2 in 2 files


If any of these errors are false positives, please file a bug against check-webkit-style.

[Anders Carlsson]

Comment on attachment 163220 [details]
Use Noncopyable

Looks great! Thanks for tackling this!

[WebKit Review Bot]

Comment on attachment 163220 [details]
Use Noncopyable

Clearing flags on attachment: 163220

Committed r128203: <http://trac.webkit.org/changeset/128203>

[WebKit Review Bot]

All reviewed patches have been landed.  Closing bug.