Last Comment Bug 750582 - IonMonkey: eager write on global non-writable properties. (jaeger/bug584647.js:2: Error: Assertion failed: got false, expected true)
: IonMonkey: eager write on global non-writable properties. (jaeger/bug584647.j...
Status: RESOLVED FIXED
:
Product: Core
Classification: Components
Component: JavaScript Engine (show other bugs)
: Other Branch
: x86_64 Linux
: -- normal (vote)
: ---
Assigned To: Nicolas B. Pierron [:nbp]
:
: Jason Orendorff [:jorendorff]
Mentors:
Depends on:
Blocks: IonEager
  Show dependency treegraph
 
Reported: 2012-04-30 18:44 PDT by Nicolas B. Pierron [:nbp]
Modified: 2012-05-04 00:16 PDT (History)
1 user (show)
See Also:
Crash Signature:
(edit)
QA Whiteboard:
Iteration: ---
Points: ---
Has Regression Range: ---
Has STR: ---


Attachments
Check if shape is writable before registering IC. (1.52 KB, patch)
2012-04-30 18:50 PDT, Nicolas B. Pierron [:nbp]
no flags Details | Diff | Splinter Review
Remove unexpected properties from the global object. (14.56 KB, patch)
2012-05-01 16:46 PDT, Nicolas B. Pierron [:nbp]
dvander: review+
Details | Diff | Splinter Review

Description Nicolas B. Pierron [:nbp] 2012-04-30 18:44:53 PDT
This bug can be reproduced with

./js --ion-eager ./jit-test/tests/jaeger/bug584647.js
./jit-test/tests/jaeger/bug584647.js:2: Error: Assertion failed: got false, expected true
Comment 1 Nicolas B. Pierron [:nbp] 2012-04-30 18:50:48 PDT
Created attachment 619807 [details] [diff] [review]
Check if shape is writable before registering IC.
Comment 2 Nicolas B. Pierron [:nbp] 2012-05-01 16:40:47 PDT
Another similar issue which implies changes of set-property (JSOP_SETNAME) code is:

./js --ion-eager ./jit-test/tests/jaeger/bug735161.js => ./jit-test/tests/jaeger/bug735161.js:4: Error: Assertion failed: got true, expected false

As it look similar and modify similar code I am adding the changes to the current patch in addition to the suggestion to clean-up SetPropertyCache made by dvander on IRC.
Comment 3 Nicolas B. Pierron [:nbp] 2012-05-01 16:46:00 PDT
Created attachment 620135 [details] [diff] [review]
Remove unexpected properties from the global object.

* Check if the shape is writable.
* Clean-up SetPropertyCache and extract isSetPropertyInlinable and
  isPropertyInlinable, rename the other predicate to 
  isAddPropertyInlinable to clarify.
* Add VMFunction to handle the bottom of SetPropertyOperation and use
  it inside SetPropertyCache to fix wrong use of SETNAME on the global 
  object.
Comment 4 David Anderson [:dvander] 2012-05-03 11:55:43 PDT
Comment on attachment 620135 [details] [diff] [review]
Remove unexpected properties from the global object.

Review of attachment 620135 [details] [diff] [review]:
-----------------------------------------------------------------

Thanks for doing this refactoring! This is much more readable than the existing code.

::: js/src/ion/IonCaches.cpp
@@ +449,5 @@
>      return true;
>  }
>  
>  static bool
> +isPropertyInlinable(JSObject *obj, IonCacheSetProperty &cache)

nit: IsPropertyInlineable

@@ +466,5 @@
> +    return true;
> +}
> +
> +static bool
> +isSetPropertyInlinable(JSContext *cx, JSObject *obj, JSAtom *atom, jsid *pId, const Shape **pShape)

nit: IsPropertySetInlineable

@@ +492,5 @@
> +}
> +
> +static bool
> +isAddPropertyInlinable(JSContext *cx, JSObject *obj, jsid id, uint32_t oldSlots,
> +                       const Shape **pShape)

nit: IsPropertyAddInlineable

@@ +527,5 @@
>      return true;
>  }
>  
>  bool
> +js::ion::SetPropertyCache(JSContext *cx, size_t cacheIndex, JSObject *obj, const Value& value,

We should probably make this gc-safe while we're here: change |JSObject *obj| to be |HandleObject obj| and |const Value &value| to be |HandleValue value|.

::: js/src/ion/VMFunctions.cpp
@@ +288,5 @@
>  }
>  
> +bool
> +SetProperty(JSContext *cx, JSObject *obj, JSAtom *atom, const Value &value,
> +                       bool strict, bool isSetName)

nit: bool should align with JSContext

@@ +292,5 @@
> +                       bool strict, bool isSetName)
> +{
> +    Value v = value;
> +    jsid id = ATOM_TO_JSID(atom);
> +    RootObject objRoot(cx, &obj);

|obj| isn't rooted, so we can't use RootObject (wow these names are confusing!)

Instead, change |JSObject *obj| to |HandleObject obj|.
Comment 5 David Anderson [:dvander] 2012-05-03 11:58:19 PDT
Err -- sorry, we actually *can* use RootObject there, but I think we're supposed to propagate HandleObject instead
Comment 6 Nicolas B. Pierron [:nbp] 2012-05-04 00:16:23 PDT
https://hg.mozilla.org/projects/ionmonkey/rev/68777651999a

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