795743 - Miscellaneous rooting issues in self-hosting infrastructure and Method JIT

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Norbert Lindenberg]

Attachment: patch with miscellaneous fixes

Using rooting verification mode on code for the ECMAScript Internationalization API, I found and fixed a few issues in other code (self-hosting support, JIT). I'd appreciate if someone more familiar with the modified code could take it from here.

Changes in the attached patch:

- a few API changes from jsid to HandleId and from Value* to MutableHandleValue, plus the required changes to callers,

- several additional Rooted<T> declarations and related changes.

For the latter I verified that they are required, i.e., undoing any of them reintroduced assertion failures.

Comment 1

[Terrence Cole [:terrence]]

Comment on attachment 666364 [details] [diff] [review]
patch with miscellaneous fixes

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

Nice job!  Just a couple small nits.  I'll go ahead and apply them and get this in tree.

::: js/src/jscntxt.cpp
@@ +314,5 @@
>      return funVal.toObject().toFunction();
>  }
>  
>  bool
> +JSRuntime::cloneSelfHostedValueById(JSContext *cx, HandleId id, HandleObject holder, Value *vp)

We should just make vp a MutableHandleValue here, rather than using result as a temporary.

::: js/src/methodjit/Compiler.cpp
@@ +78,5 @@
>      globalSlots(globalObj ? globalObj->getRawSlots() : NULL),
>      sps(&cx->runtime->spsProfiler),
>      masm(&sps, &PC),
>      frame(cx, *thisFromCtor(), masm, stubcc),
> +    a(NULL), outer(NULL), script_(cx, NULL), PC(NULL), loop(NULL),

Rooted will default to a sane value (NULL for T*), so we leave if off here.

Comment 2

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/495a115ec2cc

Comment 3

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/495a115ec2cc

Should this have tests?

Comment 4

[Norbert Lindenberg]

(In reply to Ryan VanderMeulen from comment #3)
> 
> Should this have tests?

Reliably reproducing the assertion failures that prompted these changes requires a special SpiderMonkey build (--enable-root-analysis). I assume such a build isn't produced as part of regular build and test processes, so a test case wouldn't be very useful. In addition, the test case I used relies on a pile of changes that haven't been committed yet.

Comment 5

[Terrence Cole [:terrence]]

Actually, we run the full jit-test suite in the rooting analysis mode.  You can find it here: https://tbpl.mozilla.org/?tree=Mozilla-Inbound&noignore=1&jobname=spidermonkey under 'r'.

This particular change is just moving to a new API, so the existing tests should cover this case, or new tests should get added on the bug covering the actual feature work.