65414 2011-07-30 12:25:56 -0700 DequeIterator lacks default constructor 2011-10-03 03:57:24 -0700 1 1 1 Unclassified WebKit Web Template Framework 528+ (Nightly build) All All RESOLVED LATER P3 Normal --- 1 kbalazs webkit-unassigned darin dglazkov morrita webkit.review.bot oldest_to_newest 444316 0 kbalazs 2011-07-30 12:25:56 -0700 Pointed out in https://bugs.webkit.org/show_bug.cgi?id=65263#c9. 462457 1 106405 kbalazs 2011-09-06 04:30:06 -0700 Created attachment 106405 proposed fix 462475 2 106405 webkit.review.bot 2011-09-06 05:47:19 -0700 Comment on attachment 106405 proposed fix Attachment 106405 did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9594610 462488 3 106405 webkit.review.bot 2011-09-06 06:48:32 -0700 Comment on attachment 106405 proposed fix Attachment 106405 did not pass cr-mac-ews (chromium): Output: http://queues.webkit.org/results/9592827 462556 4 106405 darin 2011-09-06 08:47:07 -0700 Comment on attachment 106405 proposed fix View in context: https://bugs.webkit.org/attachment.cgi?id=106405&action=review > Source/JavaScriptCore/wtf/Deque.h:598 > + , m_next(0) > + , m_previous(0) I’m not sure I understand the full rationale for setting these in a debug build but not a release build. Will this help us catch mistakes we would otherwise miss? Or perhaps it will hide mistakes we might otherwise catch. > Source/JavaScriptCore/wtf/MainThread.cpp:-213 > - // We must redefine 'i' each pass, because the itererator's operator= > - // requires 'this' to be valid, and remove() invalidates all iterators The substance of this comment still seems correct, even if the word redefine is no longer accurate, so it’s not clear to me that it’s good to remove it. 463218 5 kbalazs 2011-09-07 00:57:59 -0700 (In reply to comment #4) > (From update of attachment 106405 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=106405&action=review > > > Source/JavaScriptCore/wtf/Deque.h:598 > > + , m_next(0) > > + , m_previous(0) > > I’m not sure I understand the full rationale for setting these in a debug build but not a release build. Will this help us catch mistakes we would otherwise miss? Or perhaps it will hide mistakes we might otherwise catch. This is just to pass the assertions in removeFromIteratorList with an empty iterator: if (!m_deque) { ASSERT(!m_next); ASSERT(!m_previous); } Maybe ASSERT_DISABLED would be the appropriate condition. > > > Source/JavaScriptCore/wtf/MainThread.cpp:-213 > > - // We must redefine 'i' each pass, because the itererator's operator= > > - // requires 'this' to be valid, and remove() invalidates all iterators > > The substance of this comment still seems correct, even if the word redefine is no longer accurate, so it’s not clear to me that it’s good to remove it. I think this comment is not valid since http://trac.webkit.org/changeset/92050 - my bad to not updated this call site - so we can remove it. 463220 6 kbalazs 2011-09-07 01:09:13 -0700 > > Maybe ASSERT_DISABLED would be the appropriate condition. > On the other hand removeFromIteratorList is in #ifndef NDEBUG so I won't change this in the patch. 463221 7 kbalazs 2011-09-07 01:09:50 -0700 (In reply to comment #3) > (From update of attachment 106405 [details]) > Attachment 106405 [details] did not pass cr-mac-ews (chromium): > Output: http://queues.webkit.org/results/9592827 From the output it does not seems to be my fault. 463841 8 106405 webkit.review.bot 2011-09-07 19:51:27 -0700 Comment on attachment 106405 proposed fix Attachment 106405 did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9599942 463875 9 106405 webkit.review.bot 2011-09-07 21:18:38 -0700 Comment on attachment 106405 proposed fix Attachment 106405 did not pass mac-ews (mac): Output: http://queues.webkit.org/results/9615060 463979 10 kbalazs 2011-09-08 03:44:19 -0700 (In reply to comment #9) > (From update of attachment 106405 [details]) > Attachment 106405 [details] did not pass mac-ews (mac): > Output: http://queues.webkit.org/results/9615060 This is the build error: cc1plus: warnings being treated as errors /Users/balazs/WebKitGit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/MessageQueue.h: In member function 'void* WebCore::DatabaseThread::databaseThread()': /Users/balazs/WebKitGit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/MessageQueue.h:135: warning: 'found.WTF::DequeConstIterator<WebCore::DatabaseTask*, 0ul>::<anonymous>.WTF::DequeIteratorBase<WebCore::DatabaseTask*, 0ul>::m_index' may be used uninitialized in this function /Users/balazs/WebKitGit/WebKitBuild/Release/JavaScriptCore.framework/PrivateHeaders/MessageQueue.h:135: note: 'found.WTF::DequeConstIterator<WebCore::DatabaseTask*, 0ul>::<anonymous>.WTF::DequeIteratorBase<WebCore::DatabaseTask*, 0ul>::m_index' was declared here Honestly I don't get it. It is coming from here: DequeConstIterator<DataType*> found; while (!m_killed && !timedOut && (found = m_queue.findIf(predicate)) == m_queue.end()) timedOut = !m_condition.timedWait(m_mutex, absoluteTime); found.m_index is uninitialized first bug the iterator is assigned before used and operator= sets the value. Maybe a compiler error? 465158 11 darin 2011-09-09 18:27:25 -0700 Probably a compiler bug. 475296 12 morrita 2011-09-29 00:50:03 -0700 > Honestly I don't get it. It is coming from here: > > DequeConstIterator<DataType*> found; > while (!m_killed && !timedOut && (found = m_queue.findIf(predicate)) == m_queue.end()) > timedOut = !m_condition.timedWait(m_mutex, absoluteTime); > > found.m_index is uninitialized first bug the iterator is assigned before used and operator= sets the value. Maybe a compiler error? The default constructor of DequeConstIteratorBase doesn't initialized m_index, and this patch instantiates the constructor code, that causes the warning. You can add "m_index(0)" to the constructor definition. 476845 13 kbalazs 2011-10-03 02:05:35 -0700 (In reply to comment #12) > > Honestly I don't get it. It is coming from here: > > > > DequeConstIterator<DataType*> found; > > while (!m_killed && !timedOut && (found = m_queue.findIf(predicate)) == m_queue.end()) > > timedOut = !m_condition.timedWait(m_mutex, absoluteTime); > > > > found.m_index is uninitialized first bug the iterator is assigned before used and operator= sets the value. Maybe a compiler error? > > The default constructor of DequeConstIteratorBase doesn't initialized m_index, > and this patch instantiates the constructor code, that causes the warning. > You can add "m_index(0)" to the constructor definition. It does not change the fact that it is not used (read) before it has been initialized. The whole point is to avoid unnecessary initializations so adding "m_index(0)" is not a very good solution. I would rather keep the current code - where callers initialize iterators with the end value - if nobody has an idea how to trick over the warning. Maybe the current patch will work when Mac will have a newer gcc. 476873 14 kbalazs 2011-10-03 03:57:24 -0700 Let it rot for a while. 106405 2011-09-06 04:30:06 -0700 2011-10-03 03:56:59 -0700 proposed fix deque_iterators.diff text/plain 4853 kbalazs ZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cgYi9Tb3VyY2UvSmF2 YVNjcmlwdENvcmUvQ2hhbmdlTG9nCmluZGV4IDM0MzEwYzQuLjEyZDllMTMgMTAwNjQ0Ci0tLSBh L1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL0NoYW5nZUxvZwpAQCAtMSwzICsxLDI1IEBACisyMDExLTA5LTA2ICBCYWxhenMgS2VsZW1l biAgPGtiYWxhenNAd2Via2l0Lm9yZz4KKworICAgICAgICBEZXF1ZUl0ZXJhdG9yIGxhY2tzIGRl ZmF1bHQgY29uc3RydWN0b3IKKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19i dWcuY2dpP2lkPTY1NDE0CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisK KyAgICAgICAgQWRkIGRlZmF1bHQgY29uc3RydWN0b3IgZm9yIGl0ZXJhdG9yIGNsYXNzZXMgb2Yg RGVxdWUgYW5kCisgICAgICAgIHVwZGF0ZSBjYWxsIHNpdGVzIHRvIG5vdCBkbyB1bm5lY2Vzc2Fy eSBpbml0YWxpemF0aW9uLgorCisgICAgICAgICogd3RmL0RlcXVlLmg6CisgICAgICAgIChXVEY6 OkRlcXVlSXRlcmF0b3I6OkRlcXVlSXRlcmF0b3IpOgorICAgICAgICAoV1RGOjpEZXF1ZUNvbnN0 SXRlcmF0b3I6OkRlcXVlQ29uc3RJdGVyYXRvcik6CisgICAgICAgIChXVEY6OkRlcXVlUmV2ZXJz ZUl0ZXJhdG9yOjpEZXF1ZVJldmVyc2VJdGVyYXRvcik6CisgICAgICAgIChXVEY6OkRlcXVlQ29u c3RSZXZlcnNlSXRlcmF0b3I6OkRlcXVlQ29uc3RSZXZlcnNlSXRlcmF0b3IpOgorICAgICAgICAo V1RGOjo6OkRlcXVlSXRlcmF0b3JCYXNlKToKKyAgICAgICAgKiB3dGYvTWFpblRocmVhZC5jcHA6 CisgICAgICAgIChXVEY6OmNhbmNlbENhbGxPbk1haW5UaHJlYWQpOgorICAgICAgICAqIHd0Zi9N ZXNzYWdlUXVldWUuaDoKKyAgICAgICAgKFdURjo6Ojp3YWl0Rm9yTWVzc2FnZUZpbHRlcmVkV2l0 aFRpbWVvdXQpOgorICAgICAgICAoV1RGOjo6OnJlbW92ZUlmKToKKwogMjAxMS0wOS0wNSAgU2hl cmlmZiBCb3QgIDx3ZWJraXQucmV2aWV3LmJvdEBnbWFpbC5jb20+CiAKICAgICAgICAgVW5yZXZp ZXdlZCwgcm9sbGluZyBvdXQgcjk0NDQ1IGFuZCByOTQ0NDguCmRpZmYgLS1naXQgYS9Tb3VyY2Uv SmF2YVNjcmlwdENvcmUvd3RmL0RlcXVlLmggYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL0Rl cXVlLmgKaW5kZXggMWVkNDdhOC4uMzdiOGE5OCAxMDA2NDQKLS0tIGEvU291cmNlL0phdmFTY3Jp cHRDb3JlL3d0Zi9EZXF1ZS5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYvRGVxdWUu aApAQCAtMTYxLDYgKzE2MSw3IEBAIG5hbWVzcGFjZSBXVEYgewogICAgICAgICB0eXBlZGVmIERl cXVlSXRlcmF0b3I8VCwgaW5saW5lQ2FwYWNpdHk+IEl0ZXJhdG9yOwogCiAgICAgcHVibGljOgor ICAgICAgICBEZXF1ZUl0ZXJhdG9yKCkgOiBCYXNlKCkgeyB9CiAgICAgICAgIERlcXVlSXRlcmF0 b3IoRGVxdWU8VCwgaW5saW5lQ2FwYWNpdHk+KiBkZXF1ZSwgc2l6ZV90IGluZGV4KSA6IEJhc2Uo ZGVxdWUsIGluZGV4KSB7IH0KIAogICAgICAgICBEZXF1ZUl0ZXJhdG9yKGNvbnN0IEl0ZXJhdG9y JiBvdGhlcikgOiBCYXNlKG90aGVyKSB7IH0KQEAgLTE4Niw2ICsxODcsNyBAQCBuYW1lc3BhY2Ug V1RGIHsKICAgICAgICAgdHlwZWRlZiBEZXF1ZUl0ZXJhdG9yPFQsIGlubGluZUNhcGFjaXR5PiBO b25Db25zdEl0ZXJhdG9yOwogCiAgICAgcHVibGljOgorICAgICAgICBEZXF1ZUNvbnN0SXRlcmF0 b3IoKSA6IEJhc2UoKSB7IH0KICAgICAgICAgRGVxdWVDb25zdEl0ZXJhdG9yKGNvbnN0IERlcXVl PFQsIGlubGluZUNhcGFjaXR5PiogZGVxdWUsIHNpemVfdCBpbmRleCkgOiBCYXNlKGRlcXVlLCBp bmRleCkgeyB9CiAKICAgICAgICAgRGVxdWVDb25zdEl0ZXJhdG9yKGNvbnN0IEl0ZXJhdG9yJiBv dGhlcikgOiBCYXNlKG90aGVyKSB7IH0KQEAgLTIxMiw2ICsyMTQsNyBAQCBuYW1lc3BhY2UgV1RG IHsKICAgICAgICAgdHlwZWRlZiBEZXF1ZVJldmVyc2VJdGVyYXRvcjxULCBpbmxpbmVDYXBhY2l0 eT4gSXRlcmF0b3I7CiAKICAgICBwdWJsaWM6CisgICAgICAgIERlcXVlUmV2ZXJzZUl0ZXJhdG9y KCkgOiBCYXNlKCkgeyB9CiAgICAgICAgIERlcXVlUmV2ZXJzZUl0ZXJhdG9yKGNvbnN0IERlcXVl PFQsIGlubGluZUNhcGFjaXR5PiogZGVxdWUsIHNpemVfdCBpbmRleCkgOiBCYXNlKGRlcXVlLCBp bmRleCkgeyB9CiAKICAgICAgICAgRGVxdWVSZXZlcnNlSXRlcmF0b3IoY29uc3QgSXRlcmF0b3Im IG90aGVyKSA6IEJhc2Uob3RoZXIpIHsgfQpAQCAtMjM3LDYgKzI0MCw3IEBAIG5hbWVzcGFjZSBX VEYgewogICAgICAgICB0eXBlZGVmIERlcXVlUmV2ZXJzZUl0ZXJhdG9yPFQsIGlubGluZUNhcGFj aXR5PiBOb25Db25zdEl0ZXJhdG9yOwogCiAgICAgcHVibGljOgorICAgICAgICBEZXF1ZUNvbnN0 UmV2ZXJzZUl0ZXJhdG9yKCkgOiBCYXNlKCkgeyB9CiAgICAgICAgIERlcXVlQ29uc3RSZXZlcnNl SXRlcmF0b3IoY29uc3QgRGVxdWU8VCwgaW5saW5lQ2FwYWNpdHk+KiBkZXF1ZSwgc2l6ZV90IGlu ZGV4KSA6IEJhc2UoZGVxdWUsIGluZGV4KSB7IH0KIAogICAgICAgICBEZXF1ZUNvbnN0UmV2ZXJz ZUl0ZXJhdG9yKGNvbnN0IEl0ZXJhdG9yJiBvdGhlcikgOiBCYXNlKG90aGVyKSB7IH0KQEAgLTU4 OSw2ICs1OTMsMTAgQEAgbmFtZXNwYWNlIFdURiB7CiAgICAgdGVtcGxhdGU8dHlwZW5hbWUgVCwg c2l6ZV90IGlubGluZUNhcGFjaXR5PgogICAgIGlubGluZSBEZXF1ZUl0ZXJhdG9yQmFzZTxULCBp bmxpbmVDYXBhY2l0eT46OkRlcXVlSXRlcmF0b3JCYXNlKCkKICAgICAgICAgOiBtX2RlcXVlKDAp CisjaWZuZGVmIE5ERUJVRworICAgICAgICAsIG1fbmV4dCgwKQorICAgICAgICAsIG1fcHJldmlv dXMoMCkKKyNlbmRpZgogICAgIHsKICAgICB9CiAKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2Ny aXB0Q29yZS93dGYvTWFpblRocmVhZC5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvd3RmL01h aW5UaHJlYWQuY3BwCmluZGV4IDA5ZmVhN2IuLmI5ZDE3N2UgMTAwNjQ0Ci0tLSBhL1NvdXJjZS9K YXZhU2NyaXB0Q29yZS93dGYvTWFpblRocmVhZC5jcHAKKysrIGIvU291cmNlL0phdmFTY3JpcHRD b3JlL3d0Zi9NYWluVGhyZWFkLmNwcApAQCAtMjA4LDE0ICsyMDgsOSBAQCB2b2lkIGNhbmNlbENh bGxPbk1haW5UaHJlYWQoTWFpblRocmVhZEZ1bmN0aW9uKiBmdW5jdGlvbiwgdm9pZCogY29udGV4 dCkKIAogICAgIEZ1bmN0aW9uV2l0aENvbnRleHRGaW5kZXIgcHJlZChGdW5jdGlvbldpdGhDb250 ZXh0KGZ1bmN0aW9uLCBjb250ZXh0KSk7CiAKLSAgICB3aGlsZSAodHJ1ZSkgewotICAgICAgICAv LyBXZSBtdXN0IHJlZGVmaW5lICdpJyBlYWNoIHBhc3MsIGJlY2F1c2UgdGhlIGl0ZXJlcmF0b3In cyBvcGVyYXRvcj0gCi0gICAgICAgIC8vIHJlcXVpcmVzICd0aGlzJyB0byBiZSB2YWxpZCwgYW5k IHJlbW92ZSgpIGludmFsaWRhdGVzIGFsbCBpdGVyYXRvcnMKLSAgICAgICAgRnVuY3Rpb25RdWV1 ZTo6aXRlcmF0b3IgaShmdW5jdGlvblF1ZXVlKCkuZmluZElmKHByZWQpKTsKLSAgICAgICAgaWYg KGkgPT0gZnVuY3Rpb25RdWV1ZSgpLmVuZCgpKQotICAgICAgICAgICAgYnJlYWs7CisgICAgRnVu Y3Rpb25RdWV1ZTo6aXRlcmF0b3IgaTsKKyAgICB3aGlsZSAoKGkgPSBmdW5jdGlvblF1ZXVlKCku ZmluZElmKHByZWQpKSAhPSBmdW5jdGlvblF1ZXVlKCkuZW5kKCkpCiAgICAgICAgIGZ1bmN0aW9u UXVldWUoKS5yZW1vdmUoaSk7Ci0gICAgfQogfQogCiB2b2lkIHNldE1haW5UaHJlYWRDYWxsYmFj a3NQYXVzZWQoYm9vbCBwYXVzZWQpCmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUv d3RmL01lc3NhZ2VRdWV1ZS5oIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL3d0Zi9NZXNzYWdlUXVl dWUuaAppbmRleCBmZTA0NzIyLi5iMmU1OWUyIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvd3RmL01lc3NhZ2VRdWV1ZS5oCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS93dGYv TWVzc2FnZVF1ZXVlLmgKQEAgLTEzMiw3ICsxMzIsNyBAQCBuYW1lc3BhY2UgV1RGIHsKICAgICAg ICAgTXV0ZXhMb2NrZXIgbG9jayhtX211dGV4KTsKICAgICAgICAgYm9vbCB0aW1lZE91dCA9IGZh bHNlOwogCi0gICAgICAgIERlcXVlQ29uc3RJdGVyYXRvcjxEYXRhVHlwZSo+IGZvdW5kID0gbV9x dWV1ZS5lbmQoKTsKKyAgICAgICAgRGVxdWVDb25zdEl0ZXJhdG9yPERhdGFUeXBlKj4gZm91bmQ7 CiAgICAgICAgIHdoaWxlICghbV9raWxsZWQgJiYgIXRpbWVkT3V0ICYmIChmb3VuZCA9IG1fcXVl dWUuZmluZElmKHByZWRpY2F0ZSkpID09IG1fcXVldWUuZW5kKCkpCiAgICAgICAgICAgICB0aW1l ZE91dCA9ICFtX2NvbmRpdGlvbi50aW1lZFdhaXQobV9tdXRleCwgYWJzb2x1dGVUaW1lKTsKIApA QCAtMTcyLDcgKzE3Miw3IEBAIG5hbWVzcGFjZSBXVEYgewogICAgIGlubGluZSB2b2lkIE1lc3Nh Z2VRdWV1ZTxEYXRhVHlwZT46OnJlbW92ZUlmKFByZWRpY2F0ZSYgcHJlZGljYXRlKQogICAgIHsK ICAgICAgICAgTXV0ZXhMb2NrZXIgbG9jayhtX211dGV4KTsKLSAgICAgICAgRGVxdWVDb25zdEl0 ZXJhdG9yPERhdGFUeXBlKj4gZm91bmQgPSBtX3F1ZXVlLmVuZCgpOworICAgICAgICBEZXF1ZUNv bnN0SXRlcmF0b3I8RGF0YVR5cGUqPiBmb3VuZDsKICAgICAgICAgd2hpbGUgKChmb3VuZCA9IG1f cXVldWUuZmluZElmKHByZWRpY2F0ZSkpICE9IG1fcXVldWUuZW5kKCkpIHsKICAgICAgICAgICAg IERhdGFUeXBlKiBtZXNzYWdlID0gKmZvdW5kOwogICAgICAgICAgICAgbV9xdWV1ZS5yZW1vdmUo Zm91bmQpOwo=