company_banner

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

    Чуть больше месяца назад была опубликована статья, содержащая анализ исходного кода 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 весьма полезны:



    Coverity


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


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


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


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



    После этого стал доступен красивый веб-интерфейс, в котором можно в интерактивном режиме изучать код с аннотациями дефектов, а также отмечать резолюции (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 я попал на конференцию «Технологии Баз Данных», на которой мне удалось договориться с коллегами из ИСП РАН об использовании их инструмента статического анализа Svace. Этот инструмент внедрен в корпорации Samsung, но малоизвестен в среде отечественных программистов.


    Результаты анализа доступны в удобном веб-интерфейсе с просмотром исходного кода и аннотаций к нему. Самая серьёзная из обнаруженных ошибок – отсутствие freeaddrinfo в одной из веток кода. Стоит отметить, что и результаты анализа, и выборку дефектов для изучения нам предоставили коллеги из ИСП РАН.


    Выводы


    Использование статических анализаторов – не первая по важности мера для улучшения кода вашего продукта. Например, если у вас нет автосборки с запуском тестов хотя бы под Address Sanitizer – полезнее будет сначала это обеспечить. Разбор результатов работы статических анализаторов требует большого количества времени, по меньшей мере, в самом начале. Правильное их использование требует наличия интеграции в систему CI. Только если вы сделали более простые вещи – можно попробовать такую замечательную вещь как статические анализаторы. Впрочем, зачем это я вас отговариваю – обязательно попробуйте!


    Напоследок, хочется ещё раз поблагодарить коллег из PVS-Studio за работу, которую они делают, в частности – за статью, посвящённую ClickHouse. Они не только помогли нам, обнаружив несколько важных проблем в нашем коде, но и вдохновили исследование, которому посвящена настоящая статья.

    • +90
    • 9,5k
    • 8
    Яндекс 569,85
    Как мы делаем Яндекс
    Поделиться публикацией
    Комментарии 8
    • +5
      Тавтологические условия в if'ах в шаблонном коде. Это полностью нормально, является распространённой практикой и стилем кода.

      Можно про эту часть чуть подробнее?
      • +1
        Самый простой пример:

        template <typename T>
        void f(T x)
        {
            if (x < 0)  /// Код будет полностью удалён, если x - unsigned.
            {
                ...
            }
            ...
        }
        ...
        
        int x;
        f(x);
        
        unsigned y;
        f(y);
        


        Реальный пример из практики:

        github.com/yandex/ClickHouse/blob/ff1598c8d12970c9bb76d3f0fd6bb8a22471b7ae/dbms/src/Interpreters/Aggregator.cpp#L596

        Смотрите на no_more_keys и Method::no_consecutive_keys_optimization.
        Это всё bool, известный на этапе компиляции.

        В C++17 это можно написать более явно с помощью if constexpr, впрочем область применения if constexpr больше.

        Кстати, в некоторых случаях для достижения такого же эффекта даже не обязательно использовать шаблоны. Можно передать обычный параметр в функцию, понадеясь на то, что компилятор будет достаточно умён, чтобы применить оптимизацию — клонирование функций после constant propagation (см. параметр -fipa-cp-clone в gcc, входит в -O3).
      • +1
        Clang-Tidy отличается от Clang static analyzer (также известного под названием scan-build) наличием проверок стиля.

        Чуть разверну. Clang-Tidy включает в себя Clang Static Analyzer (также известного как clang --analyze, как scan-build и под видом менюшки Product->Analyze в Xcode и Analyze->Clang Static Analyzer в QtCreator) посредством clang-tidy -checks=clang-analyzer-* и дополняет его собственными проверками, как стилистическими, так и несложным bug-finding-ом на уровне синтаксиса.


        Я проспал момент, почему эти два анализатора вообще существуют отдельно, но сейчас у них сильно разная "философия" (Analyzer старается, насколько это возможно, искать только "реальные баги" и таким образом не сильно отвлекать разработчиков, а Tidy собирает под себя все мыслимые и немыслимые стилистические проверки) и немного разные фичи (Analyzer никак не может включить в себя проверки из Tidy — только наоборот; у Tidy мало интеграции с известными IDE, зато есть приятные fixit-ы; а в собственных проверках Tidy не используется движок "символьного выполнения" программы, на котором в Analyzer'е реализованы его самые хитрые и уважаемые проверки).

        • 0
          Спасибо! Я никак не мог понять, почему появилось два инструмента, и документации, где было бы об этом написано, не нашёл.
          • 0
            Clang Static Analyzer — часть Clang. Clang-Tidy — входит в clang-tools-extra, т. е. это независимый исполняемый файл. Исторически над Clang Static Analyzer в основном работал Apple, над Clang-Tidy — Google. Так же Clang-Tidy поглотил Clang-modernize. Кроме этого есть ещё предупреждения Clang. Но очень не хватает координации, для того, чтобы определить, какая функциональность где должна находиться.
        • +3
          Спасибо за статью!
          • +1
            А метрика у вас как раз ведь на ClickHouse работает?

            Когда почините глюки с запросом больших колличеств статистических данных? Глюк очень просто воспроизвести — переходим в метрику, выбираем детализацию по дням, а затем период статистики в пару лет и… Вуаля! Вместо данных шляпа. Хотя иногда почему-то отрабатывает нормально. Похоже в случаях, когда до первого запроса большого периода использовалась детализация «по неделям», но не уверен.

            Давно уже писал об этой проблеме, её игнорируют. Раньше, правда, ещё хуже было — после такой манипуляции метрика вообще переставала нормально данные по любым периодам отображать, приходилось заходить заново, и то не всегда помогало. В общем, если почините, будет совсем круто.

            В качестве немного костыльного решения, предлагаю при слишком большом периоде автоматически уменьшать доступную детализацию до «недели».

            Возможно совпадение, но вроде бы стало работать чуть лучше как раз примерно в то же время что и вышла статья, где вас проверяли в PVS-Studio =)
            • 0
              А метрика у вас как раз ведь на ClickHouse работает?

              Да.

              Когда почините глюки с запросом больших колличеств статистических данных? Глюк очень просто воспроизвести — переходим в метрику, выбираем детализацию по дням, а затем период статистики в пару лет и… Вуаля! Вместо данных шляпа. Хотя иногда почему-то отрабатывает нормально. Похоже в случаях, когда до первого запроса большого периода использовалась детализация «по неделям», но не уверен.

              Не могу подробно прокомментировать конкретно вашу проблему. Если коллеги из Метрики найдут ваше обращение в support, подскажут номер счётчика, то мы сможем поискать медленные запросы в query_log.

              Пока у меня только есть некоторые догадки — например, я знаю, что несколько отчётов плохо работают из-за cache словарей из MySQL (при этом для MySQL используется слабый кластер без SSD) — для решения проблемы мы реализовали словари из shared library, которые идут напрямую в YT. Но эта возможность ещё не используется. Надеюсь, что станет лучше.

              Возможно совпадение, но вроде бы стало работать чуть лучше как раз примерно в то же время что и вышла статья, где вас проверяли в PVS-Studio =)

              Это никак не связано. Впрочем я буду рад, если больше людей будут изучать и проверять наш код.

            Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

            Самое читаемое