772303 - Get rooting analysis builds green on tinderbox

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Brian Hackett [Laid off!]]

Right now the root analysis builds on tinderbox are orange, because while we used to pass jit-tests under the analysis, changes in the last couple months have introduced new rooting problems.  This bug is about fixing these and getting these entries green on the tinderbox.  Once the root analysis builds are green, they need to stay green and patches which break the build need to be promptly fixed or backed out.

Comment 1

[Brian Hackett [Laid off!]]

Attachment: nearly working (84623518d428)

Fix all but a few tests that fail when running jit-tests in the interpreter.  Not looking to fix absolutely everything in the first go, as I want to see if my config matches tbpl's behavior and things will break again quickly anyways.

Comment 2

[Bill McCloskey [inactive unless it's an emergency] (:billm)]

Comment on attachment 640465 [details] [diff] [review]
nearly working (84623518d428)

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

::: js/src/jscntxtinlines.h
@@ +107,2 @@
>  
>      JSObject *obj = js_TryNewGCObject(cx, entry->kind);

Maybe exit early if obj is null, and then the code below can be un-indented.

::: js/src/vm/RegExpObject.h
@@ +338,5 @@
>  
>      /* Accessors. */
>  
>      const Value &getLastIndex() const {
>          return getSlot(LAST_INDEX_SLOT);

Could this just return a double? As far as I can tell, we set the slot to zero on creation, so I don't think we need to worry about undefined.

Comment 3

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/c048a86eb289

Comment 4

[Brian Hackett [Laid off!]]

(In reply to Bill McCloskey (:billm) from comment #2)
> Could this just return a double? As far as I can tell, we set the slot to
> zero on creation, so I don't think we need to worry about undefined.

It turns out that lastIndex is also a normal data property, and can be assigned any value from script (blech).

Comment 5

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/c048a86eb289

Comment 6

[Brian Hackett [Laid off!]]

Attachment: rooters for JM and TI (c048a86eb289)

More rooters to get jitflags -ma and -mna to work.

Comment 7

[Terrence Cole [:terrence]]

Comment on attachment 641146 [details] [diff] [review]
rooters for JM and TI (c048a86eb289)

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

Nice.

::: js/src/jsgc.cpp
@@ +4280,5 @@
> +    if (cx->compartment->activeAnalysis)
> +        return;
> +
> +    // Can switch to the atoms compartment during analysis.
> +    if (cx->compartment == rt->atomsCompartment) {

IsAtomsCompartment(cx->compartment)

::: js/src/methodjit/StubCalls.cpp
@@ +466,5 @@
>      FrameRegs &regs = f.regs;
>  
> +    RootedValue rval_(cx, regs.sp[-1]);
> +    RootedValue lval_(cx, regs.sp[-2]);
> +    Value &rval = rval_.get(), &lval = lval_.get();

Make a note somewhere to clean all of these up later.

Comment 8

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/fad7d06d7dd5

Comment 9

[Brian Hackett [Laid off!]]

Attachment: rest of the way

The last patch left just four failures on the rooting analysis builds on tbpl.  This should fix all of them.

Comment 10

[Terrence Cole [:terrence]]

Comment on attachment 641304 [details] [diff] [review]
rest of the way

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

Gross, but good enough for now.

Comment 11

[Ed Morley [:emorley]]

Backed for causing the winxp pgo-only reftest failure:
https://tbpl.mozilla.org/php/getParsedLog.php?id=13455750&tree=Mozilla-Inbound

{
REFTEST TEST-UNEXPECTED-FAIL | file:///c:/talos-slave/test/build/jsreftest/tests/jsreftest.html?test=ecma_3/RegExp/15.10.6.2-2.js | Section 6 of test -
regexp = /abc/gi
string = 'AbcaBcabC'
ERROR !!! regexp MATCHED when we expected it to fail !!!
Expect: null
Actual: ["Abc"]
 Type mismatch, expected type object, actual type string Expected value 'null', Actual value '["Abc"]'  item 6
...etc
}

https://hg.mozilla.org/integration/mozilla-inbound/rev/f0be4b70b814

Unfortunately had to back out a whole bunch of other stuff at the same time (see commit message) due to conflicts.

Comment 12

[Ed Morley [:emorley]]

(Just to clarify, the just comment 8 landing backed out)

Comment 13

[Ed Morley [:emorley]]

s/the just/just the/

(it's clearly going to be one of those days...)

Comment 14

[Phil Ringnalda (:philor)]

WinXP-mostly, rather than WinXP-only - we got a Win7 failure in https://tbpl.mozilla.org/php/getParsedLog.php?id=13459348&tree=Mozilla-Inbound.

Comment 15

[Brian Hackett [Laid off!]]

Same patch, except that Windows PGO is disabled for RegExp.cpp.  Making this change fixed the reftests orange on WinXP PGO builds (compare https://tbpl.mozilla.org/?tree=Try&rev=ff3ccf8df758 with https://tbpl.mozilla.org/?tree=Try&rev=75c9502def0c).  This is the strategy we've taken before when the PGO compiler breaks (bug 721284).

https://hg.mozilla.org/integration/mozilla-inbound/rev/848ed9d56932

Comment 16

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/848ed9d56932

Comment 17

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

It appears rev 98920:c048a86eb289 (Add more rooters, bug 772303. r=billm) has broken Windows debug builds using VS2008.  VS2010 works, VS2008 without debug works.  The specific error is:

o:\src\mm\mozilla-hg\mozilla-central\js\src\gc/Root.h(351) : fatal error C1075: end of file found before the left brace '{' at 'o:\src\mm\mozilla-hg\mozilla-central\js\src\jsstr.h(127)' was matched

Comment 18

[Brian Hackett [Laid off!]]

Hmm, that looks like a bug in the compiler.  If you change the #ifdef DEBUG in jsstr.h near where the error occurs to:

'#if defined(DEBUG) && !defined(_MSC_VER)'

Does the build still fail, or fail somewhere else?

https://hg.mozilla.org/integration/mozilla-inbound/rev/30a4cd976842

Comment 19

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/30a4cd976842

Comment 20

[Mark Hammond (PTO; back Oct-3) [:markh] [:mhammond]]

(In reply to Brian Hackett (:bhackett) from comment #18)
> Hmm, that looks like a bug in the compiler.  If you change the #ifdef DEBUG
> in jsstr.h near where the error occurs to:
> 
> '#if defined(DEBUG) && !defined(_MSC_VER)'
> 
> Does the build still fail, or fail somewhere else?

It still fails with the exact same error in 4 places in jsnum.h which use the exact same construct.  So with that added 5 times in total, the build works.

I tried having the preprocessor output sent to a file to try and help narrow down what is going wrong but that didn't help.  I tried removing the braces in those blocks and it then generated the same error but for the opening brace of the function itself.  So I'm quite stumped by this and have to lean towards the "bug in the compiler" theory.

I probably should also note I'm using VS2008 *without* its Service Pack (mainly as the service pack bumps the version of the CRT assemblies which causes problems for other projects), so that may be a factor.

Comment 21

[[e-mail]]

I'm seeing the same compiler bug, using the compiler that ships with the Windows 7 SDK (effectively the same as VC2008 Express).

I tried extracting the JSGuardObjectNotifier, JSGuardObjectNotificationReceiver and SkipRoot (basic version without analysis) classes and the ToString function, although to avoid compiling large swathes of JS I changed all pointers to void*, and the resulting snippets did seem to compile OK.

However, while I was fiddling, I did find a trivial fix for the problem!

Comment 22

[[e-mail]]

Attachment: Proposed patch

I tried prefixing JS_GUARD_OBJECT_NOTIFIER_PARAM with `, const NullPtr& param = NullPtr()` and that didn't compile either, but `, const RootMethods<T>& param = RootMethods<T>()` did compile, so I can only guess that the compiler is somehow getting confused with its overload resolution. 

Then I realised that there was a simple way to avoid the problem :-)

Comment 23

[[e-mail]]

Comment on attachment 642513 [details] [diff] [review]
Proposed patch

Pushed mozilla-inbound changeset 4323e17a12c4.

Comment 24

[Ed Morley [:emorley]]

Didn't make it to mozilla-central before the uplift (merge was blocked on bug 774259). Adjusting milestone accordingly.

Comment 25

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/4323e17a12c4

Comment 26

[Kent James (:rkent)]

Will this get uploaded to aurora (Moz 16) at some point, because without it VS 2008 fails on aurora debug builds?

Comment 28

[Terrence Cole [:terrence]]

Attachment: v0: Fixes some obvious bustage.

Some of my rebasing of Steve's patch which landed yesterday introduced some obvious bustage, which I should have caught.  This patch fixes that and some other obvious failures triggered by jstests.  With this patch we only have a handful of jstests not passing.  A bunch still fail with timeouts, however, so we still have work to do.  There is also at least 1 semantic error introduced when we run a rooting analysis build with zeal on (but it goes away with zeal off, oddly).  It appears to be present even without this patch (or at least with the 2 trivial changes required to get it running at all).

ecma_5/strict/11.1.5.js fails at line 99 at the test:
assertEq(testLenientAndStrict('({get x() {}, x:1})',
                              parseRaisesException(SyntaxError),
                              parseRaisesException(SyntaxError)),
         true);

Comment 29

[Terrence Cole [:terrence]]

Comment on attachment 648551 [details] [diff] [review]
v0: Fixes some obvious bustage.

Hold off on this for a bit.  When I reran the test suite, pretty much everything is busted.

Comment 30

[Terrence Cole [:terrence]]

Comment on attachment 648551 [details] [diff] [review]
v0: Fixes some obvious bustage.

And the brokenness has disappeared without a rebuild.  I did not think that I had rebuilt a broken executable under the test runner, but then I would have paused it if I had remembered....

Comment 31

[Terrence Cole [:terrence]]

Attachment: v1: simple cleanup.

JSON::Walk is not hot enough to warrant saving one root.

Comment 32

[Terrence Cole [:terrence]]

Attachment: vLast: (for this sequence)

One more update: I promise this is the last.  This doesn't get jstests quite green, instead I hit a problem that the current SkipRoot can't handle: the compiler is saving and restoring a temporary that we're not skipping.  We could "solve" it by changing the code, but this is in typedarray copy, so is almost certainly going to hurt gaming perf if we go that way.

Instead I'm going to open a new bug to work on implementing InternalRoot so that the analysis can do exact tracking of these internal pointers to avoid killing temporaries.

Comment 33

[Terrence Cole [:terrence]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/633d8533e7cf

Comment 34

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/633d8533e7cf

Comment 35

[Brian Hackett [Laid off!]]

Attachment: more rooters (440c8b75f1ce)

Get jit-tests green again in the interpreter for me, fix some fuzz bugs decoder found, clean up various rubbish.

Comment 36

[Terrence Cole [:terrence]]

Comment on attachment 650980 [details] [diff] [review]
more rooters (440c8b75f1ce)

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

This is nice, particularly the correct rooting in ReportError.  It seems like every day brings us closer to being reliably and consistently rooted.  There are just a few nits to take care of before landing.

::: js/src/builtin/Eval.cpp
@@ +85,5 @@
>      /* These fields are only valid if lookup_.str is non-NULL. */
>      EvalCacheLookup lookup_;
>      EvalCache::AddPtr p_;
>  
> +    Rooted<JSLinearString*> lookupStr;

It's not my preferred style either, but please postfix this with _ to match the surrounding code.

::: js/src/jit-test/tests/sunspider/check-crypto-md5.js
@@ +283,5 @@
>      plainText += plainText;
>  }
>  
> +if (relaxRootChecks)
> +  relaxRootChecks();

Could you add a comment explaining why we disable rooting analysis on this test?

::: js/src/jit-test/tests/sunspider/check-crypto-sha1.js
@@ +221,5 @@
>      plainText += plainText;
>  }
>  
> +if (relaxRootChecks)
> +  relaxRootChecks();

Could you add a comment explaining why we disable rooting analysis on this test too?

::: js/src/jsarray.cpp
@@ +2880,4 @@
>          if (argc == 0)
>              return JS_TRUE;
>          argc--;
>          p++;

This is a bit horrifying.  If we take this path without exiting at argc==0, the CallArgs is not going to reflect the adjustment made to the args here.  I ran into this as well on bug 688852.  I think what you have is safe; I just need to make a note to revisit that bug with Jeff gets back.

::: js/src/jsatom.cpp
@@ +298,5 @@
>  
>      SkipRoot skip(cx, &chars);
>  
> +    /* Workaround for hash values in AddPtr being inadvertently poisoned. */
> +    SkipRoot skip2(cx, &p);

Thanks for the comment.  Also, eww!

::: js/src/jsdate.cpp
@@ +2619,5 @@
>          return false;
>      }
>  
>      /* Step 6. */
> +    InvokeArgsGuard newArgs;

InvokeArgsGuards should be called |ag| to match what Jeff has done in jsarray.

::: js/src/jsfun.cpp
@@ +752,3 @@
>      uint32_t indent = 0;
>  
> +    if (argc != 0 && !ToUint32(cx, args[0], &indent))

args.length()

::: js/src/jsinterp.cpp
@@ +128,5 @@
>  
> +    if (!thisv.isObject()) {
> +        if (!js_PrimitiveToObject(cx, &thisv))
> +            return false;
> +        call.setThis(thisv);

I assume js_PrimitiveToObject cannot GC after it sets thisv -- it would be nice if we could clarify this in the code somehow.  Obviously not something we can fix in this case, but something to think about for later.

::: js/src/jsinterpinlines.h
@@ -507,5 @@
>      {
> -        RootedValue lval_(cx, lhs);
> -        RootedValue rval_(cx, rhs);
> -        Value &lval = lval_.get();
> -        Value &rval = rval_.get();

Nice!

::: js/src/jsobj.cpp
@@ +765,2 @@
>      RootedId id(cx);
> +    if (!ValueToId(cx, argc != 0 ? args[0] : UndefinedValue(), id.address()))

Please use args.length().

@@ +824,2 @@
>      /* Step 1. */
> +    if (argc < 1 || !args[0].isObject()) {

args.length()

@@ +846,3 @@
>      /* Step 1. */
>      RootedId id(cx);
> +    if (!ValueToId(cx, argc != 0 ? args[0] : UndefinedValue(), id.address()))

args.length()

@@ +961,2 @@
>      RootedId id(cx);
> +    if (!ValueToId(cx, argc != 0 ? args[0] : UndefinedValue(), id.address()))

args.length()

@@ +997,2 @@
>      RootedId id(cx);
> +    if (!ValueToId(cx, argc != 0 ? args[0] : UndefinedValue(), id.address()))

args.length()

@@ +1950,3 @@
>      /* Steps 1 and 7. */
>      RootedObject obj(cx);
>      if (!GetFirstArgumentAsObject(cx, argc, vp, "Object.defineProperties", &obj))

args.length()

@@ +1955,3 @@
>  
>      /* Step 2. */
>      if (argc < 2) {

args.length()

::: js/src/jsstr.cpp
@@ +1021,5 @@
>  
>  static const size_t sRopeMatchThresholdRatioLog2 = 5;
>  
>  /*
> + * RopeMatch takes the text to search, the pattern to search for in the text.

s/to search, the pattern/to search and the pattern/

@@ +1147,5 @@
>      const jschar *text = str->getChars(cx);
>      if (!text)
>          return false;
>  
> +    SkipRoot skip(cx, &text);

This skip root is not correct in the case where we are actually moving objects.  Can you add a comment reminding us to revisit this?

@@ +1326,5 @@
>      const jschar *text = str->getChars(cx);
>      if (!text)
>          return false;
>  
> +    SkipRoot skip(cx, &text);

Same here.

@@ +1371,5 @@
>      const jschar *text = str->getChars(cx);
>      if (!text)
>          return false;
>  
> +    SkipRoot skip(cx, &text);

And here.

::: js/src/jstypedarray.cpp
@@ +1676,5 @@
>                  InvokeArgsGuard ag;
>                  if (!cx->stack.pushInvokeArgs(cx, 3, &ag))
>                      return NULL;
>  
> +                ag.setCallee(cx->compartment->maybeGlobal()->createArrayFromBuffer<NativeType>());

I think splinter is having trouble displaying this line.  Is it below 100 chars?

::: js/src/methodjit/PolyIC.cpp
@@ +1952,5 @@
>          } else {
>              LookupStatus status = cc.generateStringPropertyStub();
>              if (status == Lookup_Error)
>                  THROW();
> +            JSObject *obj = ToObjectFromStack(f.cx, objval);

This looks like it would be a good candidate for the RawObject type.

@@ +2006,5 @@
>  
>      RecompilationMonitor monitor(f.cx);
>  
> +    RootedValue objval(f.cx, f.regs.sp[-2]);
> +    JSObject *obj = ToObjectFromStack(f.cx, objval);

RawObject

::: js/src/vm/Debugger.cpp
@@ +1975,5 @@
>          debugEnum->removeFront();
>      else
>          debuggees.remove(global);
> +    if (v->empty())
> +        global->compartment()->removeDebuggee(fop, global, compartmentEnum);

This is a pretty subtle change; it would be nice if we could document this with types somehow.  I don't have any context around this location: would it be too ugly to put an AssertRootingUnnecessary guard around the bit we're moving this behind?

Comment 37

[Brian Hackett [Laid off!]]

https://hg.mozilla.org/integration/mozilla-inbound/rev/454e12a2460f

Comment 38

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/454e12a2460f

Comment 39

[Terrence Cole [:terrence]]

We're tracking this per-test-suite now.