124552 2013-11-18 17:19:56 -0800 REGRESSION (r159395): Error compiling for ARMv7 2013-11-19 13:57:24 -0800 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) All All RESOLVED FIXED P2 Normal --- 108645 1 msaboff msaboff ggaren ossy oldest_to_newest 951899 0 msaboff 2013-11-18 17:19:56 -0800 In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/llint/LLIntOffsetsExtractor.cpp:29: In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/bytecode/CodeBlock.h:37: In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/bytecode/CallLinkInfo.h:29: In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/jit/ClosureCallStubRoutine.h:33: In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/bytecode/CodeOrigin.h:32: In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/bytecode/ValueRecovery.h:30: After http://trac.webkit.org/changeset/159395, compiling for ARMv7 gives this error. In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/jit/GPRInfo.h:29: In file included from /Volumes/Data/src/webkit.work/Source/JavaScriptCore/assembler/MacroAssembler.h:34: /Volumes/Data/src/webkit.work/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1382:9: error: no matching member function for call to 'load8' load8(left, addressTempRegister); ^~~~~ /Volumes/Data/src/webkit.work/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:536:10: note: candidate function not viable: no known conversion from 'JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::AbsoluteAddress' to 'JSC::MacroAssemblerARMv7::ArmAddress' for 1st argument void load8(ArmAddress address, RegisterID dest) ^ /Volumes/Data/src/webkit.work/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:635:10: note: candidate function not viable: no known conversion from 'JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::AbsoluteAddress' to 'JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::ImplicitAddress' for 1st argument void load8(ImplicitAddress address, RegisterID dest) ^ /Volumes/Data/src/webkit.work/Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:645:10: note: candidate function not viable: no known conversion from 'JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::AbsoluteAddress' to 'JSC::AbstractMacroAssembler<JSC::ARMv7Assembler>::BaseIndex' for 1st argument void load8(BaseIndex address, RegisterID dest) ^ 1 error generated. ** BUILD FAILED ** 951906 1 217257 msaboff 2013-11-18 17:31:23 -0800 Created attachment 217257 Patch 951907 2 217257 ggaren 2013-11-18 17:37:01 -0800 Comment on attachment 217257 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217257&action=review > Source/JavaScriptCore/ChangeLog:9 > + to follow materialize and use address similar to other ARMv7 branchXX() functions. I don't think you meant to say "follow" here. > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1382 > + // use addressTempRegister incase the branchTest8 we call uses dataTempRegister. :-/ "Use" should be capitalized. "in case" is two words. ":-/" seems to imply that something is still wrong with this code. Is there something wrong with this code? This function does not call branchTest8. Did you mean branch32? When would branch32 use dataTempRegister, and why would that be bad? 951909 3 msaboff 2013-11-18 17:49:59 -0800 (In reply to comment #2) > (From update of attachment 217257 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217257&action=review > > > Source/JavaScriptCore/ChangeLog:9 > > + to follow materialize and use address similar to other ARMv7 branchXX() functions. > > I don't think you meant to say "follow" here. Eliminated "follow" > > Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1382 > > + // use addressTempRegister incase the branchTest8 we call uses dataTempRegister. :-/ > > "Use" should be capitalized. "in case" is two words. ":-/" seems to imply that something is still wrong with this code. Is there something wrong with this code? This function does not call branchTest8. Did you mean branch32? When would branch32 use dataTempRegister, and why would that be bad? The "use", "incase" and ":-/" are present in all the other similar comments. I copied from one of them. Copy/paste error. Changed it to: // Use addressTempRegister in case the branch32 we call uses dataTempRegister. branch32() calls compare32() which uses dataTempRegister when the TrustedImm32 needs to be materialized in a register because we can't encode it in a compare immediate instruction. The comment is there because we would typically load into dataTempRegister here, but can't because of the possible use in compare32(). 951915 4 217257 ggaren 2013-11-18 18:18:14 -0800 Comment on attachment 217257 Patch View in context: https://bugs.webkit.org/attachment.cgi?id=217257&action=review >>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1382 >>> + // use addressTempRegister incase the branchTest8 we call uses dataTempRegister. :-/ >> >> "Use" should be capitalized. "in case" is two words. ":-/" seems to imply that something is still wrong with this code. Is there something wrong with this code? This function does not call branchTest8. Did you mean branch32? When would branch32 use dataTempRegister, and why would that be bad? > > The "use", "incase" and ":-/" are present in all the other similar comments. I copied from one of them. > > Copy/paste error. Changed it to: > // Use addressTempRegister in case the branch32 we call uses dataTempRegister. > > branch32() calls compare32() which uses dataTempRegister when the TrustedImm32 needs to be materialized in a register because we can't encode it in a compare immediate instruction. The comment is there because we would typically load into dataTempRegister here, but can't because of the possible use in compare32(). Sounds like this comment should also say something about "addressTempRegister instead of dataTempRegister", then. 951918 5 msaboff 2013-11-18 18:26:52 -0800 (In reply to comment #4) > (From update of attachment 217257 [details]) > View in context: https://bugs.webkit.org/attachment.cgi?id=217257&action=review > > >>> Source/JavaScriptCore/assembler/MacroAssemblerARMv7.h:1382 > >>> + // use addressTempRegister incase the branchTest8 we call uses dataTempRegister. :-/ > >> > >> "Use" should be capitalized. "in case" is two words. ":-/" seems to imply that something is still wrong with this code. Is there something wrong with this code? This function does not call branchTest8. Did you mean branch32? When would branch32 use dataTempRegister, and why would that be bad? > > > > The "use", "incase" and ":-/" are present in all the other similar comments. I copied from one of them. > > > > Copy/paste error. Changed it to: > > // Use addressTempRegister in case the branch32 we call uses dataTempRegister. > > > > branch32() calls compare32() which uses dataTempRegister when the TrustedImm32 needs to be materialized in a register because we can't encode it in a compare immediate instruction. The comment is there because we would typically load into dataTempRegister here, but can't because of the possible use in compare32(). > > Sounds like this comment should also say something about "addressTempRegister instead of dataTempRegister", then. Does the following work? // Use addressTempRegister instead of dataTempRegister in case the branch32 or its callees use dataTempRegister 951921 6 ggaren 2013-11-18 18:47:21 -0800 > Does the following work? > // Use addressTempRegister instead of dataTempRegister in case the branch32 or its callees use dataTempRegister I think you can be even clearer and say, "Use addressTempRegister instead of dataTempRegister, since branch32 uses dataTempRegister.". 951929 7 msaboff 2013-11-18 19:41:12 -0800 (In reply to comment #6) > > Does the following work? > > // Use addressTempRegister instead of dataTempRegister in case the branch32 or its callees use dataTempRegister > > I think you can be even clearer and say, "Use addressTempRegister instead of dataTempRegister, since branch32 uses dataTempRegister.". Sold. I updated the comment to what you suggest. 952143 8 217257 ggaren 2013-11-19 11:16:15 -0800 Comment on attachment 217257 Patch Can you post the updated patch? 952145 9 217312 msaboff 2013-11-19 11:18:26 -0800 Created attachment 217312 Updated patch 952221 10 217312 ggaren 2013-11-19 13:44:16 -0800 Comment on attachment 217312 Updated patch r=me 952225 11 msaboff 2013-11-19 13:57:24 -0800 Committed r159521: <http://trac.webkit.org/changeset/159521> 217257 2013-11-18 17:31:23 -0800 2013-11-19 11:18:26 -0800 Patch 124552.patch text/plain 1762 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTU5NDc1KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBA CisyMDEzLTExLTE4ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIFJFR1JFU1NJT04gKHIxNTkzOTUpOiBFcnJvciBjb21waWxpbmcgZm9yIEFSTXY3CisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjQ1NTIKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXhlZCB0aGUgaW1w bGVtZW50YXRpb24gb2YgYnJhbmNoOChSZWxhdGlvbmFsQ29uZGl0aW9uIGNvbmQsIEFic29sdXRl QWRkcmVzcyBhZGRyZXNzLCBUcnVzdGVkSW1tMzIgcmlnaHQpCisgICAgICAgIHRvIGZvbGxvdyBt YXRlcmlhbGl6ZSBhbmQgdXNlIGFkZHJlc3Mgc2ltaWxhciB0byBvdGhlciBBUk12NyBicmFuY2hY WCgpIGZ1bmN0aW9ucy4KKworICAgICAgICAqIGFzc2VtYmxlci9NYWNyb0Fzc2VtYmxlckFSTXY3 Lmg6CisgICAgICAgIChKU0M6Ok1hY3JvQXNzZW1ibGVyQVJNdjc6OmJyYW5jaDgpOgorCiAyMDEz LTExLTE4ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgogCiAgICAgICAgIEFS TTY0IENSQVNIOiBEZWJ1ZyBidWlsZHMgY3Jhc2ggaW4gZW1pdFBvaW50ZXJWYWxpZGF0aW9uKCkK SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk12 Ny5oCj09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT0KLS0tIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3NlbWJsZXIvTWFjcm9B c3NlbWJsZXJBUk12Ny5oCShyZXZpc2lvbiAxNTk0NjcpCisrKyBTb3VyY2UvSmF2YVNjcmlwdENv cmUvYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNdjcuaAkod29ya2luZyBjb3B5KQpAQCAtMTM3 Nyw5ICsxMzc3LDExIEBAIHB1YmxpYzoKICAgICAgICAgcmV0dXJuIGJyYW5jaDMyKGNvbmQsIGFk ZHJlc3NUZW1wUmVnaXN0ZXIsIHJpZ2h0KTsKICAgICB9CiAgICAgCi0gICAgSnVtcCBicmFuY2g4 KFJlbGF0aW9uYWxDb25kaXRpb24gY29uZCwgQWJzb2x1dGVBZGRyZXNzIGxlZnQsIFRydXN0ZWRJ bW0zMiByaWdodCkKKyAgICBKdW1wIGJyYW5jaDgoUmVsYXRpb25hbENvbmRpdGlvbiBjb25kLCBB YnNvbHV0ZUFkZHJlc3MgYWRkcmVzcywgVHJ1c3RlZEltbTMyIHJpZ2h0KQogICAgIHsKLSAgICAg ICAgbG9hZDgobGVmdCwgYWRkcmVzc1RlbXBSZWdpc3Rlcik7CisgICAgICAgIC8vIHVzZSBhZGRy ZXNzVGVtcFJlZ2lzdGVyIGluY2FzZSB0aGUgYnJhbmNoVGVzdDggd2UgY2FsbCB1c2VzIGRhdGFU ZW1wUmVnaXN0ZXIuIDotLworICAgICAgICBtb3ZlKFRydXN0ZWRJbW1QdHIoYWRkcmVzcy5tX3B0 ciksIGFkZHJlc3NUZW1wUmVnaXN0ZXIpOworICAgICAgICBsb2FkOChBZGRyZXNzKGFkZHJlc3NU ZW1wUmVnaXN0ZXIpLCBhZGRyZXNzVGVtcFJlZ2lzdGVyKTsKICAgICAgICAgcmV0dXJuIGJyYW5j aDMyKGNvbmQsIGFkZHJlc3NUZW1wUmVnaXN0ZXIsIHJpZ2h0KTsKICAgICB9CiAgICAgCg== 217312 2013-11-19 11:18:26 -0800 2013-11-19 13:44:16 -0800 Updated patch 124552-2.patch text/plain 1764 msaboff SW5kZXg6IFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9DaGFuZ2VMb2cKPT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PQotLS0gU291 cmNlL0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkocmV2aXNpb24gMTU5NDc1KQorKysgU291cmNl L0phdmFTY3JpcHRDb3JlL0NoYW5nZUxvZwkod29ya2luZyBjb3B5KQpAQCAtMSwzICsxLDE2IEBA CisyMDEzLTExLTE4ICBNaWNoYWVsIFNhYm9mZiAgPG1zYWJvZmZAYXBwbGUuY29tPgorCisgICAg ICAgIFJFR1JFU1NJT04gKHIxNTkzOTUpOiBFcnJvciBjb21waWxpbmcgZm9yIEFSTXY3CisgICAg ICAgIGh0dHBzOi8vYnVncy53ZWJraXQub3JnL3Nob3dfYnVnLmNnaT9pZD0xMjQ1NTIKKworICAg ICAgICBSZXZpZXdlZCBieSBOT0JPRFkgKE9PUFMhKS4KKworICAgICAgICBGaXhlZCB0aGUgaW1w bGVtZW50YXRpb24gb2YgYnJhbmNoOChSZWxhdGlvbmFsQ29uZGl0aW9uIGNvbmQsIEFic29sdXRl QWRkcmVzcyBhZGRyZXNzLCBUcnVzdGVkSW1tMzIgcmlnaHQpCisgICAgICAgIHRvIG1hdGVyaWFs aXplIGFuZCB1c2UgYWRkcmVzcyBzaW1pbGFyIHRvIG90aGVyIEFSTXY3IGJyYW5jaFhYKCkgZnVu Y3Rpb25zLgorCisgICAgICAgICogYXNzZW1ibGVyL01hY3JvQXNzZW1ibGVyQVJNdjcuaDoKKyAg ICAgICAgKEpTQzo6TWFjcm9Bc3NlbWJsZXJBUk12Nzo6YnJhbmNoOCk6CisKIDIwMTMtMTEtMTgg IE1pY2hhZWwgU2Fib2ZmICA8bXNhYm9mZkBhcHBsZS5jb20+CiAKICAgICAgICAgQVJNNjQgQ1JB U0g6IERlYnVnIGJ1aWxkcyBjcmFzaCBpbiBlbWl0UG9pbnRlclZhbGlkYXRpb24oKQpJbmRleDog U291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9NYWNyb0Fzc2VtYmxlckFSTXY3LmgKPT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PQotLS0gU291cmNlL0phdmFTY3JpcHRDb3JlL2Fzc2VtYmxlci9NYWNyb0Fzc2VtYmxl ckFSTXY3LmgJKHJldmlzaW9uIDE1OTQ2NykKKysrIFNvdXJjZS9KYXZhU2NyaXB0Q29yZS9hc3Nl bWJsZXIvTWFjcm9Bc3NlbWJsZXJBUk12Ny5oCSh3b3JraW5nIGNvcHkpCkBAIC0xMzc3LDkgKzEz NzcsMTEgQEAgcHVibGljOgogICAgICAgICByZXR1cm4gYnJhbmNoMzIoY29uZCwgYWRkcmVzc1Rl bXBSZWdpc3RlciwgcmlnaHQpOwogICAgIH0KICAgICAKLSAgICBKdW1wIGJyYW5jaDgoUmVsYXRp b25hbENvbmRpdGlvbiBjb25kLCBBYnNvbHV0ZUFkZHJlc3MgbGVmdCwgVHJ1c3RlZEltbTMyIHJp Z2h0KQorICAgIEp1bXAgYnJhbmNoOChSZWxhdGlvbmFsQ29uZGl0aW9uIGNvbmQsIEFic29sdXRl QWRkcmVzcyBhZGRyZXNzLCBUcnVzdGVkSW1tMzIgcmlnaHQpCiAgICAgewotICAgICAgICBsb2Fk OChsZWZ0LCBhZGRyZXNzVGVtcFJlZ2lzdGVyKTsKKyAgICAgICAgLy8gVXNlIGFkZHJlc3NUZW1w UmVnaXN0ZXIgaW5zdGVhZCBvZiBkYXRhVGVtcFJlZ2lzdGVyLCBzaW5jZSBicmFuY2gzMiB1c2Vz IGRhdGFUZW1wUmVnaXN0ZXIuCisgICAgICAgIG1vdmUoVHJ1c3RlZEltbVB0cihhZGRyZXNzLm1f cHRyKSwgYWRkcmVzc1RlbXBSZWdpc3Rlcik7CisgICAgICAgIGxvYWQ4KEFkZHJlc3MoYWRkcmVz c1RlbXBSZWdpc3RlciksIGFkZHJlc3NUZW1wUmVnaXN0ZXIpOwogICAgICAgICByZXR1cm4gYnJh bmNoMzIoY29uZCwgYWRkcmVzc1RlbXBSZWdpc3RlciwgcmlnaHQpOwogICAgIH0KICAgICAK