18274 2008-04-01 19:35:52 -0700 ResolveNode::emitCode() doesn't make a new temporary when dst is 0, leading to incorrect codegen 2008-04-02 01:12:10 -0700 1 1 1 Unclassified WebKit JavaScriptCore 528+ (Nightly build) Mac OS X 10.5 RESOLVED FIXED P2 Normal --- 1 zwarich webkit-unassigned ggaren mjs oliver oldest_to_newest 76116 0 zwarich 2008-04-01 19:35:52 -0700 This JavaScript var o = { a: function () { return 1; } }; o.a(); o; generates the following code: [ 0] load lr1, undefined(@k0) [ 3] new_object lr1 [ 5] new_func_exp tr0, f0 [ 8] put_prop_id lr1, a(@id0), tr0 [ 12] get_prop_id tr0, lr1, a(@id0) [ 16] call lr1, tr0, lr1, 8, 1 [ 22] end lr1 The end call ends up overwriting the local variable o with 1, which is clearly incorrect. Obviously, we don't always want ResolveNode::emitCode() to return a new temporary register if dst is unspecified. However, there are two situations where this causes a bug: 1) The returned register is reused. This occurs in FunctionCallDotNode and FunctionCallBracketNode. I have a patch that fixes this particular problem, and I will post it for review. DoWhileNode and ForNode increment the reference count of the returned register, but only to return it, not to reuse it. 2) The returned register is ignored, requiring dst to be the actual target. This occurs in codegen for IfNode, IfElseNode, WhileNode, and ForInNode. It doesn't cause any realistic problems in any of them, because they are statements. The first three return 0 as it is, and ForInNode returns dst, which still isn't a problem if it doesn't contain the correct value, because nothing will use it once we have completions working. Should we just accept this behaviour of emitNode and remember to code around it? 76117 1 20289 zwarich 2008-04-01 19:40:53 -0700 Created attachment 20289 Proposed patch 76118 2 oliver 2008-04-01 19:59:16 -0700 Landed r31555 20289 2008-04-01 19:40:53 -0700 2008-04-01 19:56:14 -0700 Proposed patch tmp.diff text/plain 2318 zwarich SW5kZXg6IENoYW5nZUxvZwo9PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09Ci0tLSBDaGFuZ2VMb2cJKHJldmlzaW9uIDMxNTU0 KQorKysgQ2hhbmdlTG9nCSh3b3JraW5nIGNvcHkpCkBAIC0xLDMgKzEsMTUgQEAKKzIwMDgtMDQt MDEgIENhbWVyb24gWndhcmljaCAgPGN3endhcmljaEB1d2F0ZXJsb28uY2E+CisKKyAgICAgICAg UmV2aWV3ZWQgYnkgTk9CT0RZIChPT1BTISkuCisKKyAgICAgICAgQnVnIDE4Mjc0OiBSZXNvbHZl Tm9kZTo6ZW1pdENvZGUoKSBkb2Vzbid0IG1ha2UgYSBuZXcgdGVtcG9yYXJ5IHdoZW4gZHN0Cisg ICAgICAgICAgICAgICAgICAgaXMgMCwgbGVhZGluZyB0byBpbmNvcnJlY3QgY29kZWdlbgorICAg ICAgICA8aHR0cDovL2J1Z3Mud2Via2l0Lm9yZy9zaG93X2J1Zy5jZ2k/aWQ9MTgyNzQ+CisKKyAg ICAgICAgKiBranMvbm9kZXMuY3BwOgorICAgICAgICAoS0pTOjpGdW5jdGlvbkNhbGxCcmFja2V0 Tm9kZTo6ZW1pdENvZGUpOgorICAgICAgICAoS0pTOjpGdW5jdGlvbkNhbGxEb3ROb2RlOjplbWl0 Q29kZSk6CisKIDIwMDgtMDQtMDEgIE1hY2llaiBTdGFjaG93aWFrICA8bWpzQGFwcGxlLmNvbT4K IAogICAgICAgICBSZXZpZXdlZCBieSBPbGl2ZXIuCkluZGV4OiBranMvbm9kZXMuY3BwCj09PT09 PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09PT09 PT09PT0KLS0tIGtqcy9ub2Rlcy5jcHAJKHJldmlzaW9uIDMxNTUyKQorKysga2pzL25vZGVzLmNw cAkod29ya2luZyBjb3B5KQpAQCAtMTUzNSwxMCArMTUzNSwxMCBAQCB1aW50MzJfdCBOb25Mb2Nh bFZhckZ1bmN0aW9uQ2FsbE5vZGU6OmV2CiAKIFJlZ2lzdGVySUQqIEZ1bmN0aW9uQ2FsbEJyYWNr ZXROb2RlOjplbWl0Q29kZShDb2RlR2VuZXJhdG9yJiBnZW5lcmF0b3IsIFJlZ2lzdGVySUQqIGRz dCkKIHsKLSAgICBSZWZQdHI8UmVnaXN0ZXJJRD4gcjAgPSBnZW5lcmF0b3IuZW1pdE5vZGUoZHN0 LCBtX2Jhc2UuZ2V0KCkpOwotICAgIFJlZ2lzdGVySUQqIHIxID0gZ2VuZXJhdG9yLmVtaXROb2Rl KGRzdCwgbV9zdWJzY3JpcHQuZ2V0KCkpOworICAgIFJlZlB0cjxSZWdpc3RlcklEPiByMCA9IGdl bmVyYXRvci5lbWl0Tm9kZShtX2Jhc2UuZ2V0KCkpOworICAgIFJlZ2lzdGVySUQqIHIxID0gZ2Vu ZXJhdG9yLmVtaXROb2RlKG1fc3Vic2NyaXB0LmdldCgpKTsKICAgICBSZWdpc3RlcklEKiByMiA9 IGdlbmVyYXRvci5lbWl0R2V0UHJvcFZhbChnZW5lcmF0b3IubmV3VGVtcG9yYXJ5KCksIHIwLmdl dCgpLCByMSk7Ci0gICAgcmV0dXJuIGdlbmVyYXRvci5lbWl0Q2FsbChyMC5nZXQoKSwgcjIsIHIw LmdldCgpLCBtX2FyZ3MuZ2V0KCkpOworICAgIHJldHVybiBnZW5lcmF0b3IuZW1pdENhbGwoZHN0 ID8gZHN0IDogZ2VuZXJhdG9yLm5ld1RlbXBvcmFyeU9yKHIwLmdldCgpKSwgcjIsIHIwLmdldCgp LCBtX2FyZ3MuZ2V0KCkpOwogfQogCiB2b2lkIEZ1bmN0aW9uQ2FsbEJyYWNrZXROb2RlOjpvcHRp bWl6ZVZhcmlhYmxlQWNjZXNzKEV4ZWNTdGF0ZSosIGNvbnN0IFN5bWJvbFRhYmxlJiwgY29uc3Qg TG9jYWxTdG9yYWdlJiwgTm9kZVN0YWNrJiBub2RlU3RhY2spCkBAIC0xNjEwLDkgKzE2MTAsOSBA QCBzdGF0aWMgY29uc3QgY2hhciogZG90RXhwckRvZXNOb3RBbGxvd0NhCiAKIFJlZ2lzdGVySUQq IEZ1bmN0aW9uQ2FsbERvdE5vZGU6OmVtaXRDb2RlKENvZGVHZW5lcmF0b3ImIGdlbmVyYXRvciwg UmVnaXN0ZXJJRCogZHN0KQogewotICAgIFJlZlB0cjxSZWdpc3RlcklEPiByMCA9IGdlbmVyYXRv ci5lbWl0Tm9kZShkc3QsIG1fYmFzZS5nZXQoKSk7CisgICAgUmVmUHRyPFJlZ2lzdGVySUQ+IHIw ID0gZ2VuZXJhdG9yLmVtaXROb2RlKG1fYmFzZS5nZXQoKSk7CiAgICAgUmVnaXN0ZXJJRCogcjEg PSBnZW5lcmF0b3IuZW1pdEdldFByb3BJZChnZW5lcmF0b3IubmV3VGVtcG9yYXJ5KCksIHIwLmdl dCgpLCBtX2lkZW50KTsKLSAgICByZXR1cm4gZ2VuZXJhdG9yLmVtaXRDYWxsKHIwLmdldCgpLCBy MSwgcjAuZ2V0KCksIG1fYXJncy5nZXQoKSk7CisgICAgcmV0dXJuIGdlbmVyYXRvci5lbWl0Q2Fs bChkc3QgPyBkc3QgOiBnZW5lcmF0b3IubmV3VGVtcG9yYXJ5T3IocjAuZ2V0KCkpLCByMSwgcjAu Z2V0KCksIG1fYXJncy5nZXQoKSk7CiB9CiAKIHZvaWQgRnVuY3Rpb25DYWxsRG90Tm9kZTo6b3B0 aW1pemVWYXJpYWJsZUFjY2VzcyhFeGVjU3RhdGUqLCBjb25zdCBTeW1ib2xUYWJsZSYsIGNvbnN0 IExvY2FsU3RvcmFnZSYsIE5vZGVTdGFjayYgbm9kZVN0YWNrKQo=