Closed
Bug 758366
Opened 12 years ago
Closed 12 years ago
IonMonkey: Differential Testing: Getting null vs. undefined with/without ion
Categories
(Core :: JavaScript Engine, defect)
Tracking
()
RESOLVED
FIXED
People
(Reporter: decoder, Assigned: djvj)
References
Details
(Keywords: regression, testcase)
Attachments
(1 file, 1 obsolete file)
(deleted),
patch
|
dvander
:
review+
|
Details | Diff | Splinter Review |
The following testcase shows different behavior with options --ion -n -m --ion-eager vs. --no-ion on ionmonkey revision c05b873dad48:
function get_value_null(o) {
return o.value
}
var o_null = {value: null}
get_value_null(o_null)
print(get_value_null(0))
$ debug64/js --ion -n -m --ion-eager test.js
null
$ debug64/js --no-ion test.js
undefined
Assignee | ||
Updated•12 years ago
|
Assignee: general → kvijayan
Assignee | ||
Updated•12 years ago
|
Status: NEW → ASSIGNED
Assignee | ||
Comment 1•12 years ago
|
||
The issue here is in IonBuilder's jsop_getprop implementation. When compiling, it attempts to check for a definite slot in the case where the getprop's typeset contains a single object type with a known definite slot for the property being retrieved.
However, |GetDefiniteSlot| has the semantics of returning the definite slot _if_ the type is assumed to be an object type, and doesn't handle the case where the typeset could contain a mix of an object and primitive types.
jsop_getprop does not do a check for non-object types being present in the typeset before checking for a definite slot with |GetDefiniteSlot|, which was causing it to mishandle the case when the typeset for |o| in |o.value| was updated to include int as an incoming type.
Attachment #629884 -
Flags: review?(dvander)
Updated•12 years ago
|
Attachment #629884 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 2•12 years ago
|
||
Dvander pointed out that JM handle this case by inserting an object-check guard in front of the fixed-slot getter.
After talking it over with jandem, there seems to be another issue potentially at play here.
The type policy for MLoadFixedSlot should insert an MUnbox(Fallible) in front if necessary. However, since the result typeset of the MLoadFixedSlot includes only the null type, the MLoadFixedSlot is eliminated and replaced with a constant null. This leaves the MUnbox with zero uses, which means that it is also eliminated.
I'm not sure whether that optimization behavior is a bug or not. The elimination of the MLoadFixedSlot is only valid in the presence of the typecheck implied by the fallible unbox. So having the optimization procedures remove both via some sequence of decisions seems to be an error.
David can you comment?
In the meantime, I've modified the patch to do a guard.
Attachment #629884 -
Attachment is obsolete: true
Attachment #630236 -
Flags: review?(dvander)
Comment on attachment 630236 [details] [diff] [review]
Patch 2
Review of attachment 630236 [details] [diff] [review]:
-----------------------------------------------------------------
I think a guard somewhere is the right behavior. Unfortunately there's no feedback from the guard, so if it fails, we'll always re-compile with the same guard. If that proves to be a problem we can be more clever.
Attachment #630236 -
Flags: review?(dvander) → review+
Assignee | ||
Comment 4•12 years ago
|
||
Leaving that aside, isn't there an issue with the MUnbox getting optimized away in the first place? I.e. that sounds like an optimization problem. This is what I wanted to ask you (I talked it over with jandem and he recommended running it by you).
Namely: The MUnbox's fallible typecheck implies the ability to eliminate the MLoadFixedSlot, but once the MLoadFixedSlot is eliminated, that's taken as license by the optimizer to eliminate the MUnbox, even though the presence of the MUnbox is what allows the first elimination.
I'm wondering if there's a subtle logic error in this behaviour.
Yeah, good point. Normally the unbox could be eliminated, but MGuardObject should have the guard flag automatically set, which prevents elimination.
Assignee | ||
Comment 6•12 years ago
|
||
Committed and pushed. Waiting for tbpl before closing.
https://hg.mozilla.org/projects/ionmonkey/rev/2130ba968764
Comment 7•12 years ago
|
||
Follow-up fixing orange: http://hg.mozilla.org/projects/ionmonkey/rev/cca924e017ea
Status: ASSIGNED → RESOLVED
Closed: 12 years ago
Resolution: --- → FIXED
You need to log in
before you can comment on or make changes to this bug.
Description
•