Update Valgrind flags in jit_test.py

RESOLVED FIXED in mozilla20

Status

()

Core
JavaScript Engine
RESOLVED FIXED
5 years ago
5 years ago

People

(Reporter: gkw, Assigned: gkw)

Tracking

Trunk
mozilla20
Points:
---

Firefox Tracking Flags

(Not tracked)

Details

Attachments

(1 attachment, 1 obsolete attachment)

(Assignee)

Description

5 years ago
Created attachment 680549 [details] [diff] [review]
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.
Attachment #680549 - Flags: feedback?(terrence)
(Assignee)

Comment 1

5 years ago
> Created attachment 680549 [details] [diff] [review]
> WIP v0

(There were a couple of whitespace removals in this patch as well)
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.
Attachment #680549 - Flags: feedback+
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.
Attachment #680549 - Flags: feedback?(terrence)
(Assignee)

Comment 4

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

Comment 5

5 years ago
Created attachment 692499 [details] [diff] [review]
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.
Assignee: general → gary
Attachment #680549 - Attachment is obsolete: true
Status: NEW → ASSIGNED
Attachment #692499 - Flags: review?(n.nethercote)
Attachment #692499 - Flags: review?(n.nethercote) → review+
(Assignee)

Comment 6

5 years ago
Setting checkin-needed - inbound is closed for now.
Keywords: checkin-needed
(Assignee)

Comment 7

5 years ago
https://hg.mozilla.org/integration/mozilla-inbound/rev/7d37287856bd
Keywords: checkin-needed
(Assignee)

Comment 8

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

I filed bug 821965.
https://hg.mozilla.org/mozilla-central/rev/7d37287856bd
Status: ASSIGNED → RESOLVED
Last Resolved: 5 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla20
You need to log in before you can comment on or make changes to this bug.