Bug 159931 – Fix a linking failure caused by NetworkCache::Data::~Data()

• Component: New Bugs
• Status: REOPENED

[ChangSeok Oh]

A linking failure happens when building with clang 3.6.

>[2068/2122] Linking CXX shared library lib/libwebkit2gtk-4.0.so.37.14.2
>FAILED: : && /home/changseok/Projects/api-sanitizer/bin/llvm/bin/clang++  -fPIC -Wno-error  -std=c++1y -fcolor-diagnostics -Qunused-arguments -g  -Wl,--no-undefined  -L/home/changseok/Projects/WebKit/WebKitBuild/DependenciesGTK/Root/lib  -fuse-ld=gold -Wl,--disable-new-dtags -fuse-ld=gold -Wl,--disable-new-dtags -shared -Wl,-soname,libwebkit2gtk-4.0.so.37 -o lib/libwebkit2gtk-4.0.so.37.14.2 @CMakeFiles/WebKit2.rsp  && :
>../../Source/WebKit2/NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp:76: error: undefined reference to 'WebKit::NetworkCache::Data::~Data()'
>clang-3.6: error: linker command failed with exit code 1 (use -v to see invocation)

[ChangSeok Oh]

Created attachment 284022 [details]
Patch

[ChangSeok Oh]

Created attachment 284023 [details]
Patch

[Chris Dumez]

Comment on attachment 284023 [details]
Patch

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

> Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109
> +    virtual ~Data() { }

Why would you make this class virtual??

[Chris Dumez]

Comment on attachment 284023 [details]
Patch

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

>> Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109
>> +    virtual ~Data() { }
> 
> Why would you make this class virtual??

does "~Data() = default;" fix your build error?

[ChangSeok Oh]

(In reply to comment #4)
> Comment on attachment 284023 [details]
> Patch
> 
> View in context:
> https://bugs.webkit.org/attachment.cgi?id=284023&action=review
> 
> >> Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109
> >> +    virtual ~Data() { }
> > 
> > Why would you make this class virtual??
That is meaningless here. Removed.
 
> 
> does "~Data() = default;" fix your build error?
No, it doesn't work. It spawns other compile errors like...

../../Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:99:29: error: attempt to use a deleted function
auto existingData = mapFile(blobPath.data());

../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:446:19: error: attempt to use a deleted function
auto record = cacheEntry->encodeAsStorageRecord();

[ChangSeok Oh]

Created attachment 284043 [details]
Patch

[WebKit Commit Bot]

Comment on attachment 284043 [details]
Patch

Clearing flags on attachment: 284043

Committed r203467: <http://trac.webkit.org/changeset/203467>

[WebKit Commit Bot]

All reviewed patches have been landed.  Closing bug.

[Michael Catanzaro]

*** Bug 159264 has been marked as a duplicate of this bug. ***

[Darin Adler]

Why is this change needed? This kind of change should have no effect. Is this a compiler bug?

[Darin Adler]

(In reply to comment #5)
> > does "~Data() = default;" fix your build error?
> No, it doesn't work. It spawns other compile errors like...
> 
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:99:29:
> error: attempt to use a deleted function
> auto existingData = mapFile(blobPath.data());
> 
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:446:19: error:
> attempt to use a deleted function
> auto record = cacheEntry->encodeAsStorageRecord();

Those sound like what you would get if you tried "~Data() = delete;", not "~Data() = default;".

This change is highly suspect. Explicitly defining an empty inline constructor should have no effect!

[ChangSeok Oh]

(In reply to comment #11)
> (In reply to comment #5)
> > > does "~Data() = default;" fix your build error?
> > No, it doesn't work. It spawns other compile errors like...
> > 
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:99:29:
> > error: attempt to use a deleted function
> > auto existingData = mapFile(blobPath.data());
> > 
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:446:19: error:
> > attempt to use a deleted function
> > auto record = cacheEntry->encodeAsStorageRecord();
> 
> Those sound like what you would get if you tried "~Data() = delete;", not
> "~Data() = default;".
> 
> This change is highly suspect. Explicitly defining an empty inline
> constructor should have no effect!

Nah.. An incorrect log mislead you. Here is correct full logs. Sorry about this confusion.

../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:410:19: error: attempt to use a deleted function
    auto record = cacheEntry->encodeAsStorageRecord();
                  ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note: destructor of 'Record' is implicitly deleted because field 'header' has a deleted destructor
        Data header;
             ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note: '~Data' has been explicitly marked deleted here
    ~Data() = delete;
    ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:446:19: error: attempt to use a deleted function
    auto record = cacheEntry->encodeAsStorageRecord();
                  ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note: destructor of 'Record' is implicitly deleted because field 'header' has a deleted destructor
        Data header;
             ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note: '~Data' has been explicitly marked deleted here
    ~Data() = delete;
    ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:461:25: error: attempt to use a deleted function
    auto updateRecord = updateEntry->encodeAsStorageRecord();
                        ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note: destructor of 'Record' is implicitly deleted because field 'header' has a deleted destructor
        Data header;
             ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note: '~Data' has been explicitly marked deleted here
    ~Data() = delete;
    ^
In file included from ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:26:
In file included from ../../Source/WebKit2/config.h:44:
In file included from ../../Source/WTF/wtf/FastMalloc.h:26:
In file included from ../../Source/WTF/wtf/StdLibExtras.h:31:
In file included from /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/memory:81:
/usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/bits/unique_ptr.h:76:2: error: attempt to use a deleted function
        delete __ptr;
        ^
/usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/bits/unique_ptr.h:236:4: note: in instantiation of member function 'std::default_delete<WebKit::NetworkCache::Entry>::operator()' requested here
          get_deleter()(__ptr);
          ^
/usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/bits/unique_ptr.h:201:50: note: in instantiation of member function 'std::unique_ptr<WebKit::NetworkCache::Entry, std::default_delete<WebKit::NetworkCache::Entry> >::~unique_ptr' requested here
      constexpr unique_ptr(nullptr_t) noexcept : unique_ptr() { }
                                                 ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h:92:21: note: destructor of 'Entry' is implicitly deleted because field 'm_sourceStorageRecord' has a deleted destructor
    Storage::Record m_sourceStorageRecord { };
                    ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note: destructor of 'Record' is implicitly deleted because field 'header' has a deleted destructor
        Data header;
             ^
../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note: '~Data' has been explicitly marked deleted here
    ~Data() = delete;
    ^

[ChangSeok Oh]

(In reply to comment #10)
> Why is this change needed? This kind of change should have no effect. Is
> this a compiler bug?

That is a possible suspicion. Because this linking issue was found with clang 3.6 which is far from the latest. Let me try recent one and leave a note about it.

[Chris Dumez]

(In reply to comment #12)
> (In reply to comment #11)
> > (In reply to comment #5)
> > > > does "~Data() = default;" fix your build error?
> > > No, it doesn't work. It spawns other compile errors like...
> > > 
> > > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:99:29:
> > > error: attempt to use a deleted function
> > > auto existingData = mapFile(blobPath.data());
> > > 
> > > ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:446:19: error:
> > > attempt to use a deleted function
> > > auto record = cacheEntry->encodeAsStorageRecord();
> > 
> > Those sound like what you would get if you tried "~Data() = delete;", not
> > "~Data() = default;".
> > 
> > This change is highly suspect. Explicitly defining an empty inline
> > constructor should have no effect!
> 
> Nah.. An incorrect log mislead you. Here is correct full logs. Sorry about
> this confusion.
> 
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:410:19: error:
> attempt to use a deleted function
>     auto record = cacheEntry->encodeAsStorageRecord();
>                   ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note:
> destructor of 'Record' is implicitly deleted because field 'header' has a
> deleted destructor
>         Data header;
>              ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note:
> '~Data' has been explicitly marked deleted here
>     ~Data() = delete;
>     ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:446:19: error:
> attempt to use a deleted function
>     auto record = cacheEntry->encodeAsStorageRecord();
>                   ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note:
> destructor of 'Record' is implicitly deleted because field 'header' has a
> deleted destructor
>         Data header;
>              ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note:
> '~Data' has been explicitly marked deleted here
>     ~Data() = delete;
>     ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:461:25: error:
> attempt to use a deleted function
>     auto updateRecord = updateEntry->encodeAsStorageRecord();
>                         ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note:
> destructor of 'Record' is implicitly deleted because field 'header' has a
> deleted destructor
>         Data header;
>              ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note:
> '~Data' has been explicitly marked deleted here
>     ~Data() = delete;
>     ^
> In file included from
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:26:
> In file included from ../../Source/WebKit2/config.h:44:
> In file included from ../../Source/WTF/wtf/FastMalloc.h:26:
> In file included from ../../Source/WTF/wtf/StdLibExtras.h:31:
> In file included from
> /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/memory:
> 81:
> /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/bits/
> unique_ptr.h:76:2: error: attempt to use a deleted function
>         delete __ptr;
>         ^
> /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/bits/
> unique_ptr.h:236:4: note: in instantiation of member function
> 'std::default_delete<WebKit::NetworkCache::Entry>::operator()' requested here
>           get_deleter()(__ptr);
>           ^
> /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/bits/
> unique_ptr.h:201:50: note: in instantiation of member function
> 'std::unique_ptr<WebKit::NetworkCache::Entry,
> std::default_delete<WebKit::NetworkCache::Entry> >::~unique_ptr' requested
> here
>       constexpr unique_ptr(nullptr_t) noexcept : unique_ptr() { }
>                                                  ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h:92:21: note:
> destructor of 'Entry' is implicitly deleted because field
> 'm_sourceStorageRecord' has a deleted destructor
>     Storage::Record m_sourceStorageRecord { };
>                     ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note:
> destructor of 'Record' is implicitly deleted because field 'header' has a
> deleted destructor
>         Data header;
>              ^
> ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note:
> '~Data' has been explicitly marked deleted here
>     ~Data() = delete;
>     ^

If you look at this last line, the compiler says you deleted the Data destructor:
~Data() = delete;

The workaround I suggested was:
~Data() = default;

[ChangSeok Oh]

(In reply to comment #14)
> (In reply to comment #12)
> > (In reply to comment #11)
> > > (In reply to comment #5)
> > > > > does "~Data() = default;" fix your build error?
> > > > No, it doesn't work. It spawns other compile errors like...
> > > > 
> > > > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheBlobStorage.cpp:99:29:
> > > > error: attempt to use a deleted function
> > > > auto existingData = mapFile(blobPath.data());
> > > > 
> > > > ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:446:19: error:
> > > > attempt to use a deleted function
> > > > auto record = cacheEntry->encodeAsStorageRecord();
> > > 
> > > Those sound like what you would get if you tried "~Data() = delete;", not
> > > "~Data() = default;".
> > > 
> > > This change is highly suspect. Explicitly defining an empty inline
> > > constructor should have no effect!
> > 
> > Nah.. An incorrect log mislead you. Here is correct full logs. Sorry about
> > this confusion.
> > 
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:410:19: error:
> > attempt to use a deleted function
> >     auto record = cacheEntry->encodeAsStorageRecord();
> >                   ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note:
> > destructor of 'Record' is implicitly deleted because field 'header' has a
> > deleted destructor
> >         Data header;
> >              ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note:
> > '~Data' has been explicitly marked deleted here
> >     ~Data() = delete;
> >     ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:446:19: error:
> > attempt to use a deleted function
> >     auto record = cacheEntry->encodeAsStorageRecord();
> >                   ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note:
> > destructor of 'Record' is implicitly deleted because field 'header' has a
> > deleted destructor
> >         Data header;
> >              ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note:
> > '~Data' has been explicitly marked deleted here
> >     ~Data() = delete;
> >     ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:461:25: error:
> > attempt to use a deleted function
> >     auto updateRecord = updateEntry->encodeAsStorageRecord();
> >                         ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note:
> > destructor of 'Record' is implicitly deleted because field 'header' has a
> > deleted destructor
> >         Data header;
> >              ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note:
> > '~Data' has been explicitly marked deleted here
> >     ~Data() = delete;
> >     ^
> > In file included from
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCache.cpp:26:
> > In file included from ../../Source/WebKit2/config.h:44:
> > In file included from ../../Source/WTF/wtf/FastMalloc.h:26:
> > In file included from ../../Source/WTF/wtf/StdLibExtras.h:31:
> > In file included from
> > /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/memory:
> > 81:
> > /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/bits/
> > unique_ptr.h:76:2: error: attempt to use a deleted function
> >         delete __ptr;
> >         ^
> > /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/bits/
> > unique_ptr.h:236:4: note: in instantiation of member function
> > 'std::default_delete<WebKit::NetworkCache::Entry>::operator()' requested here
> >           get_deleter()(__ptr);
> >           ^
> > /usr/lib/gcc/x86_64-redhat-linux/6.1.1/../../../../include/c++/6.1.1/bits/
> > unique_ptr.h:201:50: note: in instantiation of member function
> > 'std::unique_ptr<WebKit::NetworkCache::Entry,
> > std::default_delete<WebKit::NetworkCache::Entry> >::~unique_ptr' requested
> > here
> >       constexpr unique_ptr(nullptr_t) noexcept : unique_ptr() { }
> >                                                  ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheEntry.h:92:21: note:
> > destructor of 'Entry' is implicitly deleted because field
> > 'm_sourceStorageRecord' has a deleted destructor
> >     Storage::Record m_sourceStorageRecord { };
> >                     ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheStorage.h:58:14: note:
> > destructor of 'Record' is implicitly deleted because field 'header' has a
> > deleted destructor
> >         Data header;
> >              ^
> > ../../Source/WebKit2/NetworkProcess/cache/NetworkCacheData.h:109:5: note:
> > '~Data' has been explicitly marked deleted here
> >     ~Data() = delete;
> >     ^
> 
> If you look at this last line, the compiler says you deleted the Data
> destructor:
> ~Data() = delete;
> 
> The workaround I suggested was:
> ~Data() = default;

Oh.. my terrible mistake. Let me fix it.

[Chris Dumez]

That said, I agree with Darin that it looks like a compiler/linker bug as there is no reason we should need an explicit destructor here :/

[Javier Fernandez]

(In reply to comment #16)
> That said, I agree with Darin that it looks like a compiler/linker bug as
> there is no reason we should need an explicit destructor here :/

I've been investigating this issue in bug #159264 and I think the proposed patch here is not correct. As darin suggested it seems a compiler bug because just doing a change like this avoids the compilation error:

diff --git a/Source/WebKit2/NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp b/Source/WebKit2/NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp
index 4726979..c4bea12 100644
--- a/Source/WebKit2/NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp
+++ b/Source/WebKit2/NetworkProcess/cache/NetworkCacheSubresourcesEntry.cpp
@@ -72,7 +72,7 @@ Storage::Record SubresourcesEntry::encodeAsStorageRecord() const
 
     encoder.encodeChecksum();
 
-    return { m_key, m_timeStamp, { encoder.buffer(), encoder.bufferSize() } , { } };
+    return { m_key, m_timeStamp, { encoder.buffer(), encoder.bufferSize() } , Data() };

[ChangSeok Oh]

O.K This needs more touch.

[ChangSeok Oh]

I confirmed that clang3.6-3.9 have the same issue. I also tried ~Data() = *default*, but it was not helpful for the linking failure. Instead Javier's approach worked. If his fix mitigates everyone's concern, I am ok with it. 

Javier, feel free to take over this ticket from me. Of course, I can lend my hand to land your fix.

[Darin Adler]

(In reply to comment #19)
> I confirmed that clang3.6-3.9 have the same issue. I also tried ~Data() =
> *default*, but it was not helpful for the linking failure. Instead Javier's
> approach worked. If his fix mitigates everyone's concern, I am ok with it. 

Now that it’s confirmed that this is a compiler bug that we are working around, I don’t have a preference for which of the two workarounds we use. But I do want a comment indicating that this is a workaround for a clang compiler bug and I would like someone to remove this when we no longer need to support the compiler with the bug. And ideally someone would report this to the clang project and we would track when it's fixed too.

[Javier Fernandez]

(In reply to comment #19)
> I confirmed that clang3.6-3.9 have the same issue. I also tried ~Data() =
> *default*, but it was not helpful for the linking failure. Instead Javier's
> approach worked. If his fix mitigates everyone's concern, I am ok with it. 
> 
> Javier, feel free to take over this ticket from me. Of course, I can lend my
> hand to land your fix.

I really don't mind doing it. I still failed to reproduce the clang bug with some reduced test case, though. I'm a bit busy these days, but I think I can do land a workaround during this week, if you don't do it before.

Anyway, if you end up implemented a similar approach to my workaround, I think it'd be better to avoid completely the aggregate-initialization of both Data arguments; it seems the bug is bypassed by just using an explicit instantiation on of of the Data fields, but I think it'd clearer doing it with both.

[Adrian Perez]

Provided that nowadays Clang is at version 9.0, maybe this
bug is not relevant anymore. What do you think? Is this
relevant anymore?