Про статический анализ С++

Тут меня навели на Coverity SCAN, которую дают помацать опенсорсным проектам на халяву.

Ну я посабмитил туда LibRaw, сегодня пустили, результаты оказались удивительными:

107 предупреждений, из них где-то треть - с High Impact.

Из High Impact:

  • Штук 10 в Microsoft STL (я самбитил виндовую сборку)
  • Еще какое-то количество такого примерно вида:
     int variable;
     if(layout==Layout1) variable=value1;
     if(layout==Layout2) variable=value2;
    И на это дается предупреждение, дескать неаккуратненько, не инициализированная переменная. Я с ним согласен по общим ощущениям, так не надо делать. Но в реальной жизни бывает два вида layout - и это явно прописано в вызывающем коде. Т.е. у машинки достаточно данных, чтобы сообразить, что это не 'High impact', а просто неаккуратненько.
  • Некоторое количество предупреждений, дескать unsigned short при расширении до 32-64 бит может больно укусить. С этим я не хочу спорить - формально машинка права, а по факту в этих unsigned short живут размеры картинки и до 32767 они в ближайшие годы не дорастут.

    Т.е. опять, фиксить не надо - в случае данной предметной области.

  • Все остальные найденные 'High Impact' проблемы - это просто ложные срабатывания. Т.е. код, согласен, не идеальный (видели бы вы этот код из dcraw !), но все найденное - не является ошибкой.

    В частности, любимый коффиновский метод проверки диапазона значений 0..N одним действием машинка просто не чует:

    int var=-1;
    if(bla-bla) { var = some_non_negative_value; }
    if( (unsigned)var < 5) // => это проверяет на диапазон 0..4
    Я сам ТАК не пишу, но читаю и считаю что прием, как минимум, надо знать и в чужих исходниках распознавать.

    Но кроме этих ложных срабатываний - еще куча других, тоже ложных.

Ну и по моему опыту, Static Analysis, примененный эпизодически, в большинстве случаев дает очень плохое отношение сигнал/шум. Наверное, если заниматься систематически, используя систему, где можно подавить предупреждения в конкретных местах, чтобы при сабмите следующей версии - весь этот шум не повторялся, польза может быть. Да и то....

Вопросы у меня такие:

  • А кто-нибудь использует статический анализ по смыслу (а не из соображений, что CTO/PM велел, чтобы предупреждений не было, вот мы и дуем на воду)?
  • А в компактных проектах, которые ведутся очень небольшим количеством опытных девелоперов? (я опять же понимаю, что в большом проекте статический анализатор наводит орднунг, от которого польза..)
  • И если на оба вопроса ответ "да" - то есть ли польза от потраченного времени? Ну вот если потрачу я сейчас час-другой на расстановку галок "это - не ошибка, а специально так сделано" - стоит ли ожидать пользу в дальнейшем?

Comments

не то что бы я занимался.
даже не то что бы мне давали пощупать.
но у меня сложилось впечетление, что gcc со всеми максимальными ворнингами дает результаты, сравнимые со всеми системами статического анализа.
ну и так же ложно срабатывает, да. примерно в тех же местах.

Ну для gcc-то несложно подправить исходник так, чтоб больше не ругался.
И ведь помогает, зараза такая, только на прошлой неделе долго ругал его за чрезмерную осторожность, пока не сообразил, что он прав, а у меня ошибка.

как-то не хочется из-за его паранойи лепить бессмысленную инициализацию

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

Но в данном наборе исходников - ошибки нет, да.

я не про конкретно этот код.
а про случаи типа когда первая итерация цикла гарантированно инициализирует переменную, использующуюся только после этого

а в твоем случае надо просто переписать через case и enum и варнинга не будет.

Это чужой (contributed) код, пальцем не дотронусь!

отправь патчи в апстрим!
кстати, достаточно просто enum

Нет никакого апстрима. Это именно contributed, конкретно в LibRaw.

Я с благодарностью такие куски кода принимаю, но что я при этом думаю - я никому не скажу!

повторюсь -- достаточно определить layout и Layout1 через енум, после этого даже ифы работают

а я уже привык везде лепить, только из-за этого варнинга поганого. Проще уже в переменную что-то сунуть, чем каждый раз мусор на экране смотреть..

Да-да-да, у меня такое тоже бывало, что думаешь "от какая цаца, не нравицца ему видите-ли! да идиты нафег!", а потом через какое-то "ух-ёёёё..." и дальше эхом от стен "мать-мать-мать..." )))

gcc с -Wall ложно срабатывает чаще, чем этот Coverity.

Т.е. предупреждений будет не 107, а бешеные мильены на конкретном коде.

а, вот в таком разе я особо не сравнивал, но мне показалось что примерно одинаково, все же.

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

Ну то есть идея в том, что
- при регулярном использовании мы обнаружим багу *раньше* и потратим на ее исправление меньше сил
- а на готовом проекте - смысла мало, т.к. он уже работает и доля ложных срабатываний будет слишком большой?

Интересная мысля, надо ее подумать.

> Но в реальной жизни бывает два вида layout - и это явно прописано в вызывающем коде. Т.е. у машинки достаточно данных, чтобы сообразить, что это не 'High impact', а просто неаккуратненько.

Э, нет, батенька, не согласен я с вами тут.
Это сегодня явно прописано строго два варианта вызова и никаких больше. А завтра в проект придёт новый чувак, которому поставят задачу сделать третий лейяут и он имеет полное право не знать о таком неявном свойстве кода, посему будет честное обращение к неинициализированной памяти.
Это очень правильный ворнинг, имхо. Это не "неаккуратненько", это мина на будущее расширение.
Я бы переписал тот код как свитч с throw exception по дефолту.

> С этим я не хочу спорить - формально машинка права, а по факту в этих unsigned short живут размеры картинки и до 32767 они в ближайшие годы не дорастут.

))) "640кб хватит всем!", "до 2000-го года ещё много лет, успеем поправить!" :)
Ну, т.е. понятно, что машина-то всех тонкостей domain specific не знает, посему имхо тоже вполне правильно ругается.

> В частности, любимый коффиновский метод проверки диапазона значений 0..N одним действием машинка просто не чует:

А чо-как ругается? Вполне нормальный метод, имхо...

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

1) Ну я согласен с тем, что так писать не надо. Но в том виде, в каком оно написано - это не ошибка и это видно если анализировать не только саму функцию, но и окружение.

2) Про unsigned short - я написал, что согласен именно по той причине, что domain specific неизвестен.

3) Про диапазон: дальше эта самая (unsigned)var < 5 используется для индексации массива - и ругается оно, что отрицательный индекс у массива. Ну, в тех местах, которые видел.

Мы используем, тот же Coverity. False positive'ов много, и вычищать было мучительно, но по ощущениям - польза от этого действия есть. Глупости всякие успешно отлавливает, вроде вот таких - http://trac.nginx.org/nginx/changeset/4937/nginx.

А как народ умудряется без -Wall хотя бы иногда (а лучше еще и -pedantic для верности)? Я недавно вылавливал совершенно адовые ошибко-опечатки, которые глазами трудно заметить, типа "for (...;;<условие>)" или какие-то ошибки в snprintf-спецификаторах. Ну, можно, наверное, идеально писать, но это же не всем дано. Но это и правда не часто как-то нужно.

А вод динамический valgrind, в режиме проверки памяти/текущих ресурсов - прекрасен. Ну, опять же, кто-то, наверное, делает все c RAII, аккуратненько и с первого раза, но я так не умею, от первого знакомства с валгриндом вообще аж вспотел, настолько много всего текло и вообще удивительно как работало

Так ведь при тестировании все вылезет.

Конкретно в LibRaw - куча унаследованного кода, который на Wall дает миллиарды триллионов ложных срабатываний, а править эти места не хочется. В частности по той причине, что много кода генерируется именно из dcraw.c, который мержится средствами git. Если в моей копии поправить - мержиться перестанет.

Ну и с Valgrind/подобными - тоже ведь не так просто. Я пользуюсь Intel Inspector (по смыслу то же самое). Да, что-то он ловит, но я вот скажем аллоцированное на старте программы (гарантированно один раз) на финише обычно не освобождаю, ибо незачем. Оно это ловит и правильно ловит, но с моей точки зрения - это false positive.

> Так ведь при тестировании все вылезет.

Вот конкретно в этом for-е и printf-е все как-то работало, цикл рвался внутри еще по одному условию, а в принт не приходили данные больше 32 бит, то есть ошибки увидел только с -Wall. Да, достаточно редкие ошибки, и у нас почти такая же история - есть чужие .h-файлы которые выдают кучу нелепых варнингов. Но с -Wall -pedantic пытаюсь собрать хотя бы раз в месяц, мало ли чего?

> Оно это ловит и правильно ловит, но с моей точки зрения - это false positive.

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

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

а ты отечественный продукт, у них еще радуга в логотипе и много постов на хабре, не пробовал? вроде не так плох

собстно насколько я слабо понимаю как они работают, они не делают предметного анализа или на входных данных. они просто парсят чем-то компилятороподобным код в дерево, а потом ищут в этом дереве известные им паттерны, не учитывая диапазон данных и т.д.

насчет того, что это шум я не согласен, если честно, это как миннимум плохой стиль.

большой практики с плюсами и анализаторами у меня нет, но вот с перлом чего мы только не находили благодаря перл-критику :)

Не.
Я бы и эту не пробовал - но на меня надавили из diigiKam (куда LibRaw импортится) - форвардили репорты, которые никуда не годные потому что кривые.

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

Ну собственно да нормальный цикл использования таких штук это до/после компиляции или хук на коммит

для С++ с его указателями гораздо полезнее valgrind, который может проверять очень многие вещи.
в целом, я скорее за статические анализаторы, ибо они дисциплинируют. ложные срабатывания можно отметить (а можно немного переписать код), но даже если 1 из 10 срабатываний не ложное, это хороший результат.
главное же средство борьбы с багами было и есть: простой и понятный код, покрытие тестами, грамотный логгинг, специальный дебаг режим с пред и постусловиями.

Хо-хо, тут через последний clang прогнали FreeBSD и понаходили вот такого:

struct X { ... }

struct X *obj;

memset(obj, 0, sizeof(obj));

И это после многих лет жизни с gcc -Wall -Werror

И судя по всему - это нисколько не мешало жить!

Особенно в коде Кербероса.
Но вообще -- вот вам и -Wall у gcc.

Добавлю, что прогонять проект через clang, как источник дополнительных warning'ов - действительно полезно. Тем более он там так красиво всё вырисовывает.

В принципе ты прав, цель таких тулов - ловить 1-2 новых багов в неделю, а не 100 старых багов на рабочем коде.

Но тут еще пару интересных моментов есть:
1. В этой проге 80 чеккеров на С/С++, и в каждом из них штук 5-10 конфигурабильных опций. Чтобы получить максимум пользы надо как правило потратить пару дней на настройки под конкретный кодбейс - тогда ложных будет процентов 10-15.

В SCAN подкрутить тебе ничего не дали, поэтому результаты - какие есть. Считай ты примерил брюки рандомального размера.

2. -Wall и Coverity разные вещи, и одно другому не мешает. Компайлер тебе не скажет где ты забыл mutex взять или память освободить, а Coverity не будет ругаться если ты налепишь 20 переменных которые нигде не используются.

3. Есть разница между False Positive и Intentional. Если то что прога горовит тупо не правда - к примеру, что у тебя утекает память а утечки нет и быть не может - это одно. А если она тебе говорит что variable может быть не инициализированным в твоем коде, то это не то что неаккуратно - в коде баг однозначно есть, но информация вне кода этот баг делает для тебя маловажным. В таком случае этот баг можно отметить как Intentional и прога его будет всегда игнорировать. А если совсем раждражает, этот конкретный чеккер можно вырубить :)

Тут вот же проблема: что coverity (даже только этап сбора, без всасывания на веб), что всякие другие, которые пробовал - они же все какие-то невероятно тормознутые.

Т.е. да, иметь этап статитческого анализа, когда все старые предупреждения или запихнуты под коврик (помечены как FP или Intentional) на этапе до компиляции (или одновременно) - было бы полезно.
Но. Оно только тогда полезно, когда этот этап проходит быстро, а не в *десятки* раз медленнее обычной компиляции.

Кроме того, в реальной жизни многих заставят быть рабом машины - тратить время на устранение предупреждений анализатора, даже если они intentional или FP. Ну как во многих местах разработчиков заставляют избавляться от всех предупреждений компилятора.

К чему это приводит - мы видели в истории с Debian и OpenSSL.

Большинство ползователей Коверити гоняют его в ночном билде а не на своем компе, так что скорость (которая обычно 3-5Х) разработчика волновать не должна. И кстати, если ты гоняешь на своем компе, билд инкрементальный.

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

Про Clang тут уже говорили, а про cppcheck вроде бы нет. Хорошая оупен-сорсная утилита для статического анализа.

"в компактных проектах, которые ведутся очень небольшим количеством опытных девелоперов" ?

Ага, в Rage 20 человек в кредитах на разработку, немного больше чем нужно.

Тогда он же, в Армадилло Аероспейс: https:// twitter.com/ID_AA_Carmack/status/226800462890946561

Целую статью на эту тему написал еще:

http:// www.altdevblogaday.com/2011/12/24/static-code-analysis/

Я так понял, шумят они все, а не детские ошибки ловит только PVS (которая наша разработка).

Прочитал заметку о проверке маленького проекта LibRaw с помощью Coverity SCAN. Из статьи следует, что ничего интересного не нашлось. Решил попробовать, сможет ли найти что-то анализатор PVS-Studio.

Читать далее: http://habrahabr.ru/company/pvs-studio/blog/211727/

Ага, спасибо, я там ответил.

Add new comment