Closed Bug 963280 Opened 6 years ago Closed 6 years ago

pass --smc-check=all-none-file when using valgrind as a debugger

Categories

(Testing :: General, defect)

defect
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla29

People

(Reporter: froydnj, Assigned: froydnj)

Details

Attachments

(1 file)

No description provided.
Explanation in the newly added comment.
Attachment #8364645 - Flags: review?(jmaher)
Comment on attachment 8364645 [details] [diff] [review]
pass --smc-check=all-none-file when using valgrind as a debugger

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

Under what circumstances do these options get used? --smc-check=all-none-file is a great addition, but it's very likely that anyone using Valgrind seriously will also need --vex-iropt-register-updates=allregs-at-mem-access (to avoid more JIT-related crashes) and --show-possibly-lost=no (to avoid bazillions of spurious "possibly lost" messages).

I use all three of these flags, along with some other less critical ones, in build/valgrind/mach_commands.py.

r=me if you want to add them too. (I figure stealing review is ok, since Valgrind expertise is more relevant for this patch than testing expertise.)
Attachment #8364645 - Flags: review?(jmaher) → review+
> Under what circumstances do these options get used?

I guess it's for cases like |mach mochitest-chrome --debugger=valgrind| if you don't specify --debugger-args?
thanks for picking up this review :njn!
(In reply to Nicholas Nethercote [:njn] from comment #3)
> > Under what circumstances do these options get used?
> 
> I guess it's for cases like |mach mochitest-chrome --debugger=valgrind| if
> you don't specify --debugger-args?

That's correct.

(In reply to Nicholas Nethercote [:njn] from comment #2)
> --smc-check=all-none-file is a great addition, but it's very likely that
> anyone using Valgrind seriously will also need
> --vex-iropt-register-updates=allregs-at-mem-access (to avoid more
> JIT-related crashes) and --show-possibly-lost=no (to avoid bazillions of
> spurious "possibly lost" messages).
> 
> I use all three of these flags, along with some other less critical ones, in
> build/valgrind/mach_commands.py.

I would like to add them, but it looks like --vex-iropt-register-updates=allregs-at-mem-access is newer (3.8?  3.9?).  --show-possibly-lost is at least in 3.7, the version that comes with Debian stable and Ubuntu's last LTS release.  I guess we can just assume that anybody running valgrind with the test harness ought to be running newer versions?
I looked at Valgrind's release notes...

* --show-possibly-lost=no was added in 3.6.0 (Oct 2010)
* --smc-check=all-non-file was added in 3.7.0 (Nov 2011)
* --vex-iropt-register-updates=allregs-at-mem-access was added in 3.8.0 (Aug 2012)

My experience with Valgrind-on-TBPL is that without --vex-iropt-register-updates=allregs-at-mem-access, the JITs crash moderately often in a mysterious fashion. Allowing an older version that's less reliable doesn't seem like a good idea.
Added the additional flags, with commentary:

https://hg.mozilla.org/integration/mozilla-inbound/rev/118d5c5d71bb
Assignee: nobody → nfroyd
Flags: in-testsuite-
"But there are a lot of objects that are semi-deliberately leaked, so we set '--show-possibly-lost=no' to avoid uninteresting output from those objects."

They're not leaked, but rather are held onto in a way that Valgrind can't tell for certain if it's a leak or not.  So we set --show-possibly-lost=no to avoid many false positive leak reports.
https://hg.mozilla.org/mozilla-central/rev/118d5c5d71bb
Status: NEW → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Target Milestone: --- → mozilla29
You need to log in before you can comment on or make changes to this bug.