845519 - Rootanalysis build failures

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Steve Fink [:sfink] [:s:]]

The rootanalysis build is still failing.

Comment 1

[Steve Fink [:sfink] [:s:]]

Attachment: Fix aliasing problem between Rooted<T> and Rooted<derived-from-T>

The build is being run on gcc 4.5 with optimization. That triggers a reordering of operations within Rooted<JSObject*> and Rooted<DerivedFromJSObject*>'s constructors. This patch changes the types of their fields to be the same.

Comment 2

[Steve Fink [:sfink] [:s:]]

Attachment: Check getType(cx) return value (also fixes rooting hazard)

getType(cx) is used all over the place without checking its return value. Getting the right return value for all of the places it can fail is tricky.

Comment 3

[Brian Hackett [Laid off!]]

Comment on attachment 718626 [details] [diff] [review]
Check getType(cx) return value (also fixes rooting hazard)

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

::: js/src/ion/IonBuilder.cpp
@@ +3871,5 @@
>  IonBuilder::getSingletonPrototype(JSFunction *target)
>  {
>      if (!target->hasSingletonType())
>          return NULL;
> +    types::TypeObject *targetType = target->getType(cx);

targetType should be null checked here

::: js/src/jsobj.h
@@ +445,5 @@
>       * Constructs a new, unique shape for the object.
>       */
>      static inline bool setSingletonType(JSContext *cx, js::HandleObject obj);
>  
> +    inline js::Unrooted<js::types::TypeObject*> getType(JSContext *cx);

rm the Unrooted.  I think the plan is to get rid of these, it'd be good to avoid adding more.

::: js/src/jsobjinlines.h
@@ +757,5 @@
>      obj->type_ = type;
>      return true;
>  }
>  
> +inline js::Unrooted<js::types::TypeObject*>

Ditto.

Comment 4

[Steve Fink [:sfink] [:s:]]

Attachment: Use SkipRoots for jschar pointers until we need to start moving them

Hm. Should we have a bug on file?

Comment 5

[Steve Fink [:sfink] [:s:]]

(In reply to Brian Hackett (:bhackett) from comment #3)
> Comment on attachment 718626 [details] [diff] [review]
> Check getType(cx) return value (also fixes rooting hazard)
> 
> targetType should be null checked here

fixed

> rm the Unrooted.  I think the plan is to get rid of these, it'd be good to
> avoid adding more.

sorry; I meant to rip these out. They were from the initial investigation.

Comment 6

[Terrence Cole [:terrence]]

Comment on attachment 718694 [details] [diff] [review]
Use SkipRoots for jschar pointers until we need to start moving them

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

::: js/src/frontend/TokenStream.h
@@ +757,5 @@
>          const jschar *base_;            /* base of buffer */
>          const jschar *limit_;           /* limit for quick bounds check */
>          const jschar *ptr;              /* next char to get */
> +
> +        // We are not yet moving strings

Missing a . at the end.

Comment 7

[Steve Fink [:sfink] [:s:]]

Attachment: Spot-fix Unrooted-related problems

The stack of patches in this bug are enough to get a green root analysis build.

https://tbpl.mozilla.org/?tree=Try&rev=f4de277690fd

Comment 8

[Steve Fink [:sfink] [:s:]]

http://hg.mozilla.org/integration/mozilla-inbound/rev/bd745ff1a2e9
http://hg.mozilla.org/integration/mozilla-inbound/rev/24f8ec282019
http://hg.mozilla.org/integration/mozilla-inbound/rev/b93b177e2a9d
http://hg.mozilla.org/integration/mozilla-inbound/rev/537fd7f9486b

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/537fd7f9486b
https://hg.mozilla.org/mozilla-central/rev/b93b177e2a9d
https://hg.mozilla.org/mozilla-central/rev/24f8ec282019
https://hg.mozilla.org/mozilla-central/rev/bd745ff1a2e9