Продолжение: обидно за мнения про статические анализаторы кода
Планировалось, что, написав статью «Обидно за мнения про статические анализаторы кода», мы выговоримся и спокойно отпустим тему. Но неожиданно эта статья вызвала бурный отклик. К сожалению, обсуждение пошло не туда, и сейчас мы сделаем вторую попытку объяснить своё видение ситуации.
Анекдот-аналогия
Итак, всё началось со статьи «Обидно за мнения про статические анализаторы кода». Её начали активно обсуждать на некоторых ресурсах и это обсуждение очень напоминает следующий старый анекдот.
Купили как-то суровым сибирским лесорубам японскую бензопилу.
Собрались в кружок лесорубы, решили её испытать.
Завели её, подсунули ей деревце.
«Вжик» — сказала японская пила.
«У, бля…» — сказали лесорубы.
Подсунули ей деревце потолще. «Вж-ж-жик!» — сказала пила.
«Ух, бля!» — сказали лесорубы.
Подсунули ей толстенный кедр. «ВЖ-Ж-Ж-Ж-Ж-Ж-Ж-ЖИК!!!» — сказала пила.
«Ух ты, бля!» — сказали лесорубы.
Подсунули ей железный лом. «КРЯК!» — сказала пила.
«Ага, бля!!!» — укоризненно сказали суровые сибирские лесорубы! И ушли рубить лес топорами…
История один в один. Люди, посмотрели на код:
if (A[0] == 0)
{
X = Y;
if (A[0] == 0)
....
}
И начали придумывать ситуации, когда он может быть оправданным, и значит, предупреждение анализатора PVS-Studio является ложно-позитивным. В ход пошли рассуждения про изменение памяти между двумя проверками, возникающее из-за:
- работы параллельных потоков;
- обработчиков сигналов/прерываний;
- переменная X является ссылкой на элемент A[0];
- аппаратного обеспечения, например, выполнения DMA-операциям;
- и так далее.
И обсудив, что не все ситуации анализатор может понять, ушли рубить лес топорами. То есть нашли оправдание, почему можно и дальше не использовать статический анализатор кода в своей работе.
Наше видение ситуации
Такой подход контрпродуктивен. Неидеальный инструмент вполне может быть полезен, а его использование экономически целесообразным.
Да, любой статический анализатор выдаёт ложно-позитивные срабатывания. И с этим ничего нельзя поделать. Однако, эта беда сильно преувеличивается. На практике статические анализаторы можно настроить и использовать различные способы подавления и работы с ложными срабатываниями (см. 1, 2, 3, 4). Плюс здесь уместно вспомнить про статью «False positives are our enemies, but may still be your friends».
Впрочем, даже это не главное. Частные случаи экзотического кода нет смысла вообще рассматривать! Можно сложным кодом запутать анализатор? Да, можно. Однако, на один такой случай, будет приходиться сотни полезных срабатываний анализатора. Можно найти и исправить множество ошибок на самом раннем этапе. А одно-два ложных срабатываний можно спокойно подавить и больше не обращать на них внимание.
И вновь PVS-Studio прав
Здесь статью можно было-бы и закончить. Однако, некоторые могут посчитать предыдущий раздел не рациональными соображениями, а попытками скрыть слабости и недостатки инструмента PVS-Studio. Поэтому придётся продолжить.
Рассмотрим конкретный компилируемый код, включающий объявление переменных:
void SetSynchronizeVar(int *);
int foo()
{
int flag = 0;
SetSynchronizeVar(&flag);
int X, Y = 1;
if (flag == 0)
{
X = Y;
if (flag == 0)
return 1;
}
return 2;
}
Анализатор PVS-Studio обоснованно выдаёт предупреждение: V547 Expression 'flag == 0' is always true.
И анализатор совершенно прав. Если кто-то начнёт разглагольствовать, что переменная может поменяться в другом потоке, в обработчике сигнала и так далее, то он просто не понимает язык C и C++. Так писать нельзя.
Компилятор в целях оптимизации вправе выбросить вторую проверку и будет абсолютно прав. С точки зрения языка, переменная измениться не может. Её фоновое изменение- это не что иное, как Undefined behavior.
Чтобы проверка осталась на месте, переменная должна быть объявлена как volatile:
void SetSynchronizeVar(volatile int *);
int foo()
{
volatile int flag = 0;
SetSynchronizeVar(&flag);
....
}
Анализатор PVS-Studio знает про это и уже не выдаёт предупреждение на такой код.
Здесь мы возвращаемся к тому, про что шла речь в первой статье. Проблемы никакой и нет. Но есть критика или непонимание, почему анализатор вправе выдавать предупреждение.
Примечание для самых дотошных читателей
Кто-то из читателей может вернуться к синтетическому примеру из первой статьи:
char get();
int foo(char *p, bool arg)
{
if (p[1] == 1)
{
if (arg)
p[0] = get();
if (p[1] == 1) // Warning
return 1;
}
// ....
return 3;
}
И добавить volatile:
char get();
int foo(volatile char *p, bool arg)
{
if (p[1] == 1)
{
if (arg)
p[0] = get();
if (p[1] == 1) // Warning :-(
return 1;
}
// ....
return 3;
}
После чего, справедливо заметить, что анализатор по-прежнему выдаёт предупреждение V547 Expression 'p[1] == 1' is always true.
Ура, наконец показано, что анализатор всё-таки бывает неправ :). Это ложное срабатывание!
Как видите, мы не скрываем какие-то недоработки. При анализе потока данных для элементов массива этот злосчастный volatile потерялся. Недоработка уже найдена и исправлена. Исправление будет доступно в следующей версии анализатора. Ложного срабатывания не будет.
А почему же этот баг не был выявлен ранее? Потому что на самом деле это опять нереальный код, который не встречается в настоящих проектах. Собственно, до сих пор подобный код мы и не встретили, хотя проверили множество открытых проектов.
Почему код нереален? Во-первых, на практике между двумя проверками будет какая-то функция синхронизации или задержки. Во-вторых, никто в здравом уме без крайней необходимости не создаёт массивы, состоящие из volatile-элементов. Работа с таким массивом- это колоссальное падение производительности.
Подытожим. Можно легко создать примеры, где анализатор ошибается. Но с практической точки зрения выявляемые недоработки практически на сказываются на качестве анализа кода и количестве выявленных настоящих ошибок. Ведь код реальных приложений- это просто код понятный одновременно анализатору и человеку, а не ребус или пазл. Если код — это пазл, то тут уж не до анализаторов :).
Спасибо за внимание.
Дополнительные ссылки
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Part 2: Upsetting Opinions about Static Analyzers.
