Geekly Articles each Day
📜 ⬆️ ⬇️

How we tested a static analysis on our project of a training simulator of endovascular surgery


I want to share with you a story about how we tried the static code analyzer PVS-Studio in our project and tried to determine what benefits can be derived from this tool. This article will not describe software errors that are unique and interesting for specialists. All the bugs and shortcomings found in the code by static analysis turned out to be quite prosaic. I will describe here a look at this tool from the point of view of the project manager. Perhaps this perspective is not as accurate and unambiguous as the assessment of the engineer: the features of the organization of work in a particular project have an impact. But still, I think that the thoughts presented in the article may be of interest to those who are thinking about using static analysis in their work. Or those who encounter in their projects with significant losses of resources on the fixes of errors caught at the testing stage.


Introduction


I work in the company "Eidos-Medicine", which specializes in the development of virtual medical simulators. These are special hardware and software systems that allow you to simulate the implementation of various surgical interventions in the educational process. The use of simulators allows medical students and interns to acquire the first practical skills in their specialty before they are entrusted to a living patient. Our project team is developing a simulator of endovascular interventions. This area includes quite a few types of operations on blood vessels performed under the control of fluoroscopy: angioplasty, stenting, spiral aneurysm embolization, endoprosthesis replacement of aortic aneurysms.

Our team has been working on the project in its current roster for a year and a half. Everything goes on as usual. Surgeons consultants work with the analyst tactics of surgical interventions step by step, working with the requirements for the visual component. A 3D artist, using CT angiography, anatomical atlases and surgeon's advice, creates models for new clinical cases of the simulator. Top-level programmers are working on the visualization of fluoroscopy, the physics of the movement of endovascular instruments inside the arteries, a logical analysis of the cadet's actions on the simulator for fixing the correctness of the implementation of the intervention steps. Circuitry engineers, microcontroller programmers and design engineers ensure the operation of various simulators of medical equipment used in the simulation, data collection from sensors, their preliminary processing and sending to the program. In response, at the top level, information is prepared for transmission to the controller, on the basis of which an indication on equipment and effects of tactile feedback that are appropriate for the course of virtual intervention are provided, designed to minimize the conventionality of the learning process.
')
And when the work is done, compiled, soldered, tied, cut and assembled, its results go to the tester. Our testing is mainly manual. Automatic tests minimum. During the entire course of development of the new version, the tester checks on his computer the current revisions of the program for performance, stability and adequacy of work. This is necessary in order to catch dangerous commits in time, since the iterations on the version are quite long. However, the main testing of the release candidate still takes place on the simulator itself. There may be surprises. Someone misunderstood someone on the coordination of the protocol of communication with the controller. Or the dynamics of the movement of simulators of tools when working on the simulator is slightly different from the debug control from the keyboard, and this “a little” results in critical problems for the physics engine. Or the distribution kit did not report any third-party libraries used in the new version. There may be many surprises, but, of course, champions of trouble are floating errors, resulting in a program crash or critical problems that prevent the cadet from exercising on the simulator in normal mode.



However, a lot of time is spent on fairly simple, easily calculated bugs. When adding new functionality, new errors are often introduced into the program. Most of them are caught in the course of work on the version, in the process of daily regressive testing of the project. Having discovered a new error, the tester must determine which of the developers is responsible for it (which, by the way, is not always obvious), and create a task in Redmine. After the programmer understands the problem and zakommitit fix, you will need to perform additional checks to confirm that the problem is really solved and can be closed. In total, no less than half a man-hour is obtained for the most trivial case. This is if the bug is reproduced quickly, and the programmer immediately understands what the problem is and what needs to be fixed in the code. If it takes 20-30 minutes to reproduce the error, then it will entail the loss of two man-hours, even in the case of the fastest and most trivial fix. Enough substantial losses. And the more offensive is that the causes of such errors often lie in the usual carelessness.

Static code analysis in the project


The idea of ​​trying to use a static code analyzer in a project came to me not myself. She was brought to me by a colleague from the “C ++ Russia” conference, where he met the guys from PVS-Studio. I took a pause to think and release the current version, and then I decided to try. I contacted the developers of PVS-Studio by mail and after a short correspondence I received a license key with a validity period of two weeks, and we started checking our project.

Here you need to say a few words about the features of the project architecture. There are not so many code on C ++. There are only about fifty libraries, but some of them contain literally several dozen lines of code. A significant part of the program logic is in the environment of the graphics engine. We integrate the C ++ code into the project via a DLL. Thus, we are implementing some specific functionality that is not represented in the environment of the graphics engine. In addition, we bring into the DLL complex or resource-intensive algorithms for dynamically changing every frame or creating polygonal meshes for rendering endovascular catheters and conductors, imitations of heartbeat and respiratory movements. In C ++, the logic of the surgical intervention simulation exercises is written, which ensures the tracking of the steps of the operation and the correctness of the cadet's actions. As a result, it turns out that the project has several very small libraries in C ++ and several medium-sized ones containing 2-3 thousand terms each. For the part of the program logic that is in the environment of the graphics engine, there are no interesting tools for static code analysis. Therefore, we checked with PVS-Studio only a part of the project.

PVS-Studio got up on my computer easily and naturally and integrated into Visual Studio 2013. Andrei Karpov from the PVS-Studio team sent me email links to the user manual and something like a Quick Start Guide, which was even unnecessary, as it was to understand The interface and functionality of the static analyzer can be used only by intuition and technology of scientific tyk.

After 15 minutes, I already checked the code of the DLL responsible for modeling the process of spreading of the radioconstrictor substance through the arteries. This library contains about 4 thousand lines of code. I was somewhat surprised that the analyzer did not fix a single level 1 error in Solution. However, it should be noted that this solution has already passed dozens of hours of testing and has worked stably lately. So, what does the static analyzer pay attention to here?

V550 An odd precise comparison: t! = 0. This is a better precision: fabs (A - B)> Epsilon. objectextractpart.cpp 3401
D3DXVECTOR3 N = VectorMultiplication( VectorMultiplication(V-VP, VN), VN); float t = Qsqrt(Scalar(N, N)); if (t!=0) { N/=t; V = V - N * DistPointToSurface(V, VP, N); }

Such errors are repeated quite often in this library. I will not say that it came as a surprise to me. Already earlier I ran into incorrect work with floating point numbers in this project. However, there were no resources to systematically check the sources for this. As a result of the test, it became clear that you need to give the developer something to read to broaden the horizon in terms of working with floating point numbers. He threw him links to a couple of good articles. Let's look at the result. It is difficult to say for sure whether this error causes real malfunctions of the program. The current solution sets a number of requirements for the original polygonal grid of arteries, by which the spreading of the radiopaque substance is simulated. If the requirements are not met, the program may fall or obviously incorrect work. Some of these requirements are derived analytically, and some empirically. It is possible that this second part of the requirements is growing just from incorrect work with floating point numbers. It should be noted that not all found cases of using an exact comparison of floating-point numbers were an error.

V807 Decreased performance. Consider creating a reference to avoid using the 'Duct.TR [cIT]' expression repeatedly. objectextractpart.cpp 2689
 for (k = 0; k < Duct.LIsize; k++) { cIT = Duct.ListIT[k]; if(DuctMain.TR[cIT].inScreen &&(Duct.TR[cIT].PNum > OneDev512)) { tuv[0].y = Duct.TR[cIT].v0 * Duct.TR[cIT].PNum; .... } .... }

About 20 similar messages in Solution. Interestingly, this library has very high performance requirements. In the functions working with vectors and matrices, at one time literally every multiplication was considered and they were looking for where to save money. The loop in this code example goes through a very large number of iterations: up to tens of thousands. It is included in the particle system algorithms, which provides a render angiography. The feature of visualization of the radiopaque substance in the fluoroscopy picture is that the vessels directed perpendicular to the plane of the frame look darker. In this case, X-ray radiation passes along the vessel, that is, it passes through a larger layer of the absorbing medium, is more attenuated and less illuminates the film in the projection. This effect in the program is achieved due to the system of translucent particles distributed inside the polygonal grid of arteries. Polygonal mesh is very detailed. The particles in the system, respectively, are also a very large number. It will be interesting to conduct an experiment. Can we win a millisecond or two by correcting these inelegant places in the code? The compiler may do this optimization automatically, but why not try to give it a hint.



V669 Message: The 'cIT', 'j' arguments are non-constant references. The analyzer is unable to determine where this argument is being modified. It is possible that the function contains an error. objectextractpart.cpp 2406
 D3DXVECTOR3 ObjectExtractPart::GetD(D3Object& Duct, int& cIT, int& j){ return DuctMain.VP[DuctMain.TR[cIT].IP[2]].P + ( DuctMain.VP[DuctMain.TR[cIT].IP[0]].P - DuctMain.VP[DuctMain.TR[cIT].IP[2]].P + ( DuctMain.VP[DuctMain.TR[cIT].IP[1]].P - DuctMain.VP[DuctMain.TR[cIT].IP[0]].P ) * Duct.TR[cIT].tt[j].x ) * Duct.TR[cIT].tt[j].y + DuctMain.TR[cIT].CNR * Duct.TR[cIT].tt[j].z; }

In this case, the code is correct. Programmer error only in incorrect description of function parameters. They had to be const int &.

Having found surprisingly few critical errors in the first subject, we moved to a more actively expanding Solution at the moment. Our next subject consists of eight libraries that provide the transfer of data on what is happening during the virtual intervention from the graphics engine to the code of the logic of exercises for the simulation of surgical interventions. Data transfer in the same direction also hangs in the opposite direction, for example, for notifying a cadet’s errors or for signaling the implementation of the intervention stage. Due to this, the logic of the exercises themselves can only be written in C ++, without being connected to the environment of the graphics engine.



Here the harvest was already more impressive, and we really found a couple of very dangerous places in the code:

V595 Message: The _idiChannel 'pointer was used before it was verified against nullptr. Check lines: 917, 918. logicinterface.cpp 917
 int instType = _idiChannel->GetActiveInstrumentTypeInGroup(instrumentId); if (_alogChannel != NULL && _idiChannel != NULL) { .... }

The place of a potential program crash. Testing did not reveal this error earlier due to the fact that so far when the application was running at this stage, the _idiChannel pointer was not NULL. But no one guarantees that with further development the situation will not change, and this bug that was previously embedded in the project will manifest itself.

V688 'C chCamera Matrix local variable variable variable variable pos pos members conf angiographlog.cpp 323
 class ANGIOGRAPHLOG_API AngiographLog: public ILogic { .... Aco_Matrix* chCameraMatrix; Aco_Matrix* chProjectionMatrix; .... } D3DXMATRIX AngiographLog::GetCameraMatrix() { D3DXMATRIX res; Aco_Matrix* chCameraMatrix=(Aco_Matrix*)GetChild(CameraMatrix); if ( chCameraMatrix != NULL) { res = chCameraMatrix->GetMatrix(); } return res; }

Four similar triggers in Solution in different files. In this case, the error did not lead. But in the future it could lead to misunderstandings and the use of uninitialized pointers in code.

V522 Dereferencing of the null pointer 'chInstrumentSubLineLengthIn' might take place. instrumentdatainterface.cpp 239
 D3DXVECTOR3 InstrumentDataInterface::GetSubLineEndPos(....) { .... if(chInstrumentSubLineLengthIn != NULL) chInstrumentSubLineLengthIn->SetFloat(subLineLengthIn); else chInstrumentSubLineLengthIn->SetFloat(0.0F); .... }

Here, I imagine this, the developer first wrote the first two lines of code. Then he was distracted by something. Maybe even something very important. Returning again to the work on the code, he had already added obvious nonsense. This happens. And in the code, meanwhile, a place of potential program crashes appeared.

There were dangerous places in the code, due to incorrect work with pointers, and in other libraries:

V614 Potentially uninitialized pointer 'tabAntiPowerSpheres' used. getnewposbyheartbeat.cpp 175
 void GetNewPosByHeartBeat::_precalc() { .... STL_Table *stlAntiPowerSpheres; CSTL_Table *tabAntiPowerSpheres; stlAntiPowerSpheres = (STL_Table *)GetChild(....); if (stlAntiPowerSpheres != NULL) tabAntiPowerSpheres = stlAntiPowerSpheres->getSTL_Table(); if (tabAntiPowerSpheres != NULL) { int tableSize = tabAntiPowerSpheres->getRowCount(); .... } .... }

This time the error is a little less obvious. If stlAntiPowerSpheres was NULL, then tabAntiPowerSpheres remains uninitialized, and indicates a random memory region. The check for NULL will be passed, and then the program will crash when accessing the fields of the object. Testing did not reveal this point, probably, for the reasons that previously (STL_Table *) GetChild (CH_ANTIPOWER_SPHERES) did not return NULL everywhere in the code.

Finally, I decided to test with a static analyzer one untested solution that is still in development and not embedded in the main project. As part of this decision, we are working on creating our own physics engine of a flexible string. There were already more mistakes. For example, a funny exhibit:

V527 It is the odd value assigned to the 'bool' type pointer. Probably meant: * outIsInScene = false. rpscene.cpp 79
 bool rpScene::CheckIsRopeInScene(...., bool* outIsInScene) { if (mEngine == NULL) { outIsInScene = false; return false; } else { *outIsInScene = mEngine->CheckIsRopeInScene(ropeToCheck); return true; } }

It can be noted here that the analyzer in this case is only partially correct. The outIsInScene parameter should not have been a pointer at all. But anyway, thanks for pointing this suspicious place in the code. Really a mistake.

I will not give all the positives here. I will note, finally, only two that have attracted attention.

V501 There are identical sub-expressions '(fabs (crossVect.x)> 1.192092896e-07F)' to the left and to the right of the '||' operator. rpmath.h 103
 inline bool IsCollinearVectors(Vector3d vect1, Vector3d vect2) { Vector3d crossVect = Vector3dMultiply(vect1, vect2); //     ; return !((fabs(crossVect.x) > FLT_EPSILON) || (fabs(crossVect.y) > FLT_EPSILON) || (fabs(crossVect.x) > FLT_EPSILON)); }

Here, on the one hand, is a common mistake caused by the negligence of the programmer. But, on the other hand, it would be very difficult to catch such an error if you directly observe the result of the program’s work, and not test the performance of individual methods with tests. In this function, a check was performed for the collinearity of two vectors. For example, if the vector of potential displacement of a point of an elastic string intersecting a collision object turned out to be, with certain assumptions, collinear with the normal of the surface of the collision object at the intersection, then this will affect how the rebound will be calculated. But since the physical model is influenced by many interrelated factors, observing a running program it is not always possible to say exactly what caused this or that inappropriate behavior. And this error we could not notice for a very long time.

There was another interesting triggering of the analyzer. At first I didn’t even understand what the error was, because the analyzer suspected something not in the code itself, but in the string literal:

V691 Empirical analysis. It is possible that a typo is present inside the string literal: "out_Radius". The 'RADIUS' word is suspicious. rpropeinstancecommand.cpp 93
 .... mCommandsDescriptions[currCommandNr].name = "Get Rope Fragments Count(Rope;out_Count)"; .... mCommandsDescriptions[currCommandNr]. params[PARAM_NR_FRAGMENTS_COUNT].name = "out_Radius"; ....

But it turned out that the analyzer swears in the case and, indeed, there must be another string literal. The string "out_Radius" in this place appeared as a result of copy-paste from the fragment a little higher. Then something was replaced in the code, but they forgot to correct the string literal with the appropriate out_Count here.

Here is the code from which you copied:
 .... mCommandsDescriptions[currCommandNr].name = "Get Rope Fragment Radius(Rope; in_FragmentNr;out_Radius)"; .... mCommandsDescriptions[currCommandNr]. params[PARAM_NR_FRAGMENT_RADIUS].name = "out_Radius"; ....

How did it all end?


It is clear that such a one-time check of the project will not bring much benefit. The existing code has already been tested for a long time. There are not many errors in it, and many of those that do not manifest themselves during normal program operation. Will we now purchase PVS-Studio licenses? I have a positive attitude to the introduction of such a tool in the project. Obviously, the use of static analysis would free up some of the resources of the tester, and indeed the developers. Redmine would have fewer tasks in the “Error” tracker. Solved problems would be less likely to be returned by testers for revision. However, before making a final decision, it is necessary to assess what kind of commercial benefit the use of PVS-Studio will bring us, and compare this with the cost of the product itself. The assessment is strongly influenced by the fact that in our project there is relatively little dynamically developing code in C ++. So far, we continue to work without this tool.



Reviews


I also handed over the temporary license key to the developers from other project groups of Eidos-Medicine. I wanted the guys to try and draw conclusions about whether they need such a tool in their work. I will cite several comments in the article:

Andrei Karpov responded to the last comment, and asked for his answer in the article:

This is an inefficient use case, which we try to warn in each article. In a nutshell - the earlier the error is found, the better. It makes no sense to search for a typo by running the debugger, if it could be found immediately after compilation using static analysis.

If the reason not to use the analyzer regularly lies in its slow work, then I suggest to get acquainted with the tips to improve the speed of work. Maybe they will help. If not, then you can always organize automatic night checks (we will tell you how best to organize everything).

If the reason is too many warnings, then you can remove all warnings first and work only with new ones ( how to implement static analysis in a large project ). ”


The article used photographs of Adel Valeev enzo2u .

Source: https://habr.com/ru/post/259911/

More articles:


All Articles