Bug 714647 got this most of the way there (hopefully!).

Passes about 80% of jit-tests in the interpreter.

Passes all jit-tests in the interpreter, modulo some timeouts. There are a couple caveats: - The rooting analysis is disabled if e4x or Reflect.parse is used. Both of these would require a lot of work and changes to poorly tested/exercised code which I'm not interested in undertaking (started on jsxml.cpp and eventually gave up). This can be addressed either by removing/disabling these in the browser, or (more likely) I think wrapping this functionality in a scope-based class which enables the conservative scanner (and disables moving GC while inside it --- not sure how much of a pain this would be for GGC). In any case the patch just punts on this for now. - Various SkipRoots added for places where internal cursors into GC things are used (strings, arrays, typed arrays). Again, the patch is punting on these because fixing these requires some deeper changes to the code, and this patch is just adding rooters without fiddling with underlying functionality.

Comment on attachment 617561 [details] [diff] [review] patch (67ca169a52d2) Review of attachment 617561 [details] [diff] [review]: ----------------------------------------------------------------- Thanks, Brian! ::: js/src/jsapi.cpp @@ +3651,5 @@ > getter = JS_DATA_TO_FUNC_PTR(PropertyOp, getobj); > attrs |= JSPROP_GETTER; > } > if (setter) { > + RootObject getRoot(cx, (JSObject **) &getter); If we root getter here, shouldn't we root setter above? ::: js/src/jsapi.h @@ +2290,5 @@ > JS_ALWAYS_INLINE bool > ToNumber(JSContext *cx, const Value &v, double *out) > { > AssertArgumentsAreSane(cx, v); > + Extra ' ' character here. ::: js/src/jscntxt.h @@ +159,5 @@ > uintptr_t words[JS_HOWMANY(sizeof(jmp_buf), sizeof(uintptr_t))]; > } registerSnapshot; > > ConservativeGCData() > + { Usually the brace goes on the same line as the function name in these sorts of definitions. ::: js/src/jsgc.cpp @@ +3938,5 @@ > rt->gcDeterministicOnly = enabled; > #endif > } > > +} } /* namespace js::gc */ Could you put this on two lines? ::: js/src/jstypedarray.cpp @@ +464,1 @@ > PropertyOp getter, StrictPropertyOp setter, unsigned attrs) Fix the indent here? ::: js/src/vm/RegExpStatics.h @@ +255,4 @@ > : original(original), > + buffer(RegExpStatics::InitBuffer()), > + matchPairsInputRoot(cx, (JSLinearString**) &buffer.matchPairsInput), > + pendingInputRoot(cx, (JSString**) &buffer.pendingInput) Could you create one of those inner class rooter things for RegExpStatics like you did in a few other places?

Brian, I needed this to debug the generational gc, so I went and rebased. Hope it saves you some time.

Looks like this broke windebug SM builds: <https://tbpl.mozilla.org/?tree=Mozilla-Inbound&noignore=1&jobname=spidermonkey&rev=07a4d4b0260c>

I landed fixes for the various build warnings this introduced -- basically from passing in |this| when constructing member variables, using the old |thisDuringConstruction()| trick: https://hg.mozilla.org/integration/mozilla-inbound/rev/489e1b75048e Also I landed a fix for bug 751567, where MSVC is going insane complaining about a mismatched brace in code that's clearly not brace-mismatching: https://hg.mozilla.org/integration/mozilla-inbound/rev/4608e8e96c87 I put the minimal investigations I made in a comment in the revision. The summary is that something about an opening brace followed by constructing an instance of a class that uses the guard-object macros triggers the issue. An explicit scope triggers it, as does the same code in a completely separate method. I didn't see a hackaround that would have equivalent functionality. Given that I have no idea what's up here, and various attempts at a hackaround all failed, I gave up and just put an #ifndef _MSC_VER / #endif around the problematic code. It's not the right long-term fix (I hope), but it gets us compiling again, at least. Leaving open for that super-small long-term fix, whatever it might end up being...

https://hg.mozilla.org/mozilla-central/rev/489e1b75048e https://hg.mozilla.org/mozilla-central/rev/4608e8e96c87

(In reply to Justin Wood (:Callek) from bug 751567 comment #1) > My initial theory is that this is failing with MSVC8 and would succeed with MSVC10 (In reply to Jeff Walden [:Waldo] (busy, try to prefer other reviewers if possible) from comment #8) > I [...] just put an #ifndef _MSC_VER / #endif around the problematic code. Would something like "#if !defined(_MSC_VER) || _MSC_VER > 1400" be (more) appropriate?

Perhaps, but I didn't see this bug or that bug before adding the ifdefs, so I didn't realize this went away in newer versions of MSVC. Also I'm assuming there's a better fix that will do the root-skipping and maybe-checking even in MSVC, so it's not important how pretty whatever hackaround's there now is, because it'll get replaced with a real fix soon.

The rooting analysis is green on TBPL now.

Nevermind, this bug still has a long-term nit for windows builds that we need to fix.

We're switching to a different mechanism for dynamic rooting analysis.