793509 - Run Valgrind on tbpl machines with "--show-possibly-lost=no" and "--smc-check=all-non-file" instead of "--smc-check=all"

Status: VERIFIED FIXED

(Release Engineering :: General, defect)

Description

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

Valgrind tbpl build logs are truncated:

Output exceeded 52428800 bytes, remaining output has been truncated
program finished with exit code 1
elapsedTime=22965.008494

(see https://tbpl.mozilla.org/php/getParsedLog.php?id=15458553&tree=Firefox )

Can we increase the threshold?

Comment 1

[Nick Thomas [:nthomas] (UTC+12)]

The 50MB limit is set at the buildbot master level, and it looks like we can't raise it for individual jobs. It's there to protect the masters from the load of handling data when jobs end up in a loop that spam the log.

Perhaps we can turn the question around - is the 50MB of log we're getting now all useful ? With appropriate suppressions and or fixes would it be below 50MB ?

Comment 2

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

> Perhaps we can turn the question around - is the 50MB of log we're getting
> now all useful ? With appropriate suppressions and or fixes would it be
> below 50MB ?

The log is useful for me if it finds leaks that are "definitely lost". There are a lot of other leaks that are "possibly lost", meaning we could use "--show-possibly-lost=no" to suppress them. I don't really look at "possibly lost" leaks because I have no way to tell if they are false positives or not.

Julian, do you think it is worth running Valgrind on tbpl machines with "--show-possibly-lost=no"? Will we be losing results?

Comment 3

[Nicholas Nethercote [inactive]]

> do you think it is worth running Valgrind on tbpl machines with
> "--show-possibly-lost=no"?

Yes!  Ignore possible leaks.  A possible leak is when you have a pointer to the middle of a heap block instead of the start of a heap block -- in C, it's suspicious but possibly ok, depending on what the program is doing.  But I'm pretty sure it can happen legitimately in C++ code, and in an app as big and complex as Firefox I'll bet other parts of the code are being "clever" with their pointers like this.

Comment 4

[Kyle Huey (Exited; not receiving bugmail, old account, do not use)]

(In reply to Nicholas Nethercote [:njn] from comment #3)
> > do you think it is worth running Valgrind on tbpl machines with
> > "--show-possibly-lost=no"?
> 
> Yes!  Ignore possible leaks.  A possible leak is when you have a pointer to
> the middle of a heap block instead of the start of a heap block -- in C,
> it's suspicious but possibly ok, depending on what the program is doing. 
> But I'm pretty sure it can happen legitimately in C++ code, and in an app as
> big and complex as Firefox I'll bet other parts of the code are being
> "clever" with their pointers like this.

It happen easily in anything with multiple inheritance, such as an XPCOM object.

Comment 5

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

nthomas points out the commands are in:

http://hg.mozilla.org/build/tools/file/default/scripts/valgrind/valgrind.sh

and while we're doing it, we should probably switch to --smc-check=all-non-file as well.

Comment 6

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

Attachment: patch to add --show-possibly-lost=no and replace --smc-check=all with --smc-check=all-non-file

Comment 7

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

For --smc-check=all-non-file, which we use when fuzzing Spidermonkey with Valgrind as well, see:

http://valgrind.org/docs/manual/manual-core.html#manual-core.rareopts

Comment 8

[Nick Thomas [:nthomas] (UTC+12)]

Comment on attachment 663886 [details] [diff] [review]
patch to add --show-possibly-lost=no and replace --smc-check=all with --smc-check=all-non-file

*stamp*

Comment 9

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

Pushed:

http://hg.mozilla.org/build/tools/rev/77fe45302ac7

Comment 10

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

From tbpl Valgrind logs:

debugger_args='--error-exitcode=1 --smc-check=all-non-file --gen-suppressions=all --leak-check=full --num-callers=50 --show-possibly-lost=no'

-> FIXED, and VERIFIED for good measure.