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)
Created attachment 284022 [details] Patch
Created attachment 284023 [details] Patch
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??
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?
(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();
Created attachment 284043 [details] Patch
Comment on attachment 284043 [details] Patch Clearing flags on attachment: 284043 Committed r203467: <http://trac.webkit.org/changeset/203467>
All reviewed patches have been landed. Closing bug.
*** Bug 159264 has been marked as a duplicate of this bug. ***
Why is this change needed? This kind of change should have no effect. Is this a compiler bug?
(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!
(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; ^
(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.
(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;
(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.
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 :/
(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() };
O.K This needs more touch.
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.
(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.
(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.
Provided that nowadays Clang is at version 9.0, maybe this bug is not relevant anymore. What do you think? Is this relevant anymore?