Conditional flags for tests - set JS stack size for simulators.

Conditional flags for tests - set JS stack size for simulators.

There are separated JS and C stacks on simulators so for some stack
extensive tests (like mozilla/js1_5/extensions/regress-355497) might
cause a C stack overflow and that overflow is not caught by V8. It is
not an issue on real HW. Increasing the C stack also solves the problem
but we have already FLAG_sim_stack_size flag to control the JS stack
size.

This patch makes it possible to add flags to tests conditionally in
.status files.

TEST=mozilla/js1_5/extensions/regress-355497
BUG=v8:3152
LOG=N

Committed: https://chromium.googlesource.com/v8/v8/+/e100acb3a76f1c9118b82626c0503af28b25894e

[balazs.kilvady, 2014-11-19 13:30:53]

balazs.kilvady@imgtec.com changed reviewers:
+ akos.palfi@imgtec.com, danno@chromium.org, dusan.milosavljevic@imgtec.com,
gergely.kis@imgtec.com, jkummerow@chromium.org, machenbach@chromium.org,
paul.lind@imgtec.com, ulan@chromium.org

[Jakob Kummerow, 2014-11-19 13:57:15]

I'm not sure having this infrastructure is worth it, as opposed to just skipping
the test forever. But if you'd like to have it, I'm not opposed. I have some
comments, though.

https://codereview.chromium.org/735723006/diff/1/test/mozilla/mozilla.status
File test/mozilla/mozilla.status (right):

https://codereview.chromium.org/735723006/diff/1/test/mozilla/mozilla.status#...
test/mozilla/mozilla.status:897: ['arch == arm64 and simulator_run == True', {
We already have such a section below, please move the new expectation there.

https://codereview.chromium.org/735723006/diff/1/tools/testrunner/local/tests...
File tools/testrunner/local/testsuite.py (right):

https://codereview.chromium.org/735723006/diff/1/tools/testrunner/local/tests...
tools/testrunner/local/testsuite.py:139: for outc in t.outcomes:
nit: no reason to abbreviate, s/outc/outcome/.

https://codereview.chromium.org/735723006/diff/1/tools/testrunner/local/tests...
tools/testrunner/local/testsuite.py:140: assert type(outc) == str
Let's drop the assert (this code is fairly performance sensitive).

https://codereview.chromium.org/735723006/diff/1/tools/testrunner/local/tests...
tools/testrunner/local/testsuite.py:142: (tmp0, tmp1, flags) =
outc.partition(':')
Suggestion: simplify the body of the if-block to:

t.flags += outcome[7:].split()

Have you tested this patch with the entire "mozilla" test suite? I think you'd
run into
http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-argum...
for t.flags. Suggested fix: change the default value in TestCase's constructor
to "flags=None", and the initialization line to "self.flags = flags or []".

[paul.l..., 2014-11-19 15:20:50]

paul.lind@imgtec.com changed reviewers:
+ jacob.bramley@arm.com, rodolph.perfetta@arm.com

[paul.l..., 2014-11-19 15:20:51]

Adding Jacob & Rodolph: any objection to us adding arm64 here? This is the cause
of v8:3152.

[balazs.kilvady, 2014-11-19 16:30:09]

On 2014/11/19 13:57:15, Jakob wrote:
> Have you tested this patch with the entire "mozilla" test suite? I think you'd
> run into
>
http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-argum...
> for t.flags. Suggested fix: change the default value in TestCase's constructor
> to "flags=None", and the initialization line to "self.flags = flags or []".

Thank you for the fixes. I am not a python expert :) After fixing flags=None I
run mozilla
tests for arm 64 and mips64el archs and all the tests are passed.

[balazs.kilvady, 2014-11-19 16:30:41]

https://codereview.chromium.org/735723006/diff/1/test/mozilla/mozilla.status
File test/mozilla/mozilla.status (right):

https://codereview.chromium.org/735723006/diff/1/test/mozilla/mozilla.status#...
test/mozilla/mozilla.status:897: ['arch == arm64 and simulator_run == True', {
On 2014/11/19 13:57:15, Jakob wrote:
> We already have such a section below, please move the new expectation there.

Done.

https://codereview.chromium.org/735723006/diff/1/tools/testrunner/local/tests...
File tools/testrunner/local/testsuite.py (right):

https://codereview.chromium.org/735723006/diff/1/tools/testrunner/local/tests...
tools/testrunner/local/testsuite.py:139: for outc in t.outcomes:
On 2014/11/19 13:57:15, Jakob wrote:
> nit: no reason to abbreviate, s/outc/outcome/.

Done.

https://codereview.chromium.org/735723006/diff/1/tools/testrunner/local/tests...
tools/testrunner/local/testsuite.py:140: assert type(outc) == str
On 2014/11/19 13:57:15, Jakob wrote:
> Let's drop the assert (this code is fairly performance sensitive).

Done.

https://codereview.chromium.org/735723006/diff/1/tools/testrunner/local/tests...
tools/testrunner/local/testsuite.py:142: (tmp0, tmp1, flags) =
outc.partition(':')
On 2014/11/19 13:57:15, Jakob wrote:
> Suggestion: simplify the body of the if-block to:
> 
> t.flags += outcome[7:].split()
> 
> Have you tested this patch with the entire "mozilla" test suite? I think you'd
> run into
>
http://docs.python-guide.org/en/latest/writing/gotchas/#mutable-default-argum...
> for t.flags. Suggested fix: change the default value in TestCase's constructor
> to "flags=None", and the initialization line to "self.flags = flags or []".

Done.

[Jakob Kummerow, 2014-11-19 19:08:19]

lgtm

[Rodolph Perfetta, 2014-11-19 19:28:56]

On 2014/11/19 15:20:51, paul.l... wrote:
> Adding Jacob & Rodolph: any objection to us adding arm64 here? This is the
cause
> of v8:3152.

no objections.

[Michael Achenbach, 2014-11-20 08:38:31]

lgtm with a comment: You can also hit the CQ - then I'll do a refactoring of the
code afterwards.

https://codereview.chromium.org/735723006/diff/20001/tools/testrunner/local/t...
File tools/testrunner/local/testsuite.py (right):

https://codereview.chromium.org/735723006/diff/20001/tools/testrunner/local/t...
tools/testrunner/local/testsuite.py:139: for outcome in t.outcomes:
I'd rather put this code into a new method/loop right after the call side of
this method:
https://code.google.com/p/chromium/codesearch#chromium/src/v8/tools/run-tests...

This method should be refactored anyway - lets restrict its concern to filtering
and outcomes definition - not also flags.

The code piece above currently only applies to "if testname in self.rules" and
not to the condition about wildcards below. But the feature should also be
available for wildcard rules I guess?

[balazs.kilvady, 2014-11-20 09:19:34]

The CQ bit was checked by balazs.kilvady@imgtec.com

[commit-bot: I haz the power, 2014-11-20 09:20:40]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/735723006/20001

[commit-bot: I haz the power, 2014-11-20 09:47:00]

Committed patchset #2 (id:20001)