Bug 18436 - Need to throw exception on read/modify/write or similar resolve for nonexistent property
Summary: Need to throw exception on read/modify/write or similar resolve for nonexiste...
Status: RESOLVED FIXED
Alias: None
Product: WebKit
Classification: Unclassified
Component: JavaScriptCore (show other bugs)
Version: 528+ (Nightly build)
Hardware: Mac OS X 10.5
: P2 Normal
Assignee: Oliver Hunt
URL:
Keywords:
Depends on:
Blocks:
 
Reported: 2008-04-11 18:10 PDT by Oliver Hunt
Modified: 2008-04-15 02:21 PDT (History)
4 users (show)

See Also:


Attachments
Here we go (11.82 KB, patch)
2008-04-11 19:51 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Slightly updated (12.62 KB, patch)
2008-04-14 22:10 PDT, Oliver Hunt
no flags Details | Formatted Diff | Diff
Stripping everything unrelated to this bug (10.63 KB, patch)
2008-04-15 01:23 PDT, Oliver Hunt
mjs: review+
Details | Formatted Diff | Diff

Note You need to log in before you can comment on or make changes to this bug.
Description Oliver Hunt 2008-04-11 18:10:43 PDT
task tracking bug; basically
x++ (where is is undefined) should throw, but does not
Comment 1 Oliver Hunt 2008-04-11 19:51:15 PDT
Created attachment 20489 [details]
Here we go
Comment 2 Oliver Hunt 2008-04-14 22:10:03 PDT
Created attachment 20544 [details]
Slightly updated
Comment 3 Geoffrey Garen 2008-04-14 22:49:56 PDT
Comment on attachment 20544 [details]
Slightly updated

This patch is really 3 patches:
(1) Fix a bug where resolve-dependent writes do a get_prop_id and/or put_prop_id instead of a full resolve.
(2) Add a superinstruction optimization that merges resolve_base and resolve.
(3) Add exception checks after resolve operations.

Because each of these is a substantial change, I think each should land separately.

Taken individually:

Fix (1) needs to include assign and for-in, not just read/modify.

Fix (2) seems like a nice optimization, but why not do this as a peephole optimization in the code generator? 

Fix (3) needs to include normal resolve and resolve_base, not just read/modify. I'd also like to know exactly what the cost of fix (3) is before we land it, to make sure it doesn't cost more than we would reasonably expect.
Comment 4 Oliver Hunt 2008-04-14 23:01:13 PDT
(In reply to comment #3)
> (From update of attachment 20544 [details] [edit])
> This patch is really 3 patches:
> (1) Fix a bug where resolve-dependent writes do a get_prop_id and/or
> put_prop_id instead of a full resolve.
> (2) Add a superinstruction optimization that merges resolve_base and resolve.
> (3) Add exception checks after resolve operations.
> 
> Because each of these is a substantial change, I think each should land
> separately.
> 
> Taken individually:
> 
> Fix (1) needs to include assign and for-in, not just read/modify.
nope, resolve for assignment cannot throw an exception -- the put may, but that is handled by op_put_property_*

> 
> Fix (2) seems like a nice optimization, but why not do this as a peephole
> optimization in the code generator? 
Because it seems retarded to go to great lengths to deliberately obfuscate this fix in the name of not creating a superinstruction -- given the only difference would be us *deliberately* throwing away the base pointer

> 
> Fix (3) needs to include normal resolve and resolve_base, not just read/modify.
> I'd also like to know exactly what the cost of fix (3) is before we land it, to
> make sure it doesn't cost more than we would reasonably expect.
See above.

> 

Comment 5 Oliver Hunt 2008-04-14 23:01:54 PDT
Comment on attachment 20544 [details]
Slightly updated

Marking for re-review, i don't believe ggaren's were correct.
Comment 6 Oliver Hunt 2008-04-14 23:03:30 PDT
I suppose i could remove the op_resolve_base_and_func change, but that seems somewhat pointless given the triviality and obvious relationship to the op_resolve_base_and_property change
Comment 7 Geoffrey Garen 2008-04-14 23:16:56 PDT
resolve *does* need to throw if the resolve fails. "x + 1" must throw if x is not defined.

If you think the resolve_base_and_property optimization is required scaffolding for the bug fix, that's fine with me. Please land it first, and then land the bug fix, so we can measure the cost of the bug fix by itself.

On a personal note, please try to keep this conversation professional, Oliver. I think we can discuss differing opinions without resorting to insults like "retarded".
Comment 8 Oliver Hunt 2008-04-15 00:20:52 PDT
Comment on attachment 20544 [details]
Slightly updated

I'm removing the changes to resolveBaseAndFunc, that can be handled in a separate patch.
Comment 9 Oliver Hunt 2008-04-15 01:23:44 PDT
Created attachment 20547 [details]
Stripping everything unrelated to this bug
Comment 10 Maciej Stachowiak 2008-04-15 02:01:57 PDT
I'd look at this a little differently from Geoff.

Combining resolve_base followed by resolve of the same property on the same object isn't your typical superinstruction - the savings in combining them is not primarily dispatch but rather avoiding the need to walk the scope chain twice, because these operations do substantially the same work, they just report different pieces of the output (base vs. value). Indeed, we went to great lengths in the past to be able to share this work, when we used to always walk scope chains twice; this is the whole point of the getPropertySlot architecture.

In addition, we already have the very similar resolve_base_and_func.

I would say this is a special-purpose two-output opcode, and not just a regular combined instruction.

Also, doing a resolve_base followed by resolve would be a performance regression (one that is inessential to the basic regression of doing exception checks), as documented by Oliver.  Given that we have to change codegen to get the exception check right, may as well do it in a way that is a speedup instead of a regression.

Thus, since Oliver removed the truly unrelated parts, I will review the patch on the substance.
Comment 11 Maciej Stachowiak 2008-04-15 02:07:37 PDT
Comment on attachment 20547 [details]
Stripping everything unrelated to this bug

r=me

We could use test cases of these edge cases - maybe we already have them.
Comment 12 Oliver Hunt 2008-04-15 02:21:07 PDT
Committed r31898