Get rooting analysis builds green on tinderbox

RESOLVED FIXED in mozilla17

Status

()

RESOLVED FIXED
7 years ago
6 years ago

People

(Reporter: bhackett, Assigned: bhackett)

Tracking

unspecified
mozilla17
x86_64
Linux
Points:
---
Dependency tree / graph
Bug Flags:
in-testsuite -

Firefox Tracking Flags

(Not tracked)

Details

(Whiteboard: [leave open][js:t])

Attachments

(6 attachments, 2 obsolete attachments)

(Assignee)

Description

7 years ago
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.
(Assignee)

Comment 1

7 years ago
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.
Assignee: general → bhackett1024
Attachment #640465 - Flags: review?(wmccloskey)
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.
Attachment #640465 - Flags: review?(wmccloskey) → review+
Whiteboard: [leave open when merging] → [leave open]
(Assignee)

Comment 4

7 years ago
(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).
(Assignee)

Comment 6

7 years ago
More rooters to get jitflags -ma and -mna to work.
Attachment #641146 - Flags: review?(wmccloskey)
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.
Attachment #641146 - Flags: review?(wmccloskey) → review+
(Assignee)

Comment 9

7 years ago
The last patch left just four failures on the rooting analysis builds on tbpl.  This should fix all of them.
Attachment #641304 - Flags: review?(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.
Attachment #641304 - Flags: review?(terrence) → review+
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.
(Just to clarify, the just comment 8 landing backed out)
s/the just/just the/

(it's clearly going to be one of those days...)
WinXP-mostly, rather than WinXP-only - we got a Win7 failure in https://tbpl.mozilla.org/php/getParsedLog.php?id=13459348&tree=Mozilla-Inbound.
(Assignee)

Comment 15

7 years ago
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
Whiteboard: [leave open] → [leave open][js:t]
https://hg.mozilla.org/mozilla-central/rev/848ed9d56932
Flags: in-testsuite-
Target Milestone: --- → mozilla16
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
(Assignee)

Comment 18

7 years ago
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
(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.
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!
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 :-)
Attachment #642513 - Flags: review?(bhackett1024)
(Assignee)

Updated

7 years ago
Attachment #642513 - Flags: review?(bhackett1024) → review+
Didn't make it to mozilla-central before the uplift (merge was blocked on bug 774259). Adjusting milestone accordingly.
Target Milestone: mozilla16 → mozilla17
Will this get uploaded to aurora (Moz 16) at some point, because without it VS 2008 fails on aurora debug builds?
Duplicate of this bug: 767973
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);
Attachment #648551 - Flags: review?(bhackett1024)
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.
Attachment #648551 - Flags: review?(bhackett1024)
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....
Attachment #648551 - Flags: review?(bhackett1024)
Posted patch v1: simple cleanup. (obsolete) — Splinter Review
JSON::Walk is not hot enough to warrant saving one root.
Attachment #648551 - Attachment is obsolete: true
Attachment #648551 - Flags: review?(bhackett1024)
Attachment #648749 - Flags: review?(bhackett1024)
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.
Attachment #648749 - Attachment is obsolete: true
Attachment #648749 - Flags: review?(bhackett1024)
Attachment #648881 - Flags: review?(bhackett1024)
(Assignee)

Updated

7 years ago
Attachment #648881 - Flags: review?(bhackett1024) → review+
(Assignee)

Comment 35

7 years ago
Get jit-tests green again in the interpreter for me, fix some fuzz bugs decoder found, clean up various rubbish.
Attachment #650980 - Flags: review?(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?
Attachment #650980 - Flags: review?(terrence) → review+

Updated

7 years ago
Depends on: 783540
Depends on: 828607
We're tracking this per-test-suite now.
Status: NEW → RESOLVED
Last Resolved: 6 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.