Miscellaneous rooting issues in self-hosting infrastructure and Method JIT

RESOLVED FIXED in mozilla18

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: Norbert Lindenberg, Unassigned)

Tracking

unspecified
mozilla18
Points:
---
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [js:t])

Attachments

(1 attachment)

(Reporter)

Description

5 years ago
Created attachment 666364 [details] [diff] [review]
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.
Attachment #666364 - Attachment is patch: true
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.
Attachment #666364 - Flags: review+
Attachment #666364 - Flags: checkin?(terrence)
https://hg.mozilla.org/integration/mozilla-inbound/rev/495a115ec2cc
Blocks: 753203
Whiteboard: [js:t]
Attachment #666364 - Flags: checkin?(terrence) → checkin+
https://hg.mozilla.org/mozilla-central/rev/495a115ec2cc

Should this have tests?
Status: NEW → RESOLVED
Last Resolved: 5 years ago
Flags: in-testsuite?
Resolution: --- → FIXED
Target Milestone: --- → mozilla18
(Reporter)

Comment 4

5 years ago
(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.
Flags: in-testsuite? → in-testsuite-
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.
You need to log in before you can comment on or make changes to this bug.