631841 - Move Valgrind suppression files into mozilla-central

Status: RESOLVED FIXED

(Release Engineering :: General, defect)

Description

[Phil Ringnalda (:philor)]

To get to the happy end-state where people either run Valgrind themselves, or run their patch through the tryserver, and add a suppression for new leaks they expose in broken system libraries, we need something easier to do and to test than "clone the releng tools repo, don't test that you actually copy-pasted correctly and it works because you don't have any way to tryserver it, and do whatever it takes to get a change to the tools repo pushed live, um, I don't actually know what that involves."

It's a little weird to have suppression files that are specific to the OS our build slaves use checked in to mozilla-central, but I think it's worth it to make it reasonable to land something that Valgrind isn't going to like seeing.

Comment 1

[Chris AtLee [:catlee]]

(In reply to comment #0)
> To get to the happy end-state where people either run Valgrind themselves, or
> run their patch through the tryserver, and add a suppression for new leaks they
> expose in broken system libraries, we need something easier to do and to test
> than "clone the releng tools repo, don't test that you actually copy-pasted
> correctly and it works because you don't have any way to tryserver it, and do
> whatever it takes to get a change to the tools repo pushed live, um, I don't
> actually know what that involves."
> 
> It's a little weird to have suppression files that are specific to the OS our
> build slaves use checked in to mozilla-central, but I think it's worth it to
> make it reasonable to land something that Valgrind isn't going to like seeing.

Yeah, this is precisely why I didn't put them in there. Also, only a few folks in RelEng can push changes to m-c...although I guess that gets me off the hook for changes!

If we do this, I'd like to think about other possibilities for build-farm information in the tree, e.g. mozconfigs (bug 558180)

Comment 2

[Phil Ringnalda (:philor)]

Attachment: move 'em

Comment 3

[Phil Ringnalda (:philor)]

Attachment: use 'em (probably)

With a little luck, this will actually work.

Comment 4

[Phil Ringnalda (:philor)]

If we need to pick one of two problems to fix, either "there's no way you can push to either try or mozilla-central with a patch to build/tools, plus there are hundreds of people with mozilla-central access and no idea how to drive a patch into build/tools" or "there are a couple of releng people who might add things to the Valgrind suppression files who don't have mozilla-central access," I'll choose the latter as a thing to fix. We've finally rewritten the access requirements to where we _can_ easily do what we _should_ do, so that problem is a non-problem: 1) find http://www.mozilla.org/hacking/committer/ 2) cc :philor on the bug.

Comment 5

[(not currently active) Ted Mielczarek]

Comment on attachment 510087 [details] [diff] [review]
move 'em

Can you do this without an extra makefile? You can leave the files in build/valgrind, but just install them from build/Makefile. Seems unnecessary. But r=me on whatever you want to do.

Comment 6

[Chris AtLee [:catlee]]

Comment on attachment 510088 [details] [diff] [review]
use 'em (probably)

Looks ok to me.

Comment 7

[Phil Ringnalda (:philor)]

Attachment: move 'em, reasonably

That extra makefile would have made more sense if I hadn't been writing a second patch at the same time, where I was going to greater lengths to *avoid* a makefile that just copied files around. Or if I was drunk.

http://hg.mozilla.org/mozilla-central/rev/935706ff9e55

Comment 8

[Phil Ringnalda (:philor)]

Comment on attachment 510088 [details] [diff] [review]
use 'em (probably)

Unfortunately, I'm in that class of people I was speaking of, who have absolutely no idea how to go about landing a patch to the tools repo.

Comment 9

[Chris AtLee [:catlee]]

Comment on attachment 510088 [details] [diff] [review]
use 'em (probably)

http://hg.mozilla.org/build/tools/rev/349420f0ccd2

Comment 10

[Phil Ringnalda (:philor)]

I screwed that up somehow, not sure how.

/builds/moz2_slave/valgrind-linux/objdir/config/nsinstall -R ../../src/build/valgrind/i686-redhat-linux-gnu.sup ../../src/build/valgrind/x86_64-redhat-linux-gnu.sup ../_valgrind

seems like it would be putting them in objdir/_valgrind/

but 
args: ['/usr/bin/valgrind', '--error-exitcode=1', '--smc-check=all', '--gen-suppressions=all', '--leak-check=full', '--suppressions=_valgrind/i686-redhat-linux-gnu.sup', '/builds/moz2_slave/valgrind-linux/objdir/dist/firefox/firefox-bin', '-no-remote', '-profile', '/builds/moz2_slave/valgrind-linux/objdir/_profile/pgo/pgoprofile/', 'http://localhost:8888/index.html']
...
==11173== Command: /builds/moz2_slave/valgrind-linux/objdir/dist/firefox/firefox-bin -no-remote -profile /builds/moz2_slave/valgrind-linux/objdir/_profile/pgo/pgoprofile/ http://localhost:8888/index.html
==11173==
==11173== FATAL: can't open suppressions file "_valgrind/i686-redhat-linux-gnu.sup"

says either I didn't stick them where I thought I did, or the current directory isn't the objdir like I thought it was, or Valgrind doesn't think my path should be relative to anything, dunno which.

Comment 11

[(not currently active) Ted Mielczarek]

Your revised patch put unnecessary tabs here:
http://hg.mozilla.org/mozilla-central/rev/935706ff9e55#l1.15

two-space indent after continuations please. (You can fix that whenever.)

Also, you're probably getting screwed by this chdir:
http://mxr.mozilla.org/mozilla-central/source/build/pgo/profileserver.py#58

I think you need some abspath-love. Maybe just sticking $PWD before the path in valgrind.sh ?

Comment 12

[Chris AtLee [:catlee]]

(In reply to comment #11)
> Your revised patch put unnecessary tabs here:
> http://hg.mozilla.org/mozilla-central/rev/935706ff9e55#l1.15
> 
> two-space indent after continuations please. (You can fix that whenever.)
> 
> Also, you're probably getting screwed by this chdir:
> http://mxr.mozilla.org/mozilla-central/source/build/pgo/profileserver.py#58
> 
> I think you need some abspath-love. Maybe just sticking $PWD before the path in
> valgrind.sh ?

I pushed a change that's already running that adds $PWD to the suppression file.