Андрей Карпов

01 Ноя 2010

Теги:

#Cpp

Разница в подходах анализа кода компилятором и выделенным инструментом

01 Ноя 2010

Автор: Андрей Карпов

Notepad++
Статический анализ в компиляторе
Внешние статические анализаторы кода
Некоторые числа
UPDATE: Примечание

У компилятора и сторонних инструментов статического анализа кода есть общая задача - выявление опасных фрагментов кода. Однако существует существенная разница в том, анализ какого типа они осуществляют. Я попробую на примере компилятора Intel C++ и анализатора PVS-Studio продемонстрировать различия подходов, и пояснить, чем они вызваны.

В качестве испытуемого на этот раз выступит проект Notepad++ версии 5.8.2.

Notepad++

Вначале о выбранном проекте. Notepad++ это свободный и бесплатный редактор исходного кода, поддерживающий большое количество языков, а также замена стандартному Блокноту. Он работает в среде Microsoft Windows и выпускается под лицензией GPL. Проект мне приглянулся тем, что он написан на языке Си++ и имеет маленький размер - 73000 строки кода. А самое главное, это достаточно аккуратный проект, о чем говорит ключ /W4 в настройках проекта и ключ /WX, заставляющий трактовать каждое предупреждение как ошибку.

Статический анализ в компиляторе

Теперь рассмотрим анализ проекта с точки зрения компилятора и отдельного специализированного инструмента. Компилятор всегда тяготит к предупреждениям, которые можно выдать, обработав только очень маленький локальный фрагмент кода. Такое предпочтение является следствием того, что на компилятор наложены очень жесткие требования по производительности. Не случайно ведь существуют инструменты распределенной сборки проекта. Время компиляции средних и больших проектов является существенным фактором, оказывающим влияние на выбор методики разработки. Поэтому если будет существовать возможность выжать 5% прироста производительности у компилятора, это будет осуществлено.

Подобная оптимизация делает компилятор более монолитным и на самом деле там вряд ли уже четко выделяются такие этапы как препроцессирование, построение AST, генерация кода. Например, по косвенным признакам я могу утверждать, что алгоритм препроцессора, который использует Visual C++ при компиляции и при генерации препроцессированных "*.i" файлов различен. Также компилятору вовсе не нужно и даже вредно хранить AST дерево целиком. Как только код для определенных узлов сгенерирован и более эти узлы не нужны, то они будут сразу уничтожены. В процессе компиляции AST может вообще никогда не существовать целиком. Это не нужно. Разобрали кусочек кода, сгенерировали код, пошли дальше. Это экономит объем используемой памяти и кеша, а, следовательно, увеличивает скорость.

Результатом такого подхода является "локальность" предупреждений. Компилятор сознательно экономит на различных структурах, которые могли бы помочь обнаружить более высокоуровневые ошибки. Посмотрим на практике, какие локальные предупреждения выдаст Intel C++ для проекта Notepad++. Напомню, проект Notepad++ собирается компилятором Visual C++ без предупреждений с ключом /W4. Компилятор Intel C++ естественно будет иметь другой набор предупреждений, а также я выставил специфичный ключ /W5 [Intel C++]. Плюс я посмотрю на то, что компилятор Intel C++ называет "remark".

Посмотрим, какого рода сообщения мы получили от Intel C++. Вот он нашел четыре однотипных ошибки при работе с функцией CharUpper (СМ. ПРИМЕЧАНИЕ В КОНЦЕ). Обратите на "локальность" диагностики данной ошибки - просто найдено крайне опасное приведение типа. Рассмотрим соответствующий фрагмент кода:

wchar_t *destStr = new wchar_t[len+1];
...
for (int j = 0 ; j < nbChar ; j++)
{
  if (Case == UPPERCASE)
    destStr[j] =
      (wchar_t)::CharUpperW((LPWSTR)destStr[j]);
  else
    destStr[j] =
      (wchar_t)::CharLowerW((LPWSTR)destStr[j]);
}

Здесь присутствуют странные приведения типов. Компилятор Intel C++ сообщает: "#810: conversion from "LPWSTR={WCHAR={__wchar_t} *}" to "__wchar_t" may lose significant bits". Посмотрим на прототип функции CharUpper.

LPTSTR WINAPI CharUpper(
  __inout  LPTSTR lpsz
);

Функция работает со строкой, а вовсе не с отдельными символами. Здесь же символ конвертируется в указатель, и некая область памяти по этому указателю модифицируется. Ужас.

Но, правда, на этом ужасы обнаруженные Intel C++ пожалуй заканчиваются. Все остальное намного скучнее и скорее являются неаккуратным кодом, чем кодом, который вызовет ошибку. Но все же рассмотрим некоторые другие предупреждения.

Выдано большое количество сообщений #1125:

"#1125: function "Window::init(HINSTANCE, HWND)" is hidden by "TabBarPlus::init" -- virtual function override intended?"

Это не ошибки, а неудачное именование функций. Нам интересно это сообщение другим. Хотя оно вроде бы задействует несколько классов для проверки, специальные данные здесь не запоминаются. Компилятору все равно надо хранить различную информацию о базовых классах. Именно поэтому данная диагностика реализована.

Следующий пример. Сообщение "#186: pointless comparison of unsigned integer with zero" выдается на бессмысленные сравнения:

static LRESULT CALLBACK hookProcMouse(
  UINT nCode, WPARAM wParam, LPARAM lParam)
{
  if(nCode < 0)
  {
    ...
    return 0;
  }
...
}

Условие "nCode < 0" всегда ложно. Хороший пример хорошей локальной диагностики. Так вполне можно обнаружить ошибку.

Рассмотрим последнее предупреждение Intel C++ и хватит. Думаю идея "локальности" уже понятна.

void ScintillaKeyMap::showCurrentSettings() {
  int i = ::SendDlgItemMessage(...);
  ...
  for (size_t i = 0 ; i < nrKeys ; i++)
  {
    ...
  }
}

Здесь вновь нет ошибки, Просто не очень удачное именование переменных. Вначале переменная "i" имеет тип "int". Затем в операторе "for()" объявляется новая переменная "i" типа "size_t" и используется для иных целей. Компилятор в момент объявления "size_t i" знает, что уже есть переменная с таким именем и выдает предупреждение. Опять-таки это не потребовало от компилятора хранения каких-то дополнительных данных. Он все равно должен помнить, что до конца тела функции, доступна переменная "int i".

Внешние статические анализаторы кода

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

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

Посмотрим, что интересного найдет в Notepad++ статический анализатор PVS-Studio. Замечу, что я использую экспериментальную версию, которая пока не доступна для скачивания. Мы представим бесплатный набор правил общего назначения через 1-2 месяца в PVS-Studio версии 4.00.

Естественно, как и Intel C++ анализатор PVS-Studio находит ошибки, которые можно отнести к "локальным". Первый пример:

bool _isPointXValid;
bool _isPointYValid;
bool isPointValid() {
  return _isPointXValid && _isPointXValid;
};

Анализатор PVS-Studio сообщает: "V501: There are identical sub-expressions to the left and to the right of the '&&' operator: _isPointXValid && _isPointXValid".

Думаю, суть ошибки очевидна, и подробнее на ней останавливаться не будем. Диагностика "локальна", так как проверку можно сделать в процессе анализа одного выражения.

Еще одна локальная ошибка, приводящая к неполной очистке массива _iContMap:

#define CONT_MAP_MAX 50
int _iContMap[CONT_MAP_MAX];
...
DockingManager::DockingManager()
{
  ...
  memset(_iContMap, -1, CONT_MAP_MAX);
  ...
}

Выдано предупреждение "V512: A call of the memset function will lead to a buffer overflow or underflow". Корректный вариант кода:

memset(_iContMap, -1, CONT_MAP_MAX * sizeof(int));

А вот теперь перейдем к более интересным вещам. Код, где надо одновременно проанализировать сразу две ветки, чтобы суметь заподозрить неладное:

void TabBarPlus::drawItem(
  DRAWITEMSTRUCT *pDrawItemStruct)
{
...

  if (!_isVertical)
    Flags |= DT_BOTTOM;
  else
    Flags |= DT_BOTTOM;
...
}

PVS-Studio сообщает здесь: "V523: The 'then' statement is equivalent to the 'else' statement". Если посмотреть на код по соседству, то можно сделать вывод, что на самом деле автор хотел написать так:

if (!_isVertical)
  Flags |= DT_VCENTER;
else
  Flags |= DT_BOTTOM;

А теперь наберитесь мужества. Вас ждет испытание в виде следующего фрагмента кода:

void KeyWordsStyleDialog::updateDlg() 
{
  ...
  Style & w1Style =
    _pUserLang->_styleArray.getStyler(STYLE_WORD1_INDEX);
  styleUpdate(w1Style, _pFgColour[0], _pBgColour[0],
    IDC_KEYWORD1_FONT_COMBO, IDC_KEYWORD1_FONTSIZE_COMBO,
    IDC_KEYWORD1_BOLD_CHECK, IDC_KEYWORD1_ITALIC_CHECK,
    IDC_KEYWORD1_UNDERLINE_CHECK);

  Style & w2Style =
    _pUserLang->_styleArray.getStyler(STYLE_WORD2_INDEX);
  styleUpdate(w2Style, _pFgColour[1], _pBgColour[1],
    IDC_KEYWORD2_FONT_COMBO, IDC_KEYWORD2_FONTSIZE_COMBO,
    IDC_KEYWORD2_BOLD_CHECK, IDC_KEYWORD2_ITALIC_CHECK,
    IDC_KEYWORD2_UNDERLINE_CHECK);

  Style & w3Style =
    _pUserLang->_styleArray.getStyler(STYLE_WORD3_INDEX);
  styleUpdate(w3Style, _pFgColour[2], _pBgColour[2],
    IDC_KEYWORD3_FONT_COMBO, IDC_KEYWORD3_FONTSIZE_COMBO,
    IDC_KEYWORD3_BOLD_CHECK, IDC_KEYWORD3_BOLD_CHECK,
    IDC_KEYWORD3_UNDERLINE_CHECK);

  Style & w4Style =
    _pUserLang->_styleArray.getStyler(STYLE_WORD4_INDEX);
  styleUpdate(w4Style, _pFgColour[3], _pBgColour[3],
    IDC_KEYWORD4_FONT_COMBO, IDC_KEYWORD4_FONTSIZE_COMBO,
    IDC_KEYWORD4_BOLD_CHECK, IDC_KEYWORD4_ITALIC_CHECK,
    IDC_KEYWORD4_UNDERLINE_CHECK);
  ...
}

Можно сказать, что я горд за наш анализатор PVS-Studio, который смог найти здесь ошибку. Думаю, вы ее вряд ли заметили и просто пропустите весь фрагмент кода, ожидая пояснения. Такой код практически не поддается обзору (code review). А вот статический анализатор терпелив и педантичен: "V525: The code containing the collection of similar blocks. Check items '7', '7', '6', '7' in lines 576, 580, 584, 588".

Сокращу текст, чтобы выделить интересное:

styleUpdate(...
  IDC_KEYWORD1_BOLD_CHECK, IDC_KEYWORD1_ITALIC_CHECK,
  ...);
styleUpdate(...
  IDC_KEYWORD2_BOLD_CHECK, IDC_KEYWORD2_ITALIC_CHECK,
  ...);
styleUpdate(...
  IDC_KEYWORD3_BOLD_CHECK, IDC_KEYWORD3_BOLD_CHECK,
  ...);
styleUpdate(...
  IDC_KEYWORD4_BOLD_CHECK, IDC_KEYWORD4_ITALIC_CHECK,
  ...);

Код, скорее всего, писался методикой Copy-Paste. Результатом стало использование IDC_KEYWORD3_BOLD_CHECK вместо IDC_KEYWORD3_ITALIC_CHECK. Предупреждение выглядит несколько странным, говоря о числах '7', '7', '6', '7'. К сожалению, выдать более понятное сообщение сложно. Эти числа берутся из макросов вида:

#define IDC_KEYWORD1_ITALIC_CHECK (IDC_KEYWORD1 + 7)
#define IDC_KEYWORD3_BOLD_CHECK (IDC_KEYWORD3 + 6)

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

Некоторые числа

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

При анализе проекта Notepad++, инструмент PVS-Studio выдал только 10 предупреждений. Из них 4 сообщения указали на настоящие ошибки. Результат скромен, но статический анализ общего назначения в PVS-Studio только начал свое развитие. Со временем он станет одним из лучших.

При анализе проекта Notepad++, компилятор Intel C++ выдал 439 предупреждений и 3139 примечаний (remarks). Я не знаю, сколько действительно из них указывают на настоящие ошибки. Из того, на что хватило сил мне посмотреть, я увидел только 4 настоящих ошибки связанных с CharUpper (см. описание выше).

3578 сообщений это слишком много, чтобы внимательно изучить каждое из них. Получается, что компилятор предлагает обратить внимание на каждую двадцатую сточку в программе (73000 / 3578 = 20). Это несерьезно. В случае анализатора общего назначения следует по возможности отсекать все лишнее.

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

UPDATE: Примечание

Оказывается я написал здесь неправду. В примере с CharUpperW ошибки нет. И к сожалению меня никто не поправил. Сам заметил, когда решил реализовать аналогичное правило в PVS-Studio.

Дело в том, что CharUpperW может работать как со строкой, так и с отдельными символами. Если старшая часть указателя нулевая, то считается что это не указатель, а символ. Интерфейс WIN API в этом конечно месте опечалил своей кривизной, но код в Notepad++ написан корректно.

Кстати, теперь получается, что Intel C++ вообще не нашел ни одной ошибки.

#Cpp