Last Comment Bug 758366 - IonMonkey: Differential Testing: Getting null vs. undefined with/without ion
: IonMonkey: Differential Testing: Getting null vs. undefined with/without ion
Status: RESOLVED FIXED
: regression, testcase
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- critical (vote)
: ---
Assigned To: Kannan Vijayan [:djvj]
:
Mentors:
Depends on:
Blocks: langfuzz IonFuzz
  Show dependency treegraph
 
Reported: 2012-05-24 13:55 PDT by Christian Holler (:decoder)
Modified: 2012-06-06 16:38 PDT (History)
7 users (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Patch (1.60 KB, patch)
2012-06-04 12:40 PDT, Kannan Vijayan [:djvj]
dvander: review+
Details | Diff | Splinter Review
Patch 2 (1.06 KB, patch)
2012-06-05 11:25 PDT, Kannan Vijayan [:djvj]
dvander: review+
Details | Diff | Splinter Review

Description Christian Holler (:decoder) 2012-05-24 13:55:31 PDT
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
Comment 1 Kannan Vijayan [:djvj] 2012-06-04 12:40:19 PDT
Created attachment 629884 [details] [diff] [review]
Patch

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.
Comment 2 Kannan Vijayan [:djvj] 2012-06-05 11:25:43 PDT
Created attachment 630236 [details] [diff] [review]
Patch 2

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.
Comment 3 David Anderson [:dvander] 2012-06-05 17:34:01 PDT
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.
Comment 4 Kannan Vijayan [:djvj] 2012-06-06 07:57:14 PDT
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.
Comment 5 David Anderson [:dvander] 2012-06-06 09:01:44 PDT
Yeah, good point. Normally the unbox could be eliminated, but MGuardObject should have the guard flag automatically set, which prevents elimination.
Comment 6 Kannan Vijayan [:djvj] 2012-06-06 14:38:03 PDT
Committed and pushed.  Waiting for tbpl before closing.

https://hg.mozilla.org/projects/ionmonkey/rev/2130ba968764
Comment 7 Sean Stangl [:sstangl] 2012-06-06 16:38:50 PDT
Follow-up fixing orange: http://hg.mozilla.org/projects/ionmonkey/rev/cca924e017ea

Note You need to log in before you can comment on or make changes to this bug.