Проверка PVS-Studio с помощью 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++.

Автор: Andrey2008

Источник