95418 2012-08-29 20:05:38 -0700 Build warning : -Wsign-compare on DFGByteCodeParser.cpp. 2012-08-30 11:44:42 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Unspecified Unspecified RESOLVED FIXED P2 Normal --- 0 bw80.lee webkit-unassigned fpizlo haraken webkit.review.bot oldest_to_newest 708240 0 bw80.lee 2012-08-29 20:05:38 -0700 There is a build warning '-Wsign-compare' on findArgumentPositionForLocal() function in DFGByteCodeParser.cpp 708250 1 161393 bw80.lee 2012-08-29 20:28:22 -0700 Created attachment 161393 Patch 708253 2 161393 haraken 2012-08-29 20:37:09 -0700 Comment on attachment 161393 Patch OK 708257 3 161393 buildbot 2012-08-29 20:45:16 -0700 Comment on attachment 161393 Patch Attachment 161393 did not pass mac-ews (mac): Output: http://queues.webkit.org/results/13681523 708321 4 161393 fpizlo 2012-08-29 22:47:53 -0700 Comment on attachment 161393 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=161393&action=review I think what you're hitting here is that different compilers have different opinions about what the sign of arithmetic on a part signed, part unsigned expression is. On our build system, the current setup works - these comparisons are all signed versus signed. If your build system disagrees (which it apparently is doing), you should insert casts into the arithmetic (inlineCallFrame->stackOffset + CallFrame::thisArgumentOffset(), and inlineCallFrame->stackOffset - RegisterFile::CallFrameHeaderSize - inlineCallFrame->arguments.size()) to ensure that it stays signed all the way through. And please don't change operand variables to unsigned. Operands are always signed, since negative operand values carry special meaning. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:362 > - ArgumentPosition* findArgumentPositionForLocal(int operand) > + ArgumentPosition* findArgumentPositionForLocal(unsigned operand) This is wrong. Operands are always signed integers. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:370 > - if (operand == inlineCallFrame->stackOffset + CallFrame::thisArgumentOffset()) > + if (static_cast<int>(operand) == inlineCallFrame->stackOffset + CallFrame::thisArgumentOffset()) Change the right hand side of this to static_cast<int>(inlineCallFrame->...) > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:372 > - if (static_cast<unsigned>(operand) < inlineCallFrame->stackOffset - RegisterFile::CallFrameHeaderSize - inlineCallFrame->arguments.size()) > + if (operand < inlineCallFrame->stackOffset - RegisterFile::CallFrameHeaderSize - inlineCallFrame->arguments.size()) Same as above. Change the right hand side to a cast to int. > Source/JavaScriptCore/dfg/DFGByteCodeParser.cpp:374 > - int argument = operandToArgument(operand - inlineCallFrame->stackOffset); > + int argument = operandToArgument(static_cast<int>(operand) - inlineCallFrame->stackOffset); Remove this. 708341 5 161393 bw80.lee 2012-08-29 23:20:52 -0700 Comment on attachment 161393 Patch Thanks for your comments to arrange this. I just changed the type of parameter because that, current implementation looks not having the case that a negative value is passed through that parameter. But now I understand your point about the special meaning of the type of the 'operand'. I'll remain the parameter type and make new patch according to your guide. Thanks :) 708343 6 fpizlo 2012-08-29 23:22:59 -0700 (In reply to comment #5) > (From update of attachment 161393 [details]) > Thanks for your comments to arrange this. > I just changed the type of parameter because that, current implementation looks not having the case that a negative value is passed through that parameter. You are absolutely correct. I just think it's better to maintain consistency here. > But now I understand your point about the special meaning of the type of the 'operand'. > I'll remain the parameter type and make new patch according to your guide. > Thanks :) Cool! Thanks for fixing these issues. 708348 7 161408 bw80.lee 2012-08-29 23:35:41 -0700 Created attachment 161408 Patch 708350 8 161408 fpizlo 2012-08-29 23:39:00 -0700 Comment on attachment 161408 Patch This looks good. Hopefully it will pass all the bots! 708727 9 bw80.lee 2012-08-30 09:43:11 -0700 All the bots were passed. Thanks for your reviews. :) 708873 10 161408 webkit.review.bot 2012-08-30 11:44:39 -0700 Comment on attachment 161408 Patch Clearing flags on attachment: 161408 Committed r127167: <http://trac.webkit.org/changeset/127167> 708874 11 webkit.review.bot 2012-08-30 11:44:42 -0700 All reviewed patches have been landed. Closing bug. 161393 2012-08-29 20:28:22 -0700 2012-08-29 23:35:29 -0700 Patch bug-95418-20120830122807.patch text/plain 3158 bw80.lee U3VidmVyc2lvbiBSZXZpc2lvbjogMTI2OTQ3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBh YmY3ZTRhMWU4NjkwZjhiNzRlZTMyOGEzNGZkMzY4MzE2MTVmNzkyLi4yMTlhOTcwN2M2MDk4YmE1 ZmNjYjEyOTM0NGIwNjhjY2MwNjM5YWI0IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwyMSBAQAorMjAxMi0wOC0yOSAgQnl1bmd3b28gTGVlICA8Ync4MC5sZWVAc2Ftc3VuZy5j b20+CisKKyAgICAgICAgQnVpbGQgd2FybmluZyA6IC1Xc2lnbi1jb21wYXJlIG9uIERGR0J5dGVD b2RlUGFyc2VyLmNwcC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTk1NDE4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgVGhlcmUgaXMgYSBidWlsZCB3YXJuaW5nICctV3NpZ24tY29tcGFyZScgb24KKyAgICAg ICAgZmluZEFyZ3VtZW50UG9zaXRpb25Gb3JMb2NhbCgpIGluIERGR0J5dGVDb2RlUGFyc2VyLmNw cC4KKworICAgICAgICBmaW5kQXJndW1lbnRQb3NpdGlvbkZvckxvY2FsKCkgZnVuY3Rpb24gYWx3 YXlzIGhhcyB1bnNpZ25lZCB2YWx1ZSBhcworICAgICAgICB0aGUgJ29wZXJhbmQnIHBhcmFtZXRl ci4gU28sIGZvciByZW1vdmluZyB0aGUgd2FybmluZywgY2hhbmdlZCB0aGUKKyAgICAgICAgcGFy YW1ldGVyIHR5cGUgb2YgdGhlIGZ1bmN0aW9uIHRvICd1bnNpZ25lZCcuCisKKyAgICAgICAgKiBk ZmcvREZHQnl0ZUNvZGVQYXJzZXIuY3BwOgorICAgICAgICAoSlNDOjpERkc6OkJ5dGVDb2RlUGFy c2VyOjpmaW5kQXJndW1lbnRQb3NpdGlvbkZvckxvY2FsKToKKyAgICAgICAgKEpTQzo6REZHOjpC eXRlQ29kZVBhcnNlcjo6ZmluZEFyZ3VtZW50UG9zaXRpb24pOgorCiAyMDEyLTA4LTI4ICBHYXZp biBCYXJyYWNsb3VnaCAgPGJhcnJhY2xvdWdoQGFwcGxlLmNvbT4KIAogICAgICAgICBSb2xsZWQg b3V0IHIxMjY5MjgsIHRoaXMgYnJva2Ugc3R1ZmYgOictKApkaWZmIC0tZ2l0IGEvU291cmNlL0ph dmFTY3JpcHRDb3JlL2RmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHAgYi9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvZGZnL0RGR0J5dGVDb2RlUGFyc2VyLmNwcAppbmRleCBkZjE5YzMwODJkN2M1YmEyYzY1 OGQ1OTIxMjQwZWYwNmRjMzg4M2E1Li5iOWYyMzMxMWU1ODkwOTcyN2M4MWFhMzE4NDc1ZmRhMjYy ZGQzZWRiIDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5dGVDb2Rl UGFyc2VyLmNwcAorKysgYi9Tb3VyY2UvSmF2YVNjcmlwdENvcmUvZGZnL0RGR0J5dGVDb2RlUGFy c2VyLmNwcApAQCAtMzU5LDcgKzM1OSw3IEBAIHByaXZhdGU6CiAgICAgICAgIHJldHVybiBzdGFj ay0+bV9hcmd1bWVudFBvc2l0aW9uc1thcmd1bWVudF07CiAgICAgfQogICAgIAotICAgIEFyZ3Vt ZW50UG9zaXRpb24qIGZpbmRBcmd1bWVudFBvc2l0aW9uRm9yTG9jYWwoaW50IG9wZXJhbmQpCisg ICAgQXJndW1lbnRQb3NpdGlvbiogZmluZEFyZ3VtZW50UG9zaXRpb25Gb3JMb2NhbCh1bnNpZ25l ZCBvcGVyYW5kKQogICAgIHsKICAgICAgICAgZm9yIChJbmxpbmVTdGFja0VudHJ5KiBzdGFjayA9 IG1faW5saW5lU3RhY2tUb3A7IDsgc3RhY2sgPSBzdGFjay0+bV9jYWxsZXIpIHsKICAgICAgICAg ICAgIElubGluZUNhbGxGcmFtZSogaW5saW5lQ2FsbEZyYW1lID0gc3RhY2stPm1faW5saW5lQ2Fs bEZyYW1lOwpAQCAtMzY3LDExICszNjcsMTEgQEAgcHJpdmF0ZToKICAgICAgICAgICAgICAgICBi cmVhazsKICAgICAgICAgICAgIGlmIChvcGVyYW5kID49IGlubGluZUNhbGxGcmFtZS0+c3RhY2tP ZmZzZXQgLSBSZWdpc3RlckZpbGU6OkNhbGxGcmFtZUhlYWRlclNpemUpCiAgICAgICAgICAgICAg ICAgY29udGludWU7Ci0gICAgICAgICAgICBpZiAob3BlcmFuZCA9PSBpbmxpbmVDYWxsRnJhbWUt PnN0YWNrT2Zmc2V0ICsgQ2FsbEZyYW1lOjp0aGlzQXJndW1lbnRPZmZzZXQoKSkKKyAgICAgICAg ICAgIGlmIChzdGF0aWNfY2FzdDxpbnQ+KG9wZXJhbmQpID09IGlubGluZUNhbGxGcmFtZS0+c3Rh Y2tPZmZzZXQgKyBDYWxsRnJhbWU6OnRoaXNBcmd1bWVudE9mZnNldCgpKQogICAgICAgICAgICAg ICAgIGNvbnRpbnVlOwotICAgICAgICAgICAgaWYgKHN0YXRpY19jYXN0PHVuc2lnbmVkPihvcGVy YW5kKSA8IGlubGluZUNhbGxGcmFtZS0+c3RhY2tPZmZzZXQgLSBSZWdpc3RlckZpbGU6OkNhbGxG cmFtZUhlYWRlclNpemUgLSBpbmxpbmVDYWxsRnJhbWUtPmFyZ3VtZW50cy5zaXplKCkpCisgICAg ICAgICAgICBpZiAob3BlcmFuZCA8IGlubGluZUNhbGxGcmFtZS0+c3RhY2tPZmZzZXQgLSBSZWdp c3RlckZpbGU6OkNhbGxGcmFtZUhlYWRlclNpemUgLSBpbmxpbmVDYWxsRnJhbWUtPmFyZ3VtZW50 cy5zaXplKCkpCiAgICAgICAgICAgICAgICAgY29udGludWU7Ci0gICAgICAgICAgICBpbnQgYXJn dW1lbnQgPSBvcGVyYW5kVG9Bcmd1bWVudChvcGVyYW5kIC0gaW5saW5lQ2FsbEZyYW1lLT5zdGFj a09mZnNldCk7CisgICAgICAgICAgICBpbnQgYXJndW1lbnQgPSBvcGVyYW5kVG9Bcmd1bWVudChz dGF0aWNfY2FzdDxpbnQ+KG9wZXJhbmQpIC0gaW5saW5lQ2FsbEZyYW1lLT5zdGFja09mZnNldCk7 CiAgICAgICAgICAgICByZXR1cm4gc3RhY2stPm1fYXJndW1lbnRQb3NpdGlvbnNbYXJndW1lbnRd OwogICAgICAgICB9CiAgICAgICAgIHJldHVybiAwOwpAQCAtMzgxLDcgKzM4MSw3IEBAIHByaXZh dGU6CiAgICAgewogICAgICAgICBpZiAob3BlcmFuZElzQXJndW1lbnQob3BlcmFuZCkpCiAgICAg ICAgICAgICByZXR1cm4gZmluZEFyZ3VtZW50UG9zaXRpb25Gb3JBcmd1bWVudChvcGVyYW5kVG9B cmd1bWVudChvcGVyYW5kKSk7Ci0gICAgICAgIHJldHVybiBmaW5kQXJndW1lbnRQb3NpdGlvbkZv ckxvY2FsKG9wZXJhbmQpOworICAgICAgICByZXR1cm4gZmluZEFyZ3VtZW50UG9zaXRpb25Gb3JM b2NhbChzdGF0aWNfY2FzdDx1bnNpZ25lZD4ob3BlcmFuZCkpOwogICAgIH0KICAgICAKICAgICB2 b2lkIGZsdXNoKGludCBvcGVyYW5kKQo= 161408 2012-08-29 23:35:41 -0700 2012-08-30 11:44:39 -0700 Patch bug-95418-20120830153526.patch text/plain 2286 bw80.lee U3VidmVyc2lvbiBSZXZpc2lvbjogMTI2OTQ3CmRpZmYgLS1naXQgYS9Tb3VyY2UvSmF2YVNjcmlw dENvcmUvQ2hhbmdlTG9nIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwppbmRleCBh YmY3ZTRhMWU4NjkwZjhiNzRlZTMyOGEzNGZkMzY4MzE2MTVmNzkyLi5hZDFjYWRkNjE0MTc2ZDcz ZDNjZmE3MWJhYmJhMzY4ZmJmNjNhZWI1IDEwMDY0NAotLS0gYS9Tb3VyY2UvSmF2YVNjcmlwdENv cmUvQ2hhbmdlTG9nCisrKyBiL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKQEAgLTEs MyArMSwxOSBAQAorMjAxMi0wOC0yOSAgQnl1bmd3b28gTGVlICA8Ync4MC5sZWVAc2Ftc3VuZy5j b20+CisKKyAgICAgICAgQnVpbGQgd2FybmluZyA6IC1Xc2lnbi1jb21wYXJlIG9uIERGR0J5dGVD b2RlUGFyc2VyLmNwcC4KKyAgICAgICAgaHR0cHM6Ly9idWdzLndlYmtpdC5vcmcvc2hvd19idWcu Y2dpP2lkPTk1NDE4CisKKyAgICAgICAgUmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAg ICAgICAgVGhlcmUgaXMgYSBidWlsZCB3YXJuaW5nICctV3NpZ24tY29tcGFyZScgb24KKyAgICAg ICAgZmluZEFyZ3VtZW50UG9zaXRpb25Gb3JMb2NhbCgpIGluIERGR0J5dGVDb2RlUGFyc2VyLmNw cC4KKworICAgICAgICBGb3IgcmVtb3ZpbmcgdGhpcyB3YXJuaW5nLCBjYXN0aW5nIHN0YXRlbWVu dCBpcyBhZGRlZCBleHBsaWNpdGx5LgorCisgICAgICAgICogZGZnL0RGR0J5dGVDb2RlUGFyc2Vy LmNwcDoKKyAgICAgICAgKEpTQzo6REZHOjpCeXRlQ29kZVBhcnNlcjo6ZmluZEFyZ3VtZW50UG9z aXRpb25Gb3JMb2NhbCk6CisgICAgICAgIChKU0M6OkRGRzo6Qnl0ZUNvZGVQYXJzZXI6OmZpbmRB cmd1bWVudFBvc2l0aW9uKToKKwogMjAxMi0wOC0yOCAgR2F2aW4gQmFycmFjbG91Z2ggIDxiYXJy YWNsb3VnaEBhcHBsZS5jb20+CiAKICAgICAgICAgUm9sbGVkIG91dCByMTI2OTI4LCB0aGlzIGJy b2tlIHN0dWZmIDonLSgKZGlmZiAtLWdpdCBhL1NvdXJjZS9KYXZhU2NyaXB0Q29yZS9kZmcvREZH Qnl0ZUNvZGVQYXJzZXIuY3BwIGIvU291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdCeXRlQ29k ZVBhcnNlci5jcHAKaW5kZXggZGYxOWMzMDgyZDdjNWJhMmM2NThkNTkyMTI0MGVmMDZkYzM4ODNh NS4uY2JkNzNjMDU3OWUzNzY3M2I0Y2RkZDYzY2I2ZDM0YzA0MWEyMzlkMCAxMDA2NDQKLS0tIGEv U291cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHAKKysrIGIvU291 cmNlL0phdmFTY3JpcHRDb3JlL2RmZy9ERkdCeXRlQ29kZVBhcnNlci5jcHAKQEAgLTM2NSwxMSAr MzY1LDExIEBAIHByaXZhdGU6CiAgICAgICAgICAgICBJbmxpbmVDYWxsRnJhbWUqIGlubGluZUNh bGxGcmFtZSA9IHN0YWNrLT5tX2lubGluZUNhbGxGcmFtZTsKICAgICAgICAgICAgIGlmICghaW5s aW5lQ2FsbEZyYW1lKQogICAgICAgICAgICAgICAgIGJyZWFrOwotICAgICAgICAgICAgaWYgKG9w ZXJhbmQgPj0gaW5saW5lQ2FsbEZyYW1lLT5zdGFja09mZnNldCAtIFJlZ2lzdGVyRmlsZTo6Q2Fs bEZyYW1lSGVhZGVyU2l6ZSkKKyAgICAgICAgICAgIGlmIChvcGVyYW5kID49IHN0YXRpY19jYXN0 PGludD4oaW5saW5lQ2FsbEZyYW1lLT5zdGFja09mZnNldCAtIFJlZ2lzdGVyRmlsZTo6Q2FsbEZy YW1lSGVhZGVyU2l6ZSkpCiAgICAgICAgICAgICAgICAgY29udGludWU7CiAgICAgICAgICAgICBp ZiAob3BlcmFuZCA9PSBpbmxpbmVDYWxsRnJhbWUtPnN0YWNrT2Zmc2V0ICsgQ2FsbEZyYW1lOjp0 aGlzQXJndW1lbnRPZmZzZXQoKSkKICAgICAgICAgICAgICAgICBjb250aW51ZTsKLSAgICAgICAg ICAgIGlmIChzdGF0aWNfY2FzdDx1bnNpZ25lZD4ob3BlcmFuZCkgPCBpbmxpbmVDYWxsRnJhbWUt PnN0YWNrT2Zmc2V0IC0gUmVnaXN0ZXJGaWxlOjpDYWxsRnJhbWVIZWFkZXJTaXplIC0gaW5saW5l Q2FsbEZyYW1lLT5hcmd1bWVudHMuc2l6ZSgpKQorICAgICAgICAgICAgaWYgKG9wZXJhbmQgPCBz dGF0aWNfY2FzdDxpbnQ+KGlubGluZUNhbGxGcmFtZS0+c3RhY2tPZmZzZXQgLSBSZWdpc3RlckZp bGU6OkNhbGxGcmFtZUhlYWRlclNpemUgLSBpbmxpbmVDYWxsRnJhbWUtPmFyZ3VtZW50cy5zaXpl KCkpKQogICAgICAgICAgICAgICAgIGNvbnRpbnVlOwogICAgICAgICAgICAgaW50IGFyZ3VtZW50 ID0gb3BlcmFuZFRvQXJndW1lbnQob3BlcmFuZCAtIGlubGluZUNhbGxGcmFtZS0+c3RhY2tPZmZz ZXQpOwogICAgICAgICAgICAgcmV0dXJuIHN0YWNrLT5tX2FyZ3VtZW50UG9zaXRpb25zW2FyZ3Vt ZW50XTsK