VTK/SoftwareQuality/Valgrind/Fall2011

Testing is critical to high quality software. There are a number of aspects to the VTK's software quality. One important aspect is valgrind. Valgrind detects a number of dynamic memory defects.

This experiment seeks to reduce VTK's valgrind defects to 0 and keep them at 0.

This experiment uses the DMAIC methodology of the Six Sigma management process to "Define", "Measure", "Analyze", "Improve" and "Control" valgrind defects in VTK.

The basic methodology (from Wikipedia) consists of the following five steps:

• Define process goals that are consistent with customer demands and VTK's strategy.
• Measure key aspects of the current process and collect relevant data.
• Analyze the data to verify cause-and-effect relationships. Determine what the relationships are, and attempt to ensure that all factors have been considered.
• Improve or optimize the process.
• Control to ensure that any deviations from target are corrected before they result in defects. Set up pilot runs to establish software quality, move on to production, set up control mechanisms and continuously monitor the process.

Define

Keep the number of VTK valgrind defects to 0. When the defects stay above 0, developers find it difficult to see if their changes have increased the number of defects.

Measure

As of November 16, 2011, there were 175 valgrind defects.

• 22 tests had defects
  • 12 tests had memory leaks
  • 6 tests had uninitialized memory conditionals
  • 4 tests had uninitialized memory reads
  • 2 test had potential memory leaks
  • 1 test has mismatched memory/deallocate

Analyze

VTK provides dynamic memory test results (via valgrind) on the daily dashboard. Even though it is presented daily, valgrind defects are for the most part not detected when they are introduced. Rather, they seem to appear and linger. For example, one defect may have been present for over 6 years! Once the number of reported defects exceeds 0, it takes extra work for the developer to see if new defects have been introduced.

In this experiment, most of the defects were either memory leaks or accessing uninitialized memory. Several leaks were present in tests that used vtkTestUtilities::ExpandDataFileName. This utility function returns a char *. Testers forgot to delete the returned pointer. A better implementation would return an std::string. The minor defects were unlikely to cause problems in the future.

However, there were several major defects that exhibited bugs in the code. Although these codes did not have failing tests, it is apparent that they may have been producing incorrect results or might crash in the future.

Improve

• Minor defects
  • TestContextImage Test was not releasing the char * returned by vtkTestUtilities::ExpandDataFileName().
  • TestGlobeSource Test was not releasing the char * returned by vtkTestUtilities::ExpandDataFileName().
  • TestKCoreLayout Test was not releasing the char * returned by vtkTestUtilities::ExpandDataFileName().
  • TestImageDataLIC* Tests were not releasing the char * returned by vtkTestUtilities::ExpandDataFileName().
  • TestQuadRotationalExtrusionMultiBlock Test was not releasing the two char * returned by vtkTestUtilities::ExpandDataFileName().
  • PCAStatistics In both vtkPCAStatistics and TestPCAStatistics, a delete was used rather than a delete[].
  • InteractorEventRecorder If vtkInteractorEventRecorder::Play() was called more than once, the vtksys_ios::istringstream was leaked.
  • Coordinate When a viewport was not specified, vtkCoordinate::GetComputedDoubleDisplayValue() was not setting ComputedDoubleDisplayValue. This patch sets the value to VTK_LARGE_INTEGER. Valgrind reported these defects for: vtkCaptionRepresentationTest1 and vtkTextRepresentationTest1.
  • vtkLinearExtractor Get methods should not be used in constructors. The Set method compares the new value with the old value. Since the old value is not initialized it generates a valgrind defect.
  • TestHyperOctreeContourFilter The 1D test needed mapper1d->SetScalarModeToUseCellData();
• Major defects
  • MeanValueCoordinatesInterpolator and Polyhedron vtkPolyhedron::IntersectWithLine did not initialize tMin. In both Polyhedron and MeanValueCoordinatesInterpolator, the operator++ was accessing memory out of range when the end of the container was reached.
  • OpenGLProjectedTetrahedraMapper this->Internals was allocated in the constructor, but not freed in the destructor. valgrind detected leaks in two tests:ProjectedTetrahedraZoomIn and TestProjectedHexahedra.
  • OpenGLExtensionManager OpenGLExtensionManager was replacing this->ExtensionsString without freeing the current contents. Valgrind detected the leaks in two tests: TestMultiTexturing and TestMultiTexturingTransform.
  • vtkControlPointsItem At least one compiler warned that a subscript was out of range. In vtkControlPointsItem::GetCenterOfMass, point[4] was being accessed. Looks like a cut and paste error.
  • ReebGraphSurfaceSkeletonFilter ReebGraphVolumeSkeletonFilter Two logic errors caused valgrind to report "Conditional jump or move depends on uninitialised value(s)" for TestReebGraph. In vtkReebGraphSurfaceSkeletonFilter::RequestData() also vtkReebGraphVolumeSkeletonFilter::RequestData() 1) Storing the wrong point id in meshToSubMeshmap. This resulted in bad vertexIds in the subMesh. 2) The logic for the contourFilter setup was incorrect. contourFilter->SetNumberOfContours(1); but contourFilter->SetValue(i, ...)
  • vtkUnstructuredGridVolumeZSweepMapper This valgrind defect has been present for years. There are instances where vtkDoubleScreenEdge::Case is not defined before RasterizeTriangle. The logic in vtkSimpleScreenEdge::Init is complex. Perhaps in the future, a more more extensive examination will uncover the real problem. For now, the patch fixes the valgrind defect and still passes the regression test.

Control

Unfortunately, there is no automatic mechanism to control the valgrind defects. This puts the burden on developers to monitor the dashboard each day. Developers who check in code should look at the valgrind defect summary the morning after their check ins.

If we can get the defect count to 0, it will be easier to detect when new defects are introduced.