810767 - Update Valgrind flags in jit_test.py

Status: RESOLVED FIXED

(Core :: JavaScript Engine, defect)

Description

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: WIP v0

Valgrind flags in js/src/jit_test.py need updating. In particular, we should use --smc-check=all-non-file instead of --smc-check=all, and adding --gen-suppressions=all and --show-possibly-lost=no would be nice.

Some questions remain:

1. Is DEBUGGER_INFO at the top of jit_tests.py used? There are some Valgrind options present here that I'm not sure if I should change.

2. I'd like to add a default --suppressions= argument to point to e.g. build/valgrind/cross-architecture.sup Or should I point it to _valgrind (the one that appears after Make finishes processing)? How should it be done?

3. What should be the default exit code be, when Valgrind encounters an error? 1 or another number, e.g. 77?

These options were previously last touched in Sep 2009 by Graydon Hoare in https://hg.mozilla.org/mozilla-central/rev/6eda292e83a7 but I don't think he's working on JS anymore.

Comment 1

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> Created attachment 680549 [details] [diff] [review]
> WIP v0

(There were a couple of whitespace removals in this patch as well)

Comment 2

[Nicholas Nethercote [inactive]]

Comment on attachment 680549 [details] [diff] [review]
WIP v0

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

The patch looks fine to me.  DEBUGGER_INFO does appear to be dead, though I'm not certain... it looks like it was copied from build/automationutils.py.  I don't know about the suppression file or the exit code.

Comment 3

[Terrence Cole [:terrence]]

Comment on attachment 680549 [details] [diff] [review]
WIP v0

I'm not a peer on the jit-test code or on valgrind, so I'm just going to let Nick have this one.

Comment 4

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> 1. Is DEBUGGER_INFO at the top of jit_tests.py used? There are some Valgrind
> options present here that I'm not sure if I should change.

This doesn't seem used - automationutils.py doesn't seem to be connected to jit_test.py as discussed on IRC. I'll remove this in the next patch.

Comment 5

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Attachment: v1

(1) Removed DEBUGGER_INFO.
(2) Let's consider adding default suppression parameters in another bug.
(3) Retained the same error exit code 1 - since it's also used for Valgrind TBPL builds at https://hg.mozilla.org/build/tools/file/default/scripts/valgrind/valgrind.sh

Let's fix up the flags first by landing this patch. Requesting review from njn for real this time.

Comment 6

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

Setting checkin-needed - inbound is closed for now.

Comment 7

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

https://hg.mozilla.org/integration/mozilla-inbound/rev/7d37287856bd

Comment 8

[Gary Kwong [:gkw] [:nth10sd] (NOT official MoCo now)]

> (2) Let's consider adding default suppression parameters in another bug.

I filed bug 821965.

Comment 9

[Ryan VanderMeulen [:RyanVM]]

https://hg.mozilla.org/mozilla-central/rev/7d37287856bd