Opened 7 years ago

Closed 7 years ago

Last modified 7 years ago

#2036 closed enhancement (fixed)

Integrate valgrind into our test suite

Reported by: muks Owned by: muks
Priority: medium Milestone: Sprint-20120619
Component: build system Version:
Keywords: Cc:
CVSS Scoring: Parent Tickets:
Sensitive: no Defect Severity: N/A
Sub-Project: DNS Feature Depending on Ticket:
Estimated Difficulty: 0 Add Hours to Ticket: 0
Total Hours: 0 Internal?: no

Description (last modified by muks)

This is a bug to integrate Valgrind into our test suite. Basic objectives are:

  • Supply --enable-valgrind to configure to run tests (make check) under valgrind
  • Use two classes of suppressions files:
    • src/valgrind-suppressions for permanent suppressions that we never want to re-consider again
    • src/valgrind-suppressions.revisit for temporary suppressions (such as issues in testcases) that are not a priority to fix, but we'd like to fix them sometime

We then get a single builder to configure with --enable-valgrind and mail us in case there are unsuppressed reports.

I am assigning it to myself as I have an implementation ready to go on a branch.

Subtickets

Change History (14)

comment:1 Changed 7 years ago by muks

  • Description modified (diff)

comment:2 Changed 7 years ago by muks

  • Owner changed from muks to UnAssigned
  • Status changed from new to reviewing

Up for review. This is just an initial implementation and we can tweak it in the future for adding more cases. Valgrind is not called with --log-file as it's nice to see the errors in the make check output itself, and will get copied into the build status website without changes.

comment:3 Changed 7 years ago by jelte

  • Milestone changed from Next-Sprint-Proposed to Sprint-20120619

comment:4 Changed 7 years ago by muks

Whoever reviews this bug, note that the list of suppressions may not completely cover every case as it may even vary from system to system. So run make check a few times and if it fails for you, add to the suppressions list. Take care not to add obj: lines in suppressions as these are site-specific. Use "..." instead (see other suppressions in src/valgrind-suppressions.revisit for examples).

comment:5 Changed 7 years ago by muks

After several make check runs, there's a failure under Valgrind which could be timing related:

[ RUN      ] NameserverEntryTest.ExpirationTime
nameserver_entry_unittest.cc:224: Failure
Value of: curtime + rrv6_->getTTL().getValue()
  Actual: 1339574778
Expected: expiration
Which is: 1339574779
nameserver_entry_unittest.cc:233: Failure
Value of: curtime + minttl
  Actual: 1339574778
Expected: expiration
Which is: 1339574779
[  FAILED  ] NameserverEntryTest.ExpirationTime (141 ms)

comment:6 Changed 7 years ago by muks

The suppressions functionality has been split into another configure switch --enable-valgrind-suppressions. The rationale for this is:

  • Developers configure with --enable-valgrind and run make check to see all the error reports (make check continues testing even if there are Valgrind issues). This encourages us to fix these issues, especially if we are modifying those parts of the code as part of another bug.
  • Builders configure with --enable-valgrind --enable-valgrind-suppressions and run make check with suppressions. make check doesn't show existing suppressed issues and stops building if it finds a new Valgrind issue that is not suppressed.

comment:7 Changed 7 years ago by jelte

  • Owner changed from UnAssigned to jelte

comment:8 Changed 7 years ago by jelte

  • Owner changed from jelte to muks

I still see lots and lots of output with --enable-valgrind-suppressions btw. If you want i have a diff that blindly adds them to the .revisit file (more than 5000 lines of additional suppressions...).

there's also the 'problem' we discussed on jabber; The FDTest.read and FDTest.write tests fail, and if they are fixed with a memset, they still fail if run together with the FDShare test). I guess that fixing those isn't in scope for this ticket, but it certainly is not ready to be run automatically yet :) (at least on systems similar to mine). (testing now: suppressing the errors also works, but it is the test itself that is failing).

I did get confused about what the two configure options meant, reading interpretation error on my part though. One thing; make check did not stop the moment it found a suppression with --enable-valgrind-suppressions (which actually did help in blindly adding them all to .revisit). It error'd at the end, not at the first non-suppressed message.

But in itself I think this is a good step towards getting these things fixed, so I think we can go ahead and merge this.

comment:9 Changed 7 years ago by jelte

retracting that last statement, make check did stop (after adding suppressions there turned out to be more), but it did not stop on all of them.

comment:10 Changed 7 years ago by muks

  • Owner changed from muks to jelte

I've had to build that suppressions list in the repo over multiple runs of make check, and now it passes on my box. But if there's a 5000 line diff between this and your computer, then I guess these suppressions are very platform specific.

In this case, one approach would be to get a single Linux x86_64 buildbot to do the valgrind runs. We'll start with an empty suppressions list, and add suppressions that this build machine prints in its runs to the suppressions file. Over several runs, we should have covered all cases and that can be our valgrind check system. Developers just run without suppressions whenever they want to run the tests under valgrind.

Another approach is that we are able to see all reports on http://git.bind10.isc.org/~tester/builder/builder.html as the commits happen, and try to get the # of reports towards 0.

What do you think?

comment:11 Changed 7 years ago by jelte

  • Owner changed from jelte to muks

I think the best approach for now would be to focus on one system, put in suppressions for that one, and letting it error on any others.

Then get hacking on that list :) (ack, from the looks of things, they are very compiler/library-dependent so we're probably just getting similar lists).

comment:12 Changed 7 years ago by muks

OK, so I'm going to merge this as it needs to get to a buildbot before we populate the suppressions file from the issues generated by the build machine. I have emptied the suppressions file for now.

comment:13 follow-up: Changed 7 years ago by muks

  • Resolution set to fixed
  • Status changed from reviewing to closed

Merged into master in commit 0557f306a4d0accdd5b6510466749f99073a78f5:

* f57a34b [2036] Empty the suppressions list for now
* 80d9f83 [2036] Track origins of uninitialized memory when not running on a build machine
* 3039e3c [2036] Introduce --enable-valgrind-suppressions configure switch
* 61814fe [2036] Show even deeper stacks
* 9e0c33a Revert "[2036] Show even deeper stacks"
* 05cfa4f [2036] Show even deeper stacks
* 4b56c81 [2036] Add a note about not using obj: lines in suppressions
* a7d656e [2036] Add suppressions for all existing issues reported by Valgrind
* 3317bd6 [2036] Return non-zero exit status when valgrind finds an issue
* d45ad9c Add comment about demangling symbols in the suppressions
* 818dd92 [2036] Turn off fd-related reports for now
* a66e8c4 [2036] Make valgrind run quietly so it looks like a regular make check
* 090ccd3 [2036] Show valgrind use status in configure output
* c490648 [2036] Show deeper stack
* 0bb55ba [2036] Integrate valgrind into our test suite

Resolving as fixed. Thank you for reviews Jelte.

For the make check-valgrind support instead of the configure switch, I tried this with a different project and got it working; will commit support for it soon.

comment:14 in reply to: ↑ 13 Changed 7 years ago by muks

Replying to muks:

For the make check-valgrind support instead of the configure switch, I tried this with a different project and got it working; will commit support for it soon.

For this, the following commit was pushed:

* 5cbaf8b [master] Use make targets for Valgrind instead of configure switches

It was not reviewed. Jelte is on holiday, and the rest of the work is in master. The Valgrind commands are the same -- they have just been moved into Makefile.am from configure.ac. I have make check'ed both with and without suppressions to test that it works.

Note: See TracTickets for help on using tickets.