- PVSM.RU - https://www.pvsm.ru -

Статические анализаторы кода на примере ClickHouse

Чуть больше месяца назад была опубликована статья [1], содержащая анализ исходного кода ClickHouse [2] с помощью PVS-Studio. Статья оказалась достаточно успешной: так, ссылку на неё мне отправили по меньшей мере десять раз в день её публикации. Общий тон статьи позитивный, а посещаемость сайта clickhouse.yandex [3] в день её выхода заметно выросла.

Я очень уважаю, когда какая-либо компания или человек делает свою работу исчерпывающим образом. Так, у PVS-Studio исчерпывающий подход к продвижению: одних только статей на Хабре 337 штук. Они проводят доклады почти на всех российских конференциях по C++. В любом случае стоит отметить: люди стараются и своим трудом приносят пользу другим людям.

Та статья пробудила в нас интерес к статическим анализаторам, и мы решили проверить работу нескольких общедоступных аналогов PVS-Studio на кодовой базе ClickHouse. В сегодняшней статье мы поделимся с вами результатами этого исследования.

Статические анализаторы кода на примере ClickHouse - 1

В день выхода статьи мы исправили все отмеченные в ней баги. Результат можно посмотреть в этом коммите [4]. Некоторых багов на момент публикации уже не было в master, так как мы исправили их ранее.

Сомнений в полезности статических анализаторов нет. Некоторые комментаторы отмечают, что гораздо полезнее иметь code review, тесты, запуски под sanitizer'ами. Польза этих решений очевидна. Тем более, у нас уже есть code review (посредственный), покоммитные тесты (с недостаточно хорошим покрытием) и запуски под sanitizers (покоммитно только ASan, раз в сутки Valgrind). Статические анализаторы могут находить другие баги – например, copy-paste в коде, который никогда не выполняется и который надо было удалить год назад. И что-нибудь более серьёзное тоже есть шанс найти (читайте дальше). То есть, польза от статических анализаторов ненулевая.

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

Мы решили сначала изучить несколько альтернатив PVS-Studio, в результате чего попробовали Clang-Tidy, Сppcheck, Coverity и Svace. Для каждого из них удалось составить инструкции о том, как правильно использовать их для кода ClickHouse.

CppCheck

Статические анализаторы кода на примере ClickHouse - 2

Инструкция по использованию: cppcheck.txt [5].

У этого инструмента есть несколько важных достоинств: он очень прост в использовании и работает чрезвычайно быстро. Так, наш репозиторий был проанализирован за несколько секунд. Стандартный режим запуска привёл к получению лишь нескольких ложно-положительных предупреждений. В режиме --enable-all было получено около 200 предупреждений, некоторые из которых оказались содержательными:

  • Кто-то забыл ввести один символ [6]. Повезло – ошибка не влияет на поведение программы.
  • Неиспользование функции log1p [7] – не знал, что такая существует. В данном случае тоже не влияет на поведение программы.

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

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

int x = 0;
auto lambda = [&]{ x = 1; };

рассматривается так же, как код:

int x = 0;
{ x = 1; };

Тем не менее, большое количество false positives – норма для статических анализаторов кода. Сценарий их использования состоит в том, чтобы просмотреть несколько сотен ошибок и, возможно, найти среди них несколько действительно существенных.

Clang-Tidy

Статические анализаторы кода на примере ClickHouse - 3

Инструкция по использованию: 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

Инструкция по использованию: coverity.txt [16].

Coverity – закрытый коммерческий продукт, но для проектов с открытым исходным кодом его можно использовать бесплатно. В числе известных продуктов, использующих Coverity есть, например, Linux.

Схема работы: с сайта скачивается утилита, которая оборачивает любую систему сборки (вместо make просто указывается cov-build make) и перехватывает вызовы компилятора. Заставить её работать удалось не с первого раза – пришлось перебрать несколько вариантов, как указать ей, какой компилятор используется.

Для некоторых единиц трансляции время работы инструмента достигает часа, а общее время анализа всего проекта – 4 часа. Результатом является tar-архив, который можно загрузить на сайт Coverity. Архив размером 1.5 GB загрузился успешно. Можно загружать и более объёмные результаты.
Затем, чтобы получить доступ к результатам работы анализатора, необходимо пройти проверку на возможность работать с исходным кодом анализируемого проекта. Например, мне доступ предоставили в течение полутора дней.

Статические анализаторы кода на примере ClickHouse - 4

После этого стал доступен красивый веб-интерфейс, в котором можно в интерактивном режиме изучать код с аннотациями дефектов, а также отмечать резолюции (false positive, intentional, fix required и т. п.).

https://scan.coverity.com/projects/yandex-clickhouse/

Сразу же выдали ачивку, правда я не понял за что.
Нашли 312 неких дефектов.

Некоторые обнаруженные дефекты на самом деле являются частью задуманного дизайна.

  • Тавтологические условия в 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 я попал на конференцию «Технологии Баз Данных» [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/