Проверка PVS-Studio с помощью Clang

    Checking PVS-Studio with Clang
    Да, да. Вы не ослышались. В этот раз статья «наоборот». Не мы проверяем какой-то проект, а проверили наш анализатор с помощью другого инструмента. На самом деле, подобное делали мы и раньше. Например, проверяли PVS-Studio с помощью Cppcheck, с помощью анализатора, встроенного в Visual Studio, смотрели на предупреждения Intel C++. Но раньше не было повода написать статью. Ничего интересного не находилось. А вот Clang смог заинтересовать своими диагностическими сообщениями.

    Мы два раза проверяли Clang с помощью PVS-Studio [1, 2] и каждый раз находили что-то интересное. А вот проверить PVS-Studio нам никак не удавалось. Разработчики Clang давно рапортуют, что хорошо собирают проекты в Windows, разработанные с помощью Visual C++. Но на практике, как-то не получается. Или нам не везёт.

    А на днях мы неожиданно поняли, что легко можем проверить свой анализатор c помощью Clang. Просто надо было подойти к задаче с другой стороны. У нас каждую ночь собирается command-line версия PVS-Studio под Linux с помощью GCC. И компилятор GCC очень просто заменяется на Clang. Соответственно, мы легко можем попробовать проверить PVS-Studio. И, действительно. В этот же день, как светлая идея пришла в голову одному из сотрудников, у нас появился отчёт о проверке PVS-Studio. Теперь я сижу и рассказываю о содержимом отчета и своих впечатлениях.

    Впечатление об html-отчётах


    Я, конечно, уже имел дело с Clang. Однако, сложно судить о качестве анализа на сторонних проектах. Я часто не могу понять, есть ошибка или нет. Особенно пугает, когда Clang показывает, что нужно просмотреть путь из 37 точек в исходном коде.

    Код PVS-Studio, напротив, знаком мне, и я наконец смог полноценно поработать с отчётом, выданным Clang. К сожалению, это подтвердило мои более раннее впечатления, что часто показанный путь достижения обнаруженной ошибки избыточен и запутывает программиста. Я понимаю, что выдать ключевые точки выполнения программы и построить такой путь крайне сложная задача. Мы, например, в PVS-Studio вообще боимся браться за такую объемную задачу. Однако, раз Clang реализует показ этого пути, то явно надо идти дальше в улучшении этого механизма.

    В противном случае, вот такие точки только сбивают, замусоривают вывод и усложняют понимание:



    На рисунке показана «точка N4». Где-то ниже находится ошибка. Я понимаю, что ошибка возникнет только в том случае, если условие не выполнится. Об этом и сообщает Clang. Но зачем выводить эту информацию? И так понятно, что если условие истинно, то произойдёт выход из функции и ошибки не будет. Лишняя бессмысленная информация. Такой избыточной информации весьма много. Явно можно и стоит улучшать этот механизм.

    Однако, хочу отдать разработчикам Clang должное. Часто показ такого пути действительно помогает понять причину ошибки, особенно, когда в ней замешена более, чем одна функция. И, однозначно, у разработчиков Clang отображение пути достижения ошибки получилось намного лучше, чем в статическом анализаторе Visual Studio 2013. Там нередко подкрашено половина функции, состоящей из 500 строк. И, что даёт эта раскраска совершенно непонятно.

    Критичность найденных ошибок


    Проверка PVS-Studio очень хороший пример, какое неблагодарное занятие показывать пользу от статического анализа на работающем, хорошо протестированном проекте. На самом деле, я могу «отмазаться» от всех ошибок, сказав, что:
    • этот код сейчас не используется;
    • этот код используется крайне редко или при обработке ошибок;
    • эта ошибка, но она не приведёт к заметным последствиям (её исправление никак не проявляет себя на огромном количестве регрессионных тестов).


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

    Конечно, так я говорить не буду! То, что не найдены критические ошибки, вовсе не говорит, что Clang слаб в анализе. Отсутствие таких дефектов — это результат большой работы по тестированию различными методами:
    • внутренние юнит-тесты;
    • регрессионное тестирование по диагностикам (размеченные файлы);
    • тестирование на наборах *.i файлов, содержащих разнообразнейшие конструкции на Си++ и различные расширения;
    • регрессионное тестирование на 90 open-source проектах;
    • и, конечно, статический анализ с помощью PVS-Studio.


    После такой эшелонированной защиты трудно ожидать, что Clang найдет 20 разыменований нулевых указателей и 10 делений на 0. Однако, вдумайтесь. Даже в тщательно протестированном проекте Clang обнаружил несколько ошибок. Это значит, что при регулярном использовании можно избежать массы неприятностей. Лучше поправить ошибку, когда её найдет Clang, чем получить от пользователя *.i файл, на котором падает PVS-Studio.

    Естественно, мы сделали выводы для себя. Сейчас мой коллега как раз занимается настройкой запуска Clang на сервере и отправкой логов на почту, если анализатор что-то найдёт.

    О ложных срабатываниях


    Всего анализатор Clang выдал 45 предупреждений. Про количество ложных срабатываний говорить не хочу. Лучше скажу, что следует обязательно поправить 12 мест.

    Дело в том, что «ложное срабатывание» — дело относительное. Формально анализатор бывает прав — код плох и подозрителен. Но это вовсе не значит, что найден дефект. Поясню на примерах.

    В начале рассмотрим настоящее ложное срабатывание:
    #define CreateBitMask(bitNum) ((v_uint64)(1) << bitNum)
    
    unsigned GetBitCountForRepresntValueLoopMethod(
      v_int64 value, unsigned maxBitsCount)
    {
      if (value == 0)
        return 0;
      if (value < 0)
        return maxBitsCount;
      v_uint64 uvalue = value;
      unsigned n = 0;
      int bit;
      for (bit = maxBitsCount - 1; bit >= 0; --bit)
      {
        if ((uvalue & CreateBitMask(bit)) != 0)
         // Clang: Within the expansion of the macro 'CreateBitMask':
         // The result of the '<<' expression is undefined
        {
          n = bit + 1;
          break;
        }
      ....
    }

    Как я понимаю, анализатор сообщает, что операция сдвига может привести к неопределённому поведению. По всё видимости, Clang запутался в логике или не смог правильно вычислить возможный диапазон значений переменной maxBitsCount. Я внимательно изучил, как вызывается функция GetBitCountForRepresntValueLoopMethod() и не смог найти ситуацию, когда значение в переменной 'maxBitsCount' будет слишком большим. Я разбираюсь в сдвигах [3]. Так что уверен, что ошибки нет.

    Уверенность в себе, это хорошо, но не достаточно. Поэтому я вписал вот такой assert():
    ....
    for (bit = maxBitsCount - 1; bit >= 0; --bit)
    {
      VivaAssert(bit >= 0 && bit < 64);
      if ((uvalue & CreateBitMask(bit)) != 0)
    ....

    И на всех тестах этот assert() не сработал. Так что это действительно пример ложного срабатывания анализатора Clang.

    Приятным последствием добавления assert() стало то, что Clang перестал выдавать сообщение. Он ориентируется на макросы assert(). Они подсказывают анализатору возможные диапазоны значений переменных.

    Таких настоящих ложных срабатываний мало. Гораздо больше вот таких сообщений:
    static bool G807_IsException1(const Ptree *p)
    {
      ....
        if (kind == ntArrayExpr) {
          p = First(p);
          kind = p->What();
            // Clang: Value stored to 'kind' is never read
      ....

    Присваивание «kind = p->What();» не используется. Раньше использовалось, а после изменений эта строчка стала лишней. Анализатор прав. Строка лишняя, и её следует удалить хотя бы для того, чтобы она не сбивала с толку человека, который будет сопровождать этот код.

    Ещё пример:
    template<> template<>
    void object::test<11>() {
      ....
      // Нулевой nullWalker не будет использоваться в тестах.
      VivaCore::VivaWalker *nullWalker = 0;
      left.m_simpleType = ST_INT;
      left.SetCountOfUsedBits(32);
      left.m_creationHistory = TYPE_FROM_VALUE;
      right.m_simpleType = ST_INT;
      right.SetCountOfUsedBits(11);
      right.m_creationHistory = TYPE_FROM_EXPRESSION;
      result &= ApplyRuleN1(*nullWalker, left, right, false);
        // Clang: Forming reference to null pointer
      ....
    }

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

    Оба примера показывают код, который работает. Однако, я не называю это ложными срабатываниями. Это недочёты, которые надо устранить. Однако записывать эти предупреждения в графу «найденные ошибки» я бы тоже не стал. Вот поэтому я и говорю, что ложное срабатывание — понятие относительное.

    Найденные ошибки


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

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

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

    Использование двух неинициализированных переменных


    Код был большой и сложный. Приводить его в статье нет никакого смысла. Я сделал синтетический код, который отражает суть ошибки.
    int A, B;
    bool getA, getB;
    Get(A, getA, B, getB);
    int TmpA = A; // Clang: Assigned value is garbage or undefined
    int TmpB = B; // Clang: Assigned value is garbage or undefined
    if (getA)
      Use(TmpA);
    if (getB)
      Use(TmpB);

    Функция Get() может инициализировать переменные A и B. Инициализировала она их или нет, она отмечает в переменных getA, getB.

    Независимо от того, инициализированы переменные A и B, их значения копируются в TmpA, TmpB. В этом месте и есть использование неинициализированных переменной.

    Почему я говорю, что это некритическая ошибка? На практике, копирование неинициализированной переменной типа 'int' не вызывает какой-то беды. Формально, как я понимаю, возникает Undefined behavior. На практике, просто скопируется мусор. Далее переменные с мусором не используются.

    Код был переписан следующим образом:
    if (getA)
    {
      int TmpA = A;
      Use(TmpA);
    }
    if (getB)
    {
      int TmpB = B;
      Use(TmpB);
    }

    Неинициализированные указатели


    Посмотрим на вызов функции GetPtreePos(). Ей передаются ссылки на неинициализированные указатели.
    SourceLocation Parser::GetLocation(const Ptree* ptree)
    {
      const char *begin, *end;
      GetPtreePos(ptree, begin, end);
        return GetSourceLocation(*this, begin);
    }

    Это неправильно. Функция GetPtreePos() предполагает, что указатели будут инициализированы значением nullptr. Вот как она устроена:
    void GetPtreePos(const Ptree *p, const char *&begin, const char *&end)
    {
      while (p != nullptr)
      {
        if (p->IsLeaf())
        {
          const char *pos = p->GetLeafPosition();
          if (....)
          {
            if (begin == nullptr) {
                // Clang: The left operand of '==' is a garbage value
              begin = pos;
            } else {
              begin = min(begin, pos);
            }
            end = max(end, pos);
          }
          return;
        }
        GetPtreePos(p->Car(), begin, end);
        p = p->Cdr();
      }
    }

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

    Хороший пример, как статический анализ дополняет TDD [4].

    Страшные явные приведения типов


    Есть три похожие функции со страшными и неправильными приведениями типов. Вот одна из них:
    bool Environment::LookupType(
      CPointerDuplacateGuard &envGuard, const char* name,
      size_t len, Bind*& t, const Environment **ppRetEnv,
      bool includeFunctions) const
    {
      VivaAssert(m_isValidEnvironment);
      //todo:
      Environment *eTmp = const_cast<Environment *>(this);
      Environment **ppRetEnvTmp = const_cast<Environment **>(ppRetEnv);
      bool r = eTmp->LookupType(envGuard, name, len, t,
                                ppRetEnvTmp, includeFunctions);
      ppRetEnv = const_cast<const Environment **>(ppRetEnvTmp);
        // Clang: Value stored to 'ppRetEnv' is never read
      return r;
    }

    Содом и Гоморра. Попытались снять константность. А потом вернуть полученное значение. Но, на самом деле, в строке «ppRetEnv = const_cast....» просто меняется локальная переменная ppRetEnv.

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

    Анализатор PVS-Studio основан на библиотеке OpenC++. В ней практически не использовалось ключевое слово 'const'. В любой момент можно было изменить, что угодно и, где угодно, используя указатели на не константные объекты. PVS-Studio унаследовал этот порок.

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

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

    На что влияет эта ошибка? Как ни странно, кажется ни на что. Все юнит-тесты и регрессионные тесты не выявляли никаких изменений в поведении PVS-Studio после исправлений. Видимо, возвращаемое значение в «ppRetEnv» не очень то и нужно при работе.

    Использование потенциально неинициализированной переменной


    v_uint64 v; // Clang: 'v' declared without an initial value
    verify(GetEscape(p, len - 3, v, notation, &p));
    retValue <<= 8;
    retValue |= v; // Clang: Assigned value is garbage or undefined

    Функция GetEscape() может отработать некорректно и тогда переменная 'v' останется неинициализированной. Результат работы GetEscape() почему-то проверяет макрос verify(). Как так получилось, уже неизвестно.

    Ошибка остаётся незамеченной по следующей причине. Функция GetEscape() не инициализирует переменную только в том случае, если анализатор PVS-Studio будет работать с некорректным текстом программы. В корректном тексте программы всегда корректные ESC-последовательности и переменная всегда инициализируется.

    Сам удивлён, как это работало


    Ptree *varDecl = bind->GetDecl();
    if (varDecl != nullptr)
    {
      if (varDecl->m_wiseType.IsIntegerVirtualValue())
        varRanges.push_back(....);
      else if (varDecl->m_wiseType.IsPointerVirtualValue())
        varRanges.push_back(....);
      else
        varRanges.push_back(nullptr);
    }
    rangeTypes.push_back(varDecl->m_wiseType.m_simpleType);
      // Clang: Dereference of null pointer

    Указатель varDecl может быть равен nullptr. Однако, последняя строчка выполняется всегда. И может произойти разыменование нулевого указателя: varDecl->m_wiseType.m_simpleType.

    Почему никогда не наблюдалось падение здесь — для меня загадка. Единственно предположение, сюда мы никогда не попадаем если объект не хранит указатель на объявление переменной (declarator of variable). Но полагаться на это все равно нельзя.

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

    Удивительно, но и здесь небыли замечены падения


    Ещё одно удивительное место. Видимо, сочетание факторов, которое может привести к разыменовыванию нулевого указателя, крайне маловероятно. По крайней мере, падение не было замечено с момента написания этой функции. А ведь с того момента прошло уже полтора года. Чудо.
    
    void ApplyRuleG_657(VivaWalker &walker,
      const BindFunctionName *bind,
      const IntegerVirtualValueArray *pReturnIntegerVirtualValues,
      const PointerVirtualValueArray *pReturnPointerVirtualValues,
      const Ptree *body, const Ptree *bodySrc,
      const Environment *env)
    {
      if (body == nullptr || bodySrc == nullptr)
      {
        VivaAssert(false);
        return;
      }
    
      if (bind == nullptr)
        return;
    
      if (pReturnIntegerVirtualValues == nullptr &&
          pReturnPointerVirtualValues == nullptr)
        return;
    
      ....
    
      size_t integerValueCount = pReturnIntegerVirtualValues->size();
      // Clang: Called C++ object pointer is null

    Указатель pReturnIntegerVirtualValues вполне может быть равен nullptr.

    На первый взгляд, кажется, что ошибка в условии и что следует использовать оператор "||":
    if (pReturnIntegerVirtualValues == nullptr &&
        pReturnPointerVirtualValues == nullptr)

    Но это не так. Условие правильное. Просто нужно перед разыменовыванием указателя проверять, что указатель не нулевой. Если он нулевой, переменной integerValueCount должно быть присвоено значение 0. Корректный вариант:
    size_t integerValueCount =
      pReturnIntegerVirtualValues != nullptr ?
        pReturnIntegerVirtualValues->size() : 0;

    Удивительно. Столько тестов. Прогоны на 90 открытых проектах. Плюс за год мы проверили массу других проектов. И всё равно в коде живёт баг. И ведь проявил бы он себя у нашего важного потенциального клиента.

    Слава статическим анализаторам! Слава Clang!

    Прочее


    Анализатор выявил ещё некоторые ошибки, которые стоит поправить. Описывать их затруднительно, а делать синтетические примеры не хочется. Плюс есть пара предупреждений, которые абсолютно верны, но бесполезны. Там пришлось отключить анализ.

    Например, Clang беспокоился за неинициализированные переменные при использовании функции RunPVSBatchFileMode(). Но дело в том, что пакетный запуска просто не реализован для Linux, и там стоит заглушка. И реализовывать его мы пока не планируем.

    Выводы


    Используйте статические анализаторы в своей работе.

    Я считаю, ядро PVS-Studio крайне протестированным. Тем не менее, статический анализатор Clang нашёл 12 настоящих ошибок. Другие места не ошибки, но они являлись кодом, который пахнет, и я исправил все такие места.

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

    Вот такая получилась статья с рекламой Clang. Но он того заслужил.

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

    И, конечно, я рекомендую использовать в работе анализатор PVS-Studio. Он крайне удобен для использующих Visual C++ [5]. Особенно не забывайте про автоматический анализ, запускающийся после успешной компиляции изменившихся файлов.

    Эта статья на английском


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Checking PVS-Studio with Clang.

    Дополнительные ссылки:


    1. Андрей Карпов. PVS-Studio vs Clang.
    2. Андрей Карпов. Статический анализ следует применять регулярно.
    3. Андрей Карпов. Не зная брода, не лезь в воду. Часть третья (про операции сдвига).
    4. Андрей Карпов. Как статический анализ дополняет TDD.
    5. Андрей Карпов. PVS-Studio для Visual C++.


    Прочитали статью и есть вопрос?
    Часто к нашим статьям задают одни и те же вопросы. Ответы на них мы собрали здесь: Ответы на вопросы читателей статей про PVS-Studio и CppCat, версия 2014. Пожалуйста, ознакомьтесь со списком.
    PVS-Studio
    293.40
    Static Code Analysis for C, C++, C# and Java
    Share post

    Similar posts

    Comments 53

      +15
      А есть версия PVS-Studio и CppCat под Linux? А когда будет?

      К сожалению, нет, извините.

      © www.viva64.com/ru/a/0085/#ID0E3SAE
      Однако вы говорите что у вас есть PVS под Linux. Кому верить? :)
      Тут должен быть анекдот про крестик и трусы
        +5
        К нас нет PVS-Studio под Linux как готового продукта. У нас есть внутренняя command-line версия, собираемая под Linux. Компании могут заключить с нами контракт на адаптацию PVS-Studio под их систему сборки, их редактор и так далее. Никаких публичных версий. Только контрактные работы.
          +8
          Ну, одно дело «нет вообще», а другое — «есть, но за отдельные деньги».
          Просто по предыдущим статьям я думал что даже если сильно захотеть то под linux pvs не собирается.
            +10
            Есть подозрение что «за отдельные деньги» вам соберут PVS Studio и под Mac OS, вопрос в том, сколько ноликов будет у слова «отдельные».
              +2
              Согласен, но есть разница между «даже не начинали это делать» и «оно есть, но надо много заплатить». Вряд-ли заказчик будет ждать пол-года, пока pvs портируют под нужную платформу
              • UFO just landed and posted this here
          0
          Теперь точно есть: PVS-Studio для Linux.
            0
            А для mac os будет? :)
              0
              Возможно, но пока в планы это не вписано.
                0
                Я еще и под МК пишу, интересно было бы попробовать анализатор.
                  0
                  Так почему-бы его и не попробовать тогда? :)
                    0
                    А у меня вся разработка на mac os. Чтобы проверить, это надо толи компьютер с виндой найти, толи виртуалку с линуксом поднять.

                    А расскажите в общих чертах подключить к такому проекту?

                    Там тонкость, что это ОС со своей системой сборки и кросс-компиляция для arm. Собираются проекты из папок внутри example(например https://github.com/contiki-os/contiki/tree/master/examples/ipv6/simple-udp-rpl), и к маленькому файлу с логикой работы подключается все окружение — сетевой стек, ОС, библиотеки.

                    1)Как проверять только свой код(тот, который в папке simple-udp-rpl)? А то, боюсь, утону в предупреждениях со всей операционки. Насколько я понимаю, можно без моего кода проверить, установив параметр «игнорировать существующие ошибки», а потом добавить его?
                    2)Куда в этих мейкфайлах найти, куда вклиниваться с анализатором? Или я усложняю, и это гораздо проще?

                    P.S. Кстати, если интересно проверить «ОС для интернета вещей», то могу поучаствовать, рассказать, как она собирается, дать комментарий по местам найденных ошибок и так далее.
                      0
                      Обсудили в диалоге.
          +140
          А вы сообщили авторам проекта о найденных ошибках?
            +1
            Можно поинтересоваться, как получилось, что clang нашел данные ошибки, несмотря на проверку кода вашим же анализатором?
              +21
              Точно так же, как PVS-Studio находит ошибки в Clang, хотя они сами себя собирают и проверяют. Ссылки на проверки есть в начале статьи. Кстати, мы заодно Clang свежий проверили. Надо будет заметку сделать «PVS-Studio наносит ответный удар».
                +25
                Это еще что. Нет-нет, да и найдется клиент, который спрашивает: «А вы ГАРАНТИРУЕТЕ, что если мы купим PVS-Studio, то у нас в коде не будет ни одной ошибки?». Так и хочется ответить, что если бы так было, то мы бы первые купили себе PVS-Studio.
                  +11
                  Я как-то слышал возражение, мол, ошибку показывает, а как исправить — нет. Не нашелся, что ответить. Так можно и до кнопки «сделай мне хорошо» докатиться.
                    +7
                    Видимо, спрашивал менеджер? наверное, еще и эффективный…
                      +1
                      Да, беседа на тему внедрения и необходимости оной.
                +5
                По поводу ошибки при снятии константности. Я посмотрел ваш код, там нет никакой ошибки, просто лишняя строчка (как в случае с kind). Ну, если не считать за ошибку само снятие константности.
                Для того, чтобы вернуть значение, совершенно не обязательно добавлять к нему константность обратно :)
                  0
                  Да, Вы правы. Лишняя строчка. Содом и Гоморра частично реабилитированы.
                  –22
                  А вы занимаетесь проверкой PVS с помощью самой PVS? Это был бы вполне логичный шаг.

                  К примеру, новый GCC собирается старым GCC, и прочее
                    +11
                    www.viva64.com/ru/a/0085/#ID0EIVAE
                    Будьте внимательны в следующий раз. За этот вопрос уже многих заминусовали, так как статья не первая, и вопрос задавался миллионы раз.
                      +1
                      Да ему даже не надо было FAQ читать: прямо в статье написано «и, конечно, статический анализ с помощью PVS-Studio» в списке методов, используемых для тестирования.
                    +11
                    1. А вы уже работаете над внедрением проверок на указанные типы ошибок?
                    2. И почему PVS-Studio не нашла их раньше? Поиск таких ошибок не был предусмотрен в принципе, или был предусмотрен, но анализатор что-то смутило?
                      +9
                      1. Пока нет, но надеюсь со временем то же самое будет находить и PVS-Studio.

                      2. Нет какого-то манускрипта, где описаны все паттерны ошибок, которые может и должен находить статический анализатор кода. Есть некоторые подборки (MISRA, CERT). Но это малюсенькая часть от того, что можно диагностировать. Вдобавок, стандарт Си++ эволюционирует.

                      Поскольку нет единого идеала, каждый реализует те диагностики, которые кажутся ему наиболее интересными. Часто одни и те-же паттерны лежат на поверхности и их поиск независимо реализуется в разных анализаторах. Но в целом — «кто во что горазд».

                      PVS-Studio хорошо ищет опечатки. Clang хорошо ищет неинициализированные переменные, нулевые указатели.

                      Я понимаю, что это грустная картина. Хочется взять один инструмент и использовать именно его. Но пока такого нет. И пока не предвидится. Некоторые выбрали объект для поклонения, считая, что молясь на него, они получают хороший код. Я имею в виду MISRA, который для некоторых является обязательным. Но это ведь тоже просто маленький набор кем-то выбранных правил. Он внушает спокойствие, но ничего не гарантирует и даже не обещает.

                      Надо просто смериться и использовать какой-то набор инструментов, которые дополняют друг друга. Тем более, не так уж все страшно, как я расписал. Например, у PVS-Studio и Cppcheck совпадает по диагностикам на 6%. Я считаю, что это очень хорошее число. Есть паттерны, которые все посчитали важными и реализовали их. И со временем, ситуация будет только улучшаться. Область статического анализа очень молода. Конечно, lint существует давно. Но, да простят меня любители, lint по нынешнем временам простенькая утилита.

                      Отвечу ещё на одну крамольную мысль. Например, я возьму 5 известных статических анализаторов. Но все они вместе могут находить только 2% ошибок, которые может обнаруживать статический анализ. Быть может лучше вообще не пользоваться статическими анализаторами?

                      Неправильный ход мыслей. Во-первых, никто не отменяет правило 80/20. Т.е. поддержка 2% паттернов может обеспечить выявление, скажем 50% ошибок, которые чаще всего допускают люди. Отличны результат. Во-вторых, неважно сколько найдено или нет. Главное, анализатор даёт выигрыш больше, чем он стоит.
                      0
                      Кстати, у вас траблы с 5.18 версией когда проверяются *.i (-save-temps) файлы приготовленные gcc в больших проектах, там где можно найти сгенерированные *.c файлы которые могли удалиться после конфигурирования или компилирования. Выкидывает ексепшион о непригодном символе в пути к файлу, после чего 50/50 падает весь анализатор или продолжает работу. 5.17 — все ок.
                        0
                        Прошу написать и пообщаться с нами в поддержке.
                          +1
                          Напишу завтра, постараюсь с подробностями, но я проверяю достаточно хитрым способом.

                          Сейчас, к примеру, наконец то скопировались все *.i файлы ядра линукса(с практически всеми, которые были доступны в конфиге, дровами) из виртуалки на винду.
                          30%(из ~10400 файлов) проверило уже. Из того что явно бросается в глаза сейчас нашел вот что, копи-паст он такой :):

                          bchahce:
                          static inline bool bch_bkey_equal_header(const struct bkey *l, const struct bkey *r) {
                          return (KEY_DIRTY(l) == KEY_DIRTY( r ) &&
                          KEY_PTRS(l) == KEY_PTRS( r ) &&
                          KEY_CSUM(l) == KEY_CSUM(l)); // <===
                          }


                          btrfs:
                          if (!(fs_info->workers && fs_info->delalloc_workers &&
                          fs_info->submit_workers && fs_info->flush_workers &&
                          fs_info->endio_workers && fs_info->endio_meta_workers &&
                          fs_info->endio_meta_write_workers &&
                          fs_info->endio_write_workers && fs_info->endio_raid56_workers &&
                          fs_info->endio_freespace_worker && fs_info->rmw_workers &&
                          fs_info->caching_workers && fs_info->readahead_workers &&
                          fs_info->fixup_workers && fs_info->delayed_workers && // <===
                          fs_info->fixup_workers && fs_info->extent_workers && // <===
                          fs_info->qgroup_rescan_workers)) {
                          err = -ENOMEM;
                          goto fail_sb_buffer;
                          }


                          driver — rtl8821ae:
                          void rtl8821ae_dm_set_tx_ant_by_tx_info()
                          if (
                          (rtlefuse->antenna_div_type == CG_TRX_HW_ANTDIV) ||
                          (rtlefuse->antenna_div_type == CG_TRX_HW_ANTDIV)) {
                          SET_TX_DESC_TX_ANT(pdesc, pfat_table->antsel_a[mac_id]);
                          }


                          Это только по 501 прошелся и за что глаз зацепился, проверка достаточно долгая.Помню вы говорили о том что ядро достаточно протестировано и проверено десятками анализаторов, так и есть, но вот его модули, не совсем.
                            +1
                            Самая проблема это очень большое кол-во предупреждений на макросы, а в строенном в standalone редакторе макросы просто нереально найти и просмотреть, вот если бы запилили фичу с показом развернутого макроса было бы просто класс.
                              +1
                              Но это фактически потребует от нас сделать собственную реализацию intelliSence, что, как вы понимаете, является не совсем тривиальной задачей, особенно учитываю необходимые для этого трудо-затраты, при весьма специфичном контингенте пользователей конкретно у Standalone.

                              Однако, никто не мешает вам уже сейчас использовать текстовый редактор (и intelliSense) из той же Visual Studio — создайте пустой Visual C++ проект, добавьте в него все необходимые заголовки \ исходники и откройте plog отчёт в плагине PVS-Studio. IntelliSence распарсит, хоть и не на 100%, какую то часть исходников, и у вас будет работать переход по include'ам \ определениям типов и т.п.

                              Если у вас нет Visual Studio, вы можете всё это сделать и в бесплатной Express версии, правда вы не сможете открыть там отчёт из PVS-Studio.
                                +2
                                Так и делаю. Хорошо что еще в 2013 студии в проект можно перетаскивать просто папки, а не как в 2010 отдельными файлами.
                                  0
                                  Для мимопроходящих:
                                  Если у вас нет Visual Studio, вы можете всё это сделать и в бесплатной Express версии, правда вы не сможете открыть там отчёт из PVS-Studio.
                                  Есть уже Visual Studio Community 2013.
                                0
                                Забыл написать, проверяю свеженький 3.16
                                0
                                В общем, появилось время еще раз все перепроверить на наличие бага о котором написал, пришлось скачать свежий инсталятор с viva64.com и обновить 5.17. На тех проектах с препроцессорными файлами(*.i), которые у меня хранятся в архиве, 5.18 версия отработала нормально, как и 5.17. Странно, когда как на двух из них точно выдавало ошибку в процессе анализирования еще неделю назад. Вы сменили билд? Но я еще попробую над свежим kernel 3.17-rc1 на будущей недели, все-таки это занимает очень много времени и иногда приходится до 3 ночи сидеть. Если симптомы будут — обязательно напишу.
                              +4
                              Блин, ребята, спасибо за все, что делаете. Надеюсь, что когда-нибудь ваши продукты будут доступны и в Linux.
                              +3
                              Не понял вот это:
                              int A, B;
                              bool getA, getB;
                              Get(A, getA, B, getB);
                              int TmpA = A; // Clang: Assigned value is garbage or undefined
                              int TmpB = B; // Clang: Assigned value is garbage or undefined
                              if (getA)
                                Use(TmpA);
                              if (getB)
                                Use(TmpB);
                              

                              Функция Get() может инициализировать переменные A и B. Инициализировала она их или нет, она отмечает в переменных getA, getB

                              Значит, Get принимает все параметры по ссылке и может поменять каждый? TmpA и TmpB передаются в Use тоже по ссылке и она может их менять? Иначе для чего было вводить эти переменные?
                              Не выглядел бы код значительно проще, если бы Get был не void, а возвращал bool?
                              int A, B;
                              
                              if (TryGet(A)) {
                                  int TmpA = A;
                                  Use(TmpA);
                              }
                              
                              if (TryGet(B)) {
                                  int TmpB = B;
                                  Use(TmpB);
                              }
                                +6
                                Давайте не будем обсуждать и упрощать псевдокод. :)
                                –5
                                Наверно эту мысль уже писали, но все-таки бОльшую часть проблем вызывают ошибки в логике работы алгоритмов, а не ошибки статического характера. Статические ошибки обычно легко повторяются и очень просто исправляются.
                                  +5
                                  Мифы о статическом анализе. Миф второй – профессиональные разработчики не допускают глупых ошибок.

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

                                  И как без статического анализа найти это?
                                  www.viva64.com/ru/examples/V597/

                                    0
                                    Согласен, в случае с С++ это похоже на правду, т.к. это очень небезопасный язык. По поводу того же «выхода за границу массива», на Delphi, например, есть опция компилятора «Range checking», которая добавляет проверки на каждое общение к arr[i], что 0<=i<длины массива и если это правило нарушается, генерирует осмысленное исключение.
                                    В целом, мы стараемся использовать т.н. «защитное программирование», чтобы обнаружить ошибки как можно раньше. Это означает, что в коде много Assert'ов, много перепроверок, проверок входных параметров, различных верификаций, которые выполняются всегда во время работы программы. Причем мы их не отключаем и в релизах. Как правило на скорости работы программы это никак не сказывается: время, затрачиваемое на эти проверки обычно составляет менее 1% времени исполнения остального кода. При этом время на отлов ошибок и разборки со странными багами экономится в разы.

                                    PS По поводу минусов выше: я согласен, что внести при рефакторингах или просто по невнимательности тупую ошибку очень очень легко. И иногда она проявляется гораздо позже, бывает даже через месяцы, или в каких-то особых условиях. В этом плане статический анализ безусловно важен и полезен и у нас даже есть свой инструмент для него, правда анализирует он код Delphi и ориентирован он больше на ошибки, наиболее часто наблюдаемые именно в наших проектах (т.е. с учетом нашей статистики ошибок). Однако ошибок, возникающих из-за неверных алгоритмов или от невнимательности программиста, который забыл сделать какую-то нужную осмысленную логическую проверку (т.е. просто не написал какой-то нужный код, т.е. анализировать анализатором тут нечего), всё-таки на порядок больше.
                                      +1
                                      Для C есть различные решения вроде -fsanitize=bounds (для случаев, когда размеры массива могут быть определены статически) (clang), -fsanitize=memory и -fsanitize=address (проверяют чтение из неинициализированной памяти (первый) и выход за границы массива и различные другие ошибки вроде использования памяти после освобождения (второй)). И ещё есть valgrind и аналоги. Всё‐таки, C/C++ — это языки программирования с огромным количеством различных программ для разработчиков, проверяющих всё, что только можно. Delphi такое и не снилось (хотя тот же valgrind можно использовать для любой программы, статические анализаторы и sanitizer’ы clang’а и gcc — нет).

                                      Другой вопрос, что внедрение всех этих статических анализаторов, sanitizer’ов и отладчиков, с последующим анализом и исправлением, гораздо более трудоёмкий процесс, чем включение нескольких опций компилятора.
                                  +1
                                  По поводу двух последних багов, которые не просто UB а самое настоящее падение.
                                  Вы пробовали написать тесты на эти примеры?

                                  Кажется что либо возможно написать такой С++ код который соответствует необходимым условиям для креша — и тогда он должен быть в тестах — либо анализатор (как и авторы кода!) запутался в логике и не понял почему условия недостижимы.
                                    0
                                    Тесты написать на эти функции не пробовал. Крайне сложно. Нужно подготовить в памяти дерево разбора, напихать в определённые структуры информацию о переменных, типах и так далее. Юнит тесты построены на уровне подсовывания анализатору фрагментов кода, на которых проверяется, что он находит нужную ошибку или наоборот не выдаёт ложное срабатывание. Однако, там работает очень много подсистем, которые могут оказывать влияние на расмотренные функции.

                                    Например, как мы видим здесь, что-то отсеивает ситуации, когда об объекте что-то не известно и нулевой указатель никогда не появляется. Однако, это всё равно ошибка. Ведь те внешние механизмы могут поменяться или изменятся условия, когда вызывается ошибочная функция. Так что действительно, ни я, ни анализатор не может отследить почему условия недостижимы. Это везение очень хрупко и конечно нужно править код.
                                    –11
                                    Немного досадно, что к тексту статьи, пропагандирующей применение статического анализа кода, не применялся статический анализ орфографии хотя-бы.
                                      +5
                                      Особенно досадно, когда замечания пишут не в личную почту. Да и забавно, когда при этом комментатор в своих статьях пишет «Вобщем» или «всего-лишь».
                                        –9
                                        Приятно, спустя столько лет, обрести столь внимательного читателя. Ошибки в заметке исправил, спасибо!

                                        Твоя очередь =)

                                        +3
                                        «хотя-бы»
                                        –3
                                        Хотелось бы уточнить про функцию GetBitCountForRepresntValueLoopMethod()
                                        Находится ли эта функция в заголовочном файле или она статическая? Судя по отсутствию ключевого слова static, объявление функции вполне может быть находиться в заголовочном файле.
                                        Если функция в заголовочном файле, то кто знает, с какими аргументами её могут запустить. Например, GetBitCountForRepresntValueLoopMethod(100, 100) должен привести к переполнению.
                                          0
                                          Обыкновенная функция в *.cpp файле.

                                          >… то кто знает…
                                          Я знаю. Нет никакой ошибки. Ложное срабатывание.
                                          0
                                          Присоединяюсь к просящим выпустить версию под линукс, тем более Вы не скрываете, что она у Вас готова. Вашему бизнесу, основанному на программистах под Windows, это точно не повредит, так как нельзя просто взять и проверить линуксовой версией Windows-only проект.

                                          Приведу пример проекта, где так и поступили. Есть такой замечательный статификатор программ под линукс, Ermine. У них тоже закрытый код и программа платная, но опробовать ограниченную версию можно без проблем. Бинарники, сделанные этой версией, запускаются первые 30 дней, потом перестают работать. Кроме того, действуют скидки для разработчиков свободного софта. А начиналось у них всё с аналогичной программы под Windows (BoxedApp).

                                          Only users with full accounts can post comments. Log in, please.