Иван Андряшин

23 Июн 2015

Теги:

#Cpp

О том, как мы опробовали статический анализ на своем проекте учебного симулятора рентгенэндоваскулярной хирургии

23 Июн 2015

Автор: Иван Андряшин

Введение
Статический анализ кода в проекте
Чем все закончилось?
Отзывы

Хочу поделиться с вами историей о том, как мы опробовали статический анализатор кода PVS-Studio в своем проекте и попытались определить, какую выгоду можно извлечь из этого инструмента. В этой статье не будут описаны уникальные и интересные для специалистов программные ошибки. Все найденные в коде статическим анализом баги и недочеты оказались достаточно прозаичными. Я опишу здесь взгляд на данный инструмент с точки зрения руководителя проекта. Возможно, этот ракурс не так точен и однозначен, как оценка инженера: своё влияние оказывают особенности организации работы в конкретном проекте. Но все же я думаю, что приведенные в статье мысли могут оказаться интересны тем, кто задумывается об использовании статического анализа в работе. Или тем, кто сталкивается в своих проектах со значительными потерями ресурсов на фиксы ошибок, отлавливаемых на стадии тестирования.

0331_Eidos-Medicine-PVS-Studio_ru/image1.png

Статья первоначально была опубликована на сайте habrahabr.ru. Статья и её перевод размещаются в нашем блоге с соглашения автора.

Введение

Я работаю в компании "Эйдос-Медицина", которая специализируется в разработке виртуальных медицинских симуляторов. Это специальные программно-аппаратные комплексы, позволяющие имитировать выполнение различных хирургических вмешательств в учебном процессе. Использование симуляторов позволяет студентам-медикам и интернам получить первые практические навыки по специальности до того, как им доверят живого пациента. Наша проектная группа разрабатывает симулятор рентгенэндоваскулярных вмешательств. Эта сфера включает в себя достаточно много видов операций на кровеносные сосуды, проводимых под контролем рентгеноскопии: ангиопластика, стентирование, спиральная эмболизация аневризм, эндопротезирование аневризм аорты.

Наша команда работает над проектом в своем текущем составе в течении полутора лет. Все идет своим чередом. Хирурги-консультанты прорабатывают с аналитиком тактику хирургических вмешательств шаг за шагом, работают с требованиями к визуальной составляющей. 3D-художник, используя КТ-ангиографию, анатомические атласы и советы хирургов, создает модели для новых клинических случаев симулятора. Программисты верхнего уровня трудятся над визуализацией рентгеноскопии, физикой движения эндоваскулярных инструментов внутри артерий, логическим анализом действий курсанта на симуляторе для фиксации правильности выполнения этапов вмешательства. Схемотехники, программисты микроконтроллеров и инженеры-конструкторы обеспечивают работу различных имитаторов медицинского оборудования, используемых в симуляции, сбор данных с датчиков, предварительную их обработку и отправку в программу. В ответ, на верхнем уровне подготавливается для передачи в контроллер информация, на основании которой обеспечивается соответствующая ходу виртуального вмешательства индикация на аппаратуре и эффекты обратной тактильной связи, призванные минимизировать условность учебного процесса.

А когда работа проделана, скомпилирована, распаяна, обвязана, выфрезирована и собрана, ее результаты поступают к тестировщику. Тестирование у нас, в основном, ручное. Автоматических тестов минимум. В течении всего хода разработки новой версии тестировщик проверяет на своем компьютере актуальные ревизии программы на предмет производительности, стабильности и адекватности работы. Это нужно, чтобы вовремя отловить опасные коммиты, так как итерации на версию у нас достаточно длительные. Однако основное тестирование релиз-кандидата все же проходит уже на самом симуляторе. Там могут возникать свои сюрпризы. Кто-то кого-то неправильно понял на согласовании протокола общения с контроллером. Или динамика движения имитаторов инструментов при работе на симуляторе немного отличается от отладочного управления с клавиатуры, и это "немного" выливается в критические проблемы для физического движка. Или же в дистрибутив не доложили какие-то сторонние библиотеки, использованные в новой версии. Сюрпризов может быть много, но, конечно же, чемпионами по неприятности являются плавающие ошибки, выливающиеся в падение программы или в критические проблемы, не позволяющие курсанту выполнять упражнение на симуляторе в нормальном режиме.

0331_Eidos-Medicine-PVS-Studio_ru/image2.png

Впрочем, немало времени уходит и на довольно простые, легко вычисляемые баги. При добавлении новых функциональных возможностей нередко привносятся в программу и новые ошибки. Большинство из них отлавливается еще в ходе работы над версией, в процессе ежедневного регрессивного тестирования проекта. Обнаружив новую ошибку, тестировщик должен определить, кто из разработчиков за нее ответственен (что, кстати, не всегда бывает очевидно), и создать задачу в Redmine. После того, как программист разберется в проблеме и закоммитит фикс, нужно будет выполнять еще дополнительные проверки, подтверждающие, что задача действительно решена и ее можно закрывать. В сумме получается никак не меньше половины человеко-часа на самый тривиальный случай. Это если баг воспроизводится быстро, и программист сразу понимает, в чем проблема, и что нужно поправить в коде. Если же для воспроизведения ошибки требуется 20-30 минут, то она повлечет уже потерю двух человеко-часов даже в случае самого быстрого и тривиального фикса. Достаточно существенные потери. И тем обиднее, что причины таких ошибок часто кроются в обычной невнимательности.

Статический анализ кода в проекте

Идея попробовать использовать в проекте статический анализатор кода пришла ко мне не сама. Ее мне принес коллега с конференции "C++ Russia", где он и познакомился с ребятами из PVS-Studio. Я взял паузу на то, чтобы подумать и зарелизить текущую версию, а потом решил все-таки попробовать. Связался с разработчиками PVS-Studio по почте и после недолгой переписки получил лицензионный ключ сроком действия на две недели, и мы начали проверку нашего проекта.

Здесь нужно сказать пару слов об особенностях архитектуры проекта. Кода именно на C++ у нас не так много. Всего около пятидесяти библиотек, но некоторые из них содержат буквально несколько десятков строк кода. Значительная часть программной логики находится в среде графического движка. Код на С++ мы интегрируем в проект посредством DLL. Таким образом мы реализуем какую-то специфическую функциональность, которая не представлена в среде графического движка. Кроме того, мы выносим в DLL сложные или ресурсоемкие алгоритмы по динамическому ежекадровому изменению или созданию полигональных сеток для рендера эндоваскулярных катетеров и проводников, имитаций сердцебиения и дыхательных движений. На С++ пишется и логика упражнений симуляции хирургических вмешательств, обеспечивающая отслеживание выполнения этапов операции, и корректности действий курсанта. В итоге получается, что в проекте есть несколько совсем небольших библиотек на C++ и несколько средних, содержащих по 2-3 тысячи срок. Для той части программной логики, что находится в среде графического движка интересных инструментов статического анализа кода не имеется. Поэтому мы проверяли с помощью PVS-Studio лишь часть проекта.

PVS-Studio встала на моем компьютере легко и непринужденно и интегрировалась в Visual Studio 2013. Андрей Карпов из команды PVS-Studio скинул мне на почту ссылки на руководство пользователя и что-то вроде Quiq Start Guide, что было даже излишним, так как разобраться в интерфейсе и функциональности статического анализатора можно используя лишь интуицию и технологии научного тыка.

Спустя 15 минут, я уже проверял код DLL, ответственной за моделирование процесса растекания рентгеноконстрастного вещества по артериям. Эта библиотека содержит около 4 тысяч строк кода. Я был несколько удивлен, что анализатор не зафиксировал в Solution ни одной ошибки первого уровня. Впрочем, нужно отметить, что данное решение уже прошло десятки часов тестирования и работало в последнее время стабильно. Итак, на что здесь обращает наше внимание статический анализатор:

V550 An odd precise comparison: t != 0. It's probably better to use a comparison with defined 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);
}

Подобные ошибки повторяются достаточно часто в данной библиотеке. Не скажу, что это стало для меня неожиданностью. Уже ранее наталкивался на некорректную работу с числами с плавающей точкой в этом проекте. Однако систематически проверять исходники на этот счет не было ресурсов. По результатам проверки стало ясно, что нужно дать разработчику почитать что-то для расширения кругозора в части работы с числами с плавающей точкой. Скинул ему ссылки на пару хороших статей. Посмотрим на результат. Сложно однозначно сказать, вызывает ли эта ошибка реальные сбои в работе программы. Текущее решение выставляет ряд требований к исходной полигональной сетке артерий, по которым моделируется растекание рентгеноконтрастного вещества. Если требования не выполнены, то возможны падения программы или явно некорректная работа. Часть из этих требований получена аналитически, а часть эмпирически. Не исключено, что эта вторая часть требований растет как раз из некорректной работы с числами с плавающей точкой. Нужно отметить, что не все найденные случаи употребления точного сравнения чисел с плавающей точкой являлись ошибкой.

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;
    ....
  }
  ....
}

Порядка 20 подобных сообщений в Solution. Что интересно, в этой библиотеке очень большие требования к производительности. В функциях, работающих с векторами и матрицами, в свое время считали буквально каждое умножение и искали, где можно сэкономить. Цикл в данном примере кода проходит очень большое количество итераций: до десятков тысяч. Он входит в алгоритмы системы частиц, которая обеспечивает рендер ангиографии. Особенность визуализации рентгеноконтрастого вещества в картине рентгеноскопии заключается в том, что сосуды направленные перпендикулярно плоскости кадра, выглядят темнее. Рентгеновское излучение проходит в этом случае вдоль сосуда, то есть проходит сквозь больший слой поглощающей среды, больше ослабляется и меньше засвечивает пленку в проекции. Этот эффект в программе достигается за счет системы полупрозрачных частиц, распределяющихся внутри полигональной сетки артерий. Полигональные сетки очень детализированные. Частиц в системе, соответственно, тоже очень большое количество. Интересно будет провести эксперимент. Сможем ли мы выиграть миллисекунду-другую, исправив эти неэлегантные места в коде? Возможно, компилятор делает эту оптимизацию автоматически, но почему не попробовать дать ему подсказку.

0331_Eidos-Medicine-PVS-Studio_ru/image4.png

V669 Message: The 'cIT', 'j' arguments are non-constant references. The analyzer is unable to determine the position at which 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;
}

В данном случае код верный. Ошибка программиста только в некорректном описании параметров функции. Они должны были быть const int&.

Обнаружив на удивление мало критичных ошибок в первом испытуемом, мы перешли к более активно расширяемому в данный момент Solution. Состоит наш следующий испытуемый из восьми библиотек, которые обеспечивают передачу данных о происходящем в ходе виртуального вмешательства из графического движка в код логики упражнений на симуляцию хирургических вмешательств. На этих же библиотеках висит передача данных и в обратном направлении, например, для оповещения об ошибках курсанта или для сигнализирования о выполнении этапа вмешательства. За счет этого логику самих упражнений мы можем писать только на C++, не связываясь со средой графического движка.

0331_Eidos-Medicine-PVS-Studio_ru/image5.png

Здесь урожай был уже повнушительней, и мы действительно обнаружили пару весьма опасных мест в коде:

V595 Message: The '_idiChannel' pointer was utilized before it was verified against nullptr. Check lines: 917, 918. logicinterface.cpp 917

int instType =
      _idiChannel->GetActiveInstrumentTypeInGroup(instrumentId);

if (_alogChannel != NULL && _idiChannel != NULL) {
  ....
}

Место потенциального падения программы. Тестирование не выявило ранее эту ошибку из-за того, что до сих пор при работе приложения на данном этапе указатель _idiChannel оказывался не NULL. Но никто не гарантирует, что при дальнейшей разработке ситуация не поменяется, и этот ранее заложенный в проект баг проявит себя.

V688 The 'chCameraMatrix' local variable possesses the same name as one of the class members, which can result in a confusion. 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;
}

Четыре подобных срабатывания в Solution в разных файлах. В данном случае к ошибкам не приводило. Но в будущем могло бы привести к недопониманию и использованию неинициализированных указателей в коде.

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);
  ....
}

Тут, я так это себе представляю, разработчик написал сначала первые две строчки кода. Потом его чем-то отвлекли. Может быть даже чем-то очень важным. Вернувшись снова к работе над кодом, он уже добавил явную глупость. Такое бывает. А в коде тем временем появилось место потенциального падения программы.

Обнаружились опасные места в коде связанные с указателями и в других библиотеках:

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();
    ....
  } 
  ....
}

В этот раз ошибка чуть менее очевидная. Если stlAntiPowerSpheres оказалась NULL, то tabAntiPowerSpheres остается неинициализированной, и указывает на случайную область памяти. Проверка на NULL при этом будет пройдена, а дальше будет падение программы при обращении к полям объекта. Тестирование не выявило этот момент, вероятно, по тем причинам, что ранее везде в коде вызов (STL_Table *)GetChild(CH_ANTIPOWER_SPHERES) возвращал не NULL.

Напоследок я решил проверить статическим анализатором одно не тестированное решение, находящееся еще в разработке и не внедренное в основной проект. В рамках этого решения у нас идет работа над созданием собственного физического движка гибкой струны. Тут уже ошибок было побольше. Вот, например, забавный, экспонат:

V527 It is odd that the false value is assigned to '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;
  }
}

Тут можно отметить, что анализатор в данном случае прав лишь отчасти. Параметр outIsInScene вообще не должен был быть указателем. Но все равно спасибо за указание на это подозрительное место в коде. Действительно ошибка.

Все срабатывания здесь приводить не буду. Отмечу напоследок лишь две, которые обратили на себя внимание.

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));
}

Здесь, с одной стороны, обычная ошибка, возникшая от невнимательности программиста. Но, с другой стороны, такого рода ошибку было бы очень сложно отловить, если наблюдать непосредственно результат работы программы, а не проверять тестами работоспособность отдельных методов. В этой функции выполнялась проверка на коллинеарность двух векторов. Например, если вектор потенциального смещения точки упругой струны, пересекающего коллизионный объект, оказался, с определенными допущениями, коллинеарен нормали поверхности коллизионного объекта в месте пересечения, то это повлияет на то, каким образом будет высчитываться отскок. Но так как на физическую модель влияет множество взаимосвязанных факторов, наблюдая за работающей программой не всегда можно точно сказать, чем именно вызвано то или иное неадекватное поведение. И эту ошибку мы могли бы не замечать очень долго.

Была еще одно интересное срабатывание анализатора. Сначала я даже не понял, в чем суть ошибки, потому что анализатор заподозрил что-то не в самом коде, а в строковом литерале:

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";
....

Но оказалось, что анализатор ругается по делу и, действительно, там должен быть другой строковый литерал. Строка "out_Radius" в этом месте появилась в результате копипаста с фрагмента чуть выше. Потом в коде что-то заменили, но забыли поправить строковый литерал на уместный здесь "out_Count" .

Вот код, с которого скопипастили:

....
mCommandsDescriptions[currCommandNr].name =
  "Get Rope Fragment Radius(Rope; in_FragmentNr;out_Radius)";
....
mCommandsDescriptions[currCommandNr].
  params[PARAM_NR_FRAGMENT_RADIUS].name = "out_Radius";
....

Чем все закончилось?

Понятно, что такая разовая проверка проекта не принесет много пользы. Имеющийся код уже прошел достаточно длительное тестирование. Ошибок в нем не много, а многие из тех, что есть, не проявляют себя при нормальной работе программы. Будем ли мы теперь закупать лицензии PVS-Studio? Я положительно отношусь к внедрению такого инструмента в проект. Очевидно, использование статического анализа высвободило бы часть ресурсов тестировщика, да и разработчиков. В Redmine меньше бы было задач в трекере "Ошибка". Решенные задачи реже бы возвращались тестировщиками на доработку. Однако, до принятия итогового решения нужно оценить, какую именно коммерческую выгоду принесет нам использование PVS-Studio, и сопоставить это со стоимостью самого продукта. На оценку сильно влияет тот факт, что в нашем проекте относительно немного динамически развивающегося кода именно на С++. Пока что мы продолжаем работать без этого инструмента.

0331_Eidos-Medicine-PVS-Studio_ru/image7.png

Отзывы

Также я передал временный лицензионный ключ и разработчикам из других проектных групп компании "Эйдос-Медицина". Хотел, чтобы ребята попробовали и сделали выводы о том, нужен ли им такой инструмент в работе. Приведу в статье несколько отзывов:

Николай, программист из команды по разработке симулятора лапароскопической хирургии: "Неплохая вещь. Хорошо находил неинициализированные указатели и всякую опасную работу с ними."
Олег, программист из команды по разработке программного обеспечения для промышленных роботов: "Отличная штука. Но в старый проект тяжело впихивать. У нас вот ~9к предупреждений. Правда, там есть режим "игнорировать всё старое и смотреть только новые ошибки." (Большее количество предупреждений анализатора в этом проекте объясняется тем, что в нем уже не часть, а весь код пишется на С++. Да и масштаб программных разработок в этой проектной группе заметно больше.)
Роман, программист из команды по разработке программного обеспечения для промышленных роботов: "Полезная вещь, но пользоваться, думаю, нет смысла чаще, чем раз в месяц."

Андрей Карпов отреагировал на последний комментарий, и попросил привести его ответ в статье:

"Это неэффективный сценарий использования, о чём мы стараемся предупредить в каждой статье. В двух словах - чем раньше ошибка найдена, тем лучше. Нет смысла искать опечатку, запустив отладчик, если её можно было найти сразу после компиляции с помощью статического анализа.

Если причина не использовать анализатор регулярно кроется в его медленной работе, то предлагаю познакомиться с советами по повышению скорости работы. Возможно они помогут. Если нет, то всегда можно организовать автоматические ночные проверки (мы подскажем как лучше всё организовать).

Если причина - слишком много предупреждений, то для начала можно убрать все предупреждения и работать только с новыми (как внедрить статический анализ в большой проект)."