- PVSM.RU - https://www.pvsm.ru -
Чуть больше месяца назад была опубликована статья [1], содержащая анализ исходного кода ClickHouse [2] с помощью PVS-Studio. Статья оказалась достаточно успешной: так, ссылку на неё мне отправили по меньшей мере десять раз в день её публикации. Общий тон статьи позитивный, а посещаемость сайта clickhouse.yandex [3] в день её выхода заметно выросла.
Я очень уважаю, когда какая-либо компания или человек делает свою работу исчерпывающим образом. Так, у PVS-Studio исчерпывающий подход к продвижению: одних только статей на Хабре 337 штук. Они проводят доклады почти на всех российских конференциях по C++. В любом случае стоит отметить: люди стараются и своим трудом приносят пользу другим людям.
Та статья пробудила в нас интерес к статическим анализаторам, и мы решили проверить работу нескольких общедоступных аналогов PVS-Studio на кодовой базе ClickHouse. В сегодняшней статье мы поделимся с вами результатами этого исследования.
В день выхода статьи мы исправили все отмеченные в ней баги. Результат можно посмотреть в этом коммите [4]. Некоторых багов на момент публикации уже не было в master
, так как мы исправили их ранее.
Сомнений в полезности статических анализаторов нет. Некоторые комментаторы отмечают, что гораздо полезнее иметь code review, тесты, запуски под sanitizer'ами. Польза этих решений очевидна. Тем более, у нас уже есть code review (посредственный), покоммитные тесты (с недостаточно хорошим покрытием) и запуски под sanitizers (покоммитно только ASan, раз в сутки Valgrind). Статические анализаторы могут находить другие баги – например, copy-paste в коде, который никогда не выполняется и который надо было удалить год назад. И что-нибудь более серьёзное тоже есть шанс найти (читайте дальше). То есть, польза от статических анализаторов ненулевая.
В статье были указаны не все обнаруженные дефекты, и для поиска оставшихся нам предложили воспользоваться триальной лицензией PVS-Studio. Но перед тем, как запрашивать лицензию, ограниченную по времени, нам хотелось сначала убедиться, что мы достаточно хорошо подготовлены и не потратим время зря.
Мы решили сначала изучить несколько альтернатив PVS-Studio, в результате чего попробовали Clang-Tidy, Сppcheck, Coverity и Svace. Для каждого из них удалось составить инструкции о том, как правильно использовать их для кода ClickHouse.
Инструкция по использованию: cppcheck.txt [5].
У этого инструмента есть несколько важных достоинств: он очень прост в использовании и работает чрезвычайно быстро. Так, наш репозиторий был проанализирован за несколько секунд. Стандартный режим запуска привёл к получению лишь нескольких ложно-положительных предупреждений. В режиме --enable-all
было получено около 200 предупреждений, некоторые из которых оказались содержательными:
Помимо этого, анализатор также нашёл некоторые неоптимальности при передаче параметров в функции, забытые explicit-определения, слишком широкие области видимости некоторых переменных и так далее.
Многие ложно-положительные срабатывания связаны с тем, что анализатор очень примитивный. По-видимому, он даже не до конца парсит C++. Например, следующий код
int x = 0;
auto lambda = [&]{ x = 1; };
рассматривается так же, как код:
int x = 0;
{ x = 1; };
Тем не менее, большое количество false positives – норма для статических анализаторов кода. Сценарий их использования состоит в том, чтобы просмотреть несколько сотен ошибок и, возможно, найти среди них несколько действительно существенных.
Инструкция по использованию: clang-tidy.txt [8]
Важным достоинством этого инструмента является то, что он находится в экосистеме 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 [9], C++ Core Guidelines [10] или HTC++ [11] говорит «не используйте арифметику указателей». Это в каком-то смысле разумно, но явно отмечать все исходники, для которых эта проверка должна быть выключена, слишком мучительно.
Полезные сообщения нашлись в группах проверок static analyzer и performance. Performance обнаружил два типа ошибок: постинкременты итераторов и передачи аргументов по значению вместо передачи по ссылке. К сожалению, проверка передачи аргументов по ссылке срабатывает даже для небольших структур, например:
struct S
{
int x;
};
или STRONG_TYPEDEF
.
В этом случае замена передачи по значению на передачу по ссылке может оказаться бесполезной или даже вредной (при условии, что функция не инлайнится и не клонируется, ведь в этих случаях никакой разницы нет). В других ситуациях проверка оказалось более полезной, см. коммит [12] с исправлениями. Конечно, такие ошибки должны обнаруживаться на этапе code review – при условии, что вы были достаточно внимательны. Заметим, что после исправления всех предупреждений performance реальная производительность ClickHouse не изменилась. Это ожидаемо: если бы соответствующие ошибки были в узких местах, мы бы это заметили при первом же запуске sudo perf top
.
Предупреждения от static analyzer весьма полезны:
Инструкция по использованию: coverity.txt [16].
Coverity – закрытый коммерческий продукт, но для проектов с открытым исходным кодом его можно использовать бесплатно. В числе известных продуктов, использующих Coverity есть, например, Linux.
Схема работы: с сайта скачивается утилита, которая оборачивает любую систему сборки (вместо make
просто указывается cov-build make
) и перехватывает вызовы компилятора. Заставить её работать удалось не с первого раза – пришлось перебрать несколько вариантов, как указать ей, какой компилятор используется.
Для некоторых единиц трансляции время работы инструмента достигает часа, а общее время анализа всего проекта – 4 часа. Результатом является tar-архив, который можно загрузить на сайт Coverity. Архив размером 1.5 GB загрузился успешно. Можно загружать и более объёмные результаты.
Затем, чтобы получить доступ к результатам работы анализатора, необходимо пройти проверку на возможность работать с исходным кодом анализируемого проекта. Например, мне доступ предоставили в течение полутора дней.
После этого стал доступен красивый веб-интерфейс, в котором можно в интерактивном режиме изучать код с аннотациями дефектов, а также отмечать резолюции (false positive, intentional, fix required и т. п.).
Сразу же выдали ачивку, правда я не понял за что.
Нашли 312 неких дефектов.
Некоторые обнаруженные дефекты на самом деле являются частью задуманного дизайна.
Кроме того, одна из ошибок выглядела как Unrecoverable parse error
при виде std::shared_mutex. Это значит, что Coverity не поддерживает C++17.
30 ноября 2016 я попал на конференцию «Технологии Баз Данных» [20], на которой мне удалось договориться с коллегами из ИСП РАН об использовании их инструмента статического анализа Svace. Этот инструмент внедрен в корпорации Samsung, но малоизвестен в среде отечественных программистов.
Результаты анализа доступны в удобном веб-интерфейсе с просмотром исходного кода и аннотаций к нему. Самая серьёзная из обнаруженных ошибок – отсутствие freeaddrinfo в одной из веток кода [21]. Стоит отметить, что и результаты анализа, и выборку дефектов для изучения нам предоставили коллеги из ИСП РАН.
Использование статических анализаторов – не первая по важности мера для улучшения кода вашего продукта. Например, если у вас нет автосборки с запуском тестов хотя бы под Address Sanitizer – полезнее будет сначала это обеспечить. Разбор результатов работы статических анализаторов требует большого количества времени, по меньшей мере, в самом начале. Правильное их использование требует наличия интеграции в систему CI. Только если вы сделали более простые вещи – можно попробовать такую замечательную вещь как статические анализаторы. Впрочем, зачем это я вас отговариваю – обязательно попробуйте!
Напоследок, хочется ещё раз поблагодарить коллег из PVS-Studio за работу, которую они делают, в частности – за статью, посвящённую ClickHouse. Они не только помогли нам, обнаружив несколько важных проблем в нашем коде, но и вдохновили исследование, которому посвящена настоящая статья.
Автор: o6CuFl2Q
Источник [22]
Сайт-источник PVSM.RU: https://www.pvsm.ru
Путь до страницы источника: https://www.pvsm.ru/pvs-studio/267889
Ссылки в тексте:
[1] статья: https://habrahabr.ru/company/pvs-studio/blog/337182/
[2] исходного кода ClickHouse: https://github.com/yandex/ClickHouse/
[3] clickhouse.yandex: https://clickhouse.yandex/
[4] в этом коммите: https://habrahabr.ru/company/pvs-studio/blog/337182/#comment_10401018
[5] cppcheck.txt: https://github.com/yandex/ClickHouse/blob/master/dbms/tests/instructions/cppcheck.txt
[6] Кто-то забыл ввести один символ: https://github.com/yandex/ClickHouse/commit/8b313ab99ec6330d26d64596666725f717a29e51#diff-8b268ddacef447bca1660a97b8ef0e4cL2307
[7] log1p: https://github.com/yandex/ClickHouse/commit/8b313ab99ec6330d26d64596666725f717a29e51#diff-b3ef0d50782d4803e19506c1b9a5ad2fR104
[8] clang-tidy.txt: https://github.com/yandex/ClickHouse/blob/master/dbms/tests/instructions/clang-tidy.txt
[9] CERT Secure Coding Standards: https://www.securecoding.cert.org/confluence/pages/viewpage.action?pageId=637
[10] C++ Core Guidelines: https://github.com/isocpp/CppCoreGuidelines
[11] HTC++: http://www.codingstandard.com/section/index/
[12] коммит: https://github.com/yandex/ClickHouse/commit/d29c77adea08af7922d7b82e56ccee2f2d11783f#diff-4a276210b42fab5b36975206847b0d85L253
[13] Вызов виртуальной функции в конструкторе: https://github.com/yandex/ClickHouse/commit/e9ae1938706c8c6e4aab478c0356b1dcf4d300e9#diff-9b62998c55ce7cf34c0e6d37ed6b08b3L50
[14] Потеря точности в вычислениях: https://github.com/yandex/ClickHouse/commit/d19d9f8589d8e39d104a7ad8cd7d56c79324c250#diff-c77cad8ee42ebae8e1ae03d0abf3336eL287
[15] Разыменование нулевого указателя: https://github.com/yandex/ClickHouse/commit/4799f28dadc31f4c967f5304cf5465c307ee1573#diff-4650bcb54ab5af2e8c26f4c200075c7cR37
[16] coverity.txt: https://github.com/yandex/ClickHouse/blob/master/dbms/tests/instructions/coverity.txt
[17] Неинициализированная переменная: https://github.com/yandex/ClickHouse/commit/ed1c0820f00c33b54eda7750b3b3c746a70038c7
[18] Разыменование итератора end: https://github.com/yandex/ClickHouse/commit/9cafeb9e85ce094969e79ce46b85233bc682de4f
[19] Всякая мелочь: https://github.com/yandex/ClickHouse/commit/5e25c40a26f5499afa09916cad2df243a1a4daef#diff-909f5a9f60a9cc746f7b737de5241901L50
[20] «Технологии Баз Данных»: http://ospcon.osp.ru/tbd_dbms2017/
[21] в одной из веток кода: https://github.com/yandex/ClickHouse/commit/861684c5544970a2db9e8cdf2f3fabe4e0659c4e
[22] Источник: https://habrahabr.ru/post/342018/
Нажмите здесь для печати.