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)

Other Branch
x86_64
Linux
defect
Not set
critical

Tracking

()

RESOLVED FIXED

People

(Reporter: decoder, Assigned: djvj)

References

Details

(Keywords: regression, testcase)

Attachments

(1 file, 1 obsolete file)

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: general → kvijayan
Status: NEW → ASSIGNED
Attached patch Patch (obsolete) — Splinter Review
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)
Attachment #629884 - Flags: review?(dvander) → review+
Attached patch Patch 2Splinter Review
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+
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.
Committed and pushed.  Waiting for tbpl before closing.

https://hg.mozilla.org/projects/ionmonkey/rev/2130ba968764
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.