Чуть больше месяца назад была опубликована статья, содержащая анализ исходного кода ClickHouse с помощью PVS-Studio. Статья оказалась достаточно успешной: так, ссылку на неё мне отправили по меньшей мере десять раз в день её публикации. Общий тон статьи позитивный, а посещаемость сайта clickhouse.yandex в день её выхода заметно выросла.
Я очень уважаю, когда какая-либо компания или человек делает свою работу исчерпывающим образом. Так, у PVS-Studio исчерпывающий подход к продвижению: одних только статей на Хабре 337 штук. Они проводят доклады почти на всех российских конференциях по C++. В любом случае стоит отметить: люди стараются и своим трудом приносят пользу другим людям.
Та статья пробудила в нас интерес к статическим анализаторам, и мы решили проверить работу нескольких общедоступных аналогов PVS-Studio на кодовой базе ClickHouse. В сегодняшней статье мы поделимся с вами результатами этого исследования.
В день выхода статьи мы исправили все отмеченные в ней баги. Результат можно посмотреть в этом коммите. Некоторых багов на момент публикации уже не было в master, так как мы исправили их ранее.
Сомнений в полезности статических анализаторов нет. Некоторые комментаторы отмечают, что гораздо полезнее иметь code review, тесты, запуски под sanitizer'ами. Польза этих решений очевидна. Тем более, у нас уже есть code review (посредственный), покоммитные тесты (с недостаточно хорошим покрытием) и запуски под sanitizers (покоммитно только ASan, раз в сутки Valgrind). Статические анализаторы могут находить другие баги – например, copy-paste в коде, который никогда не выполняется и который надо было удалить год назад. И что-нибудь более серьёзное тоже есть шанс найти (читайте дальше). То есть, польза от статических анализаторов ненулевая.
В статье были указаны не все обнаруженные дефекты, и для поиска оставшихся нам предложили воспользоваться триальной лицензией PVS-Studio. Но перед тем, как запрашивать лицензию, ограниченную по времени, нам хотелось сначала убедиться, что мы достаточно хорошо подготовлены и не потратим время зря.
Мы решили сначала изучить несколько альтернатив PVS-Studio, в результате чего попробовали Clang-Tidy, Сppcheck, Coverity и Svace. Для каждого из них удалось составить инструкции о том, как правильно использовать их для кода ClickHouse.
CppCheck
Инструкция по использованию: cppcheck.txt.
У этого инструмента есть несколько важных достоинств: он очень прост в использовании и работает чрезвычайно быстро. Так, наш репозиторий был проанализирован за несколько секунд. Стандартный режим запуска привёл к получению лишь нескольких ложно-положительных предупреждений. В режиме --enable-all было получено около 200 предупреждений, некоторые из которых оказались содержательными:
- Кто-то забыл ввести один символ. Повезло – ошибка не влияет на поведение программы.
- Неиспользование функции log1p – не знал, что такая существует. В данном случае тоже не влияет на поведение программы.
Помимо этого, анализатор также нашёл некоторые неоптимальности при передаче параметров в функции, забытые explicit-определения, слишком широкие области видимости некоторых переменных и так далее.
Многие ложно-положительные срабатывания связаны с тем, что анализатор очень примитивный. По-видимому, он даже не до конца парсит C++. Например, следующий код
int x = 0;
auto lambda = [&]{ x = 1; };рассматривается так же, как код:
int x = 0;
{ x = 1; };Тем не менее, большое количество false positives – норма для статических анализаторов кода. Сценарий их использования состоит в том, чтобы просмотреть несколько сотен ошибок и, возможно, найти среди них несколько действительно существенных.
Clang-Tidy
Инструкция по использованию: clang-tidy.txt
Важным достоинством этого инструмента является то, что он находится в экосистеме Clang. Это значит, что, если ваша система компилируется при помощи Clang, то Clang-Tidy будет корректно учитывать все особенности сборки вашего проекта. Есть инструкция, как добавлять свои проверки.
На анализ каждого translation unit'а Clang-Tidy тратит около десятка секунд. Это достаточно медленно.
Clang-Tidy отличается от Clang static analyzer (также известного под названием scan-build) наличием проверок стиля. Некоторые из проверок стиля позволяют автоматически переписывать код. Стоит отметить, что Clang static analyzer, в отличие от Clang-Tidy, умеет генерировать HTML-страницы с результатами, тогда как Clang-Tidy лишь выводит сообщения о найденных проблемах в окно терминала.
Использовать все типы проверок нет смысла. Так, одна из проверок CERT Secure Coding Standards, C++ Core Guidelines или HTC++ говорит «не используйте арифметику указателей». Это в каком-то смысле разумно, но явно отмечать все исходники, для которых эта проверка должна быть выключена, слишком мучительно.
Полезные сообщения нашлись в группах проверок static analyzer и performance. Performance обнаружил два типа ошибок: постинкременты итераторов и передачи аргументов по значению вместо передачи по ссылке. К сожалению, проверка передачи аргументов по ссылке срабатывает даже для небольших структур, например:
struct S
{
int x;
};или STRONG_TYPEDEF.
В этом случае замена передачи по значению на передачу по ссылке может оказаться бесполезной или даже вредной (при условии, что функция не инлайнится и не клонируется, ведь в этих случаях никакой разницы нет). В других ситуациях проверка оказалось более полезной, см. коммит с исправлениями. Конечно, такие ошибки должны обнаруживаться на этапе code review – при условии, что вы были достаточно внимательны. Заметим, что после исправления всех предупреждений performance реальная производительность ClickHouse не изменилась. Это ожидаемо: если бы соответствующие ошибки были в узких местах, мы бы это заметили при первом же запуске sudo perf top.
Предупреждения от static analyzer весьма полезны:
- Вызов виртуальной функции в конструкторе. Оказалось, что virtual написали по ошибке.
- Потеря точности в вычислениях
- Разыменование нулевого указателя.
Coverity
Инструкция по использованию: coverity.txt.
Coverity – закрытый коммерческий продукт, но для проектов с открытым исходным кодом его можно использовать бесплатно. В числе известных продуктов, использующих Coverity есть, например, Linux.
Схема работы: с сайта скачивается утилита, которая оборачивает любую систему сборки (вместо make просто указывается cov-build make) и перехватывает вызовы компилятора. Заставить её работать удалось не с первого раза – пришлось перебрать несколько вариантов, как указать ей, какой компилятор используется.
Для некоторых единиц трансляции время работы инструмента достигает часа, а общее время анализа всего проекта – 4 часа. Результатом является tar-архив, который можно загрузить на сайт Coverity. Архив размером 1.5 GB загрузился успешно. Можно загружать и более объёмные результаты.
Затем, чтобы получить доступ к результатам работы анализатора, необходимо пройти проверку на возможность работать с исходным кодом анализируемого проекта. Например, мне доступ предоставили в течение полутора дней.
После этого стал доступен красивый веб-интерфейс, в котором можно в интерактивном режиме изучать код с аннотациями дефектов, а также отмечать резолюции (false positive, intentional, fix required и т. п.).
Сразу же выдали ачивку, правда я не понял за что.
Нашли 312 неких дефектов.
- Неинициализированная переменная, если пользователь в конфиге не укажет ни одного элемента в одной из секций.
- Разыменование итератора end только для того, чтобы превратить его в адрес.
- Всякая мелочь.
Некоторые обнаруженные дефекты на самом деле являются частью задуманного дизайна.
- Тавтологические условия в if'ах в шаблонном коде. Это полностью нормально, является распространённой практикой и стилем кода. Более того, код пишется, исходя из предположения, что компилятор об этом прекрасно знает и удалит ненужный код.
- Неинициализированный член класса в конструкторе по умолчанию. Иногда это действительно ошибка, но в некоторых случаях бывает допустимо.
- Необработанное исключение в функции main в тестовой программе. Это допускает наш стиль. Если исключение вылетело из функции main, то оно будет обработано с помощью std::terminate, стандартная библиотека сделает несколько полезных вещей за вас (verbose terminate handler): выведет тип исключения, для наследников std::exception выведет what, позовёт функцию abort; программа получит сигнал abort, операционная система запишет core dump, и программа завершится с ненулевым кодом. Всё это очень полезно.
Кроме того, одна из ошибок выглядела как Unrecoverable parse error при виде std::shared_mutex. Это значит, что Coverity не поддерживает C++17.
Svace
30 ноября 2016 я попал на конференцию «Технологии Баз Данных», на которой мне удалось договориться с коллегами из ИСП РАН об использовании их инструмента статического анализа Svace. Этот инструмент внедрен в корпорации Samsung, но малоизвестен в среде отечественных программистов.
Результаты анализа доступны в удобном веб-интерфейсе с просмотром исходного кода и аннотаций к нему. Самая серьёзная из обнаруженных ошибок – отсутствие freeaddrinfo в одной из веток кода. Стоит отметить, что и результаты анализа, и выборку дефектов для изучения нам предоставили коллеги из ИСП РАН.
Выводы
Использование статических анализаторов – не первая по важности мера для улучшения кода вашего продукта. Например, если у вас нет автосборки с запуском тестов хотя бы под Address Sanitizer – полезнее будет сначала это обеспечить. Разбор результатов работы статических анализаторов требует большого количества времени, по меньшей мере, в самом начале. Правильное их использование требует наличия интеграции в систему CI. Только если вы сделали более простые вещи – можно попробовать такую замечательную вещь как статические анализаторы. Впрочем, зачем это я вас отговариваю – обязательно попробуйте!
Напоследок, хочется ещё раз поблагодарить коллег из PVS-Studio за работу, которую они делают, в частности – за статью, посвящённую ClickHouse. Они не только помогли нам, обнаружив несколько важных проблем в нашем коде, но и вдохновили исследование, которому посвящена настоящая статья.
Автор: o6CuFl2Q
