Bug 18436 – Need to throw exception on read/modify/write or similar resolve for nonexistent property

• Component: JavaScriptCore
• Status: RESOLVED FIXED

[Oliver Hunt]

task tracking bug; basically
x++ (where is is undefined) should throw, but does not

[Oliver Hunt]

Created attachment 20489 [details]
Here we go

[Oliver Hunt]

Created attachment 20544 [details]
Slightly updated

[Geoffrey Garen]

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.

[Oliver Hunt]

(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.

> 

[Oliver Hunt]

Comment on attachment 20544 [details]
Slightly updated

Marking for re-review, i don't believe ggaren's were correct.

[Oliver Hunt]

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

[Geoffrey Garen]

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".

[Oliver Hunt]

Comment on attachment 20544 [details]
Slightly updated

I'm removing the changes to resolveBaseAndFunc, that can be handled in a separate patch.

[Oliver Hunt]

Created attachment 20547 [details]
Stripping everything unrelated to this bug

[Maciej Stachowiak]

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.

[Maciej Stachowiak]

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.

[Oliver Hunt]

Committed r31898