Я регулярно проверяю различные open-source проекты, чтобы продемонстрировать возможности статического анализатора кода PVS-Studio (C, C ++, C #). Пришло время проверить компилятор GCC. Несомненно, GCC - очень качественный и хорошо протестированный проект, поэтому обнаружение в нем ошибок - уже большое достижение для инструмента. К счастью, PVS-Studio справился с этой задачей. Никто не застрахован от опечаток или небрежности. Поэтому PVS-Studio может стать для вас дополнительной линией защиты на фронте бесконечной войны с ошибками.
GCC
Коллекция компиляторов GNU (обычно сокращенно GCC) - это набор компиляторов для разных языков программирования, разработанный в рамках проекта GNU. GCC - это бесплатное программное обеспечение, распространяемое фондом свободного программного обеспечения в соответствии с условиями GNU GPL и GNU LGPL, и является ключевым компонентом инструментальной цепочки GNU. Проект написан на C и C ++.
Компилятор GCC имеет отличные встроенные средства диагностики, которые помогают обнаруживать многие проблемы на этапе компиляции. Конечно, GCC построен на GCC и, таким образом, может находить ошибки в собственном коде. Дополнительно исходный код GCC проверяется анализатором Coverity. В целом, я думаю, что многие энтузиасты также проверяли это с помощью других анализаторов и других инструментов. Это усложняет поиск ошибок в GCC для PVS-Studio.
Мы использовали транк-версию из git-репозитория: (git) commit 00a7fcca6a4657b6cf203824beda1e89f751354b svn + ssh: //gcc.gnu.org/svn/gcc/trunk@238976
Примечание. Публикация статьи немного запоздала, и, возможно, некоторые ошибки уже исправлены. Однако в этом нет ничего страшного: в код постоянно попадают новые ошибки, а старые исчезают. Главное, в статье показано, что статический анализ может помочь программистам обнаруживать ошибки после того, как они попадают в код.
Предвидя обсуждение
Как я уже сказал в начале, считаю проект GCC качественным. Я уверен, что многие захотят с этим поспорить. В качестве примера приведу цитату из Википедии на русском языке (в переводе):
Некоторые разработчики OpenBSD Тео де Раадт и Отто Моербик критикуют GCC, говоря, что «gcc становится примерно на 5–6% медленнее с каждым выпуском, имеет новые ошибки, генерирует дрянной код и сводит нас с ума».
На мой взгляд, эти утверждения безосновательны. Да, возможно, в коде GCC слишком много макросов, что затрудняет его чтение. Но я не могу согласиться с утверждением о том, что он глючит. Если бы в GCC были ошибки, вообще ничего бы не работало. Подумайте только о количестве программ, которые успешно компилируются с его помощью и, следовательно, хорошо работают. Создатели GCC проделали большую, сложную работу с профессионализмом. Мы должны их поблагодарить. Рад, что могу протестировать работу PVS-Studio на таком качественном проекте.
Тем, кто говорит, что код Clang все же намного лучше, могу напомнить: PVS-Studio также обнаружил в нем ошибки: 1, 2.
PVS-Studio
Я проверил код GCC с помощью альфа-версии PVS-Studio для Linux. В середине сентября 2016 года мы планируем предоставить бета-версию анализатора тем программистам, которые сочтут ее полезной. Инструкцию о том, как стать первым, кто попробует бета-версию PVS-Studio для Linux на своем проекте, вы можете найти в статье PVS-Studio признается в любви к Linux.
Если вы читаете эту статью намного позже сентября 2016 года и хотите попробовать PVS-Studio для Linux, то предлагаю посетить страницу продукта: http://www.viva64.com/ru/pvs-studio/
Результаты анализа
Мы перешли к самой интересной части статьи, которую с нетерпением ждут наши постоянные читатели. Давайте посмотрим на те фрагменты кода, где анализатор обнаружил ошибки или действительно подозрительный код.
К сожалению, я не могу предоставить разработчикам полный аналитический отчет. На данный момент слишком много мусора (ложных срабатываний) из-за того, что анализатор еще не готов к встрече с миром Linux. Нам предстоит еще много работы по снижению количества ложных срабатываний для типовых конструкций. Я попытаюсь объяснить на простом примере. Многие средства диагностики не должны выдавать предупреждения для выражений, связанных с макросами assert. Эти макросы иногда пишутся очень творчески, поэтому стоит научить анализатор не обращать на них внимания. Дело в том, что макрос assert может быть определен по-разному, поэтому мы должны обучить PVS-Studio всем типичным вариантам.
Вот почему я прошу разработчиков GCC подождать, пока выйдет бета-версия. Не хотелось бы портить впечатление отчетом, созданным по недоработанной версии.
Классика (Копировать-Вставить)
Начнем с наиболее частой и классической ошибки, которую выявляет диагностика V501. Обычно эти ошибки возникают из-за небрежности при копировании-вставке кода или являются просто опечатками при создании нового кода.
static bool
dw_val_equal_p (dw_val_node *a, dw_val_node *b)
{
....
case dw_val_class_vms_delta:
return (!strcmp (a->v.val_vms_delta.lbl1,
b->v.val_vms_delta.lbl1)
&& !strcmp (a->v.val_vms_delta.lbl1,
b->v.val_vms_delta.lbl1));
....
}
Предупреждение PVS-Studio: V501 Слева и справа от него есть идентичные подвыражения '! strcmp (a- ›v.val_vms_delta.lbl1, b-› v.val_vms_delta.lbl1) Оператор. dwarf2out.c 1428
Ошибки сразу увидеть сложно, здесь стоит присмотреться. Вот почему ошибка не была обнаружена при проверке кода и рефакторинге.
Функция strcmp дважды сравнивает одни и те же строки. Мне кажется, надо было сравнивать не члены класса lbl1, а lbl2. Тогда правильный код мог бы выглядеть так:
return (!strcmp (a->v.val_vms_delta.lbl1,
b->v.val_vms_delta.lbl1)
&& !strcmp (a->v.val_vms_delta.lbl2,
b->v.val_vms_delta.lbl2));
Следует отметить, что код, представленный в статье, немного выровнен, чтобы не занимать слишком много места по оси абсцисс. На самом деле код выглядит так:
Этой ошибки можно избежать, используя выравнивание «табличного» кода. Например, ошибку будет легче заметить, если вы отформатируете код следующим образом:
Я подробно рассказывал об этом подходе в электронной книге Главный вопрос программирования, рефакторинга и всего остального (см. Главу N13: Форматирование в виде таблиц). Я рекомендую всем, кому небезразлично качество кода, взглянуть на эту книгу.
Давайте посмотрим на еще одну ошибку, которая, я уверен, возникла из-за Copy-Paste:
const char *host_detect_local_cpu (int argc, const char **argv)
{
unsigned int has_avx512vl = 0;
unsigned int has_avx512ifma = 0;
....
has_avx512dq = ebx & bit_AVX512DQ;
has_avx512bw = ebx & bit_AVX512BW;
has_avx512vl = ebx & bit_AVX512VL; // <=
has_avx512vl = ebx & bit_AVX512IFMA; // <=
....
}
Предупреждение PVS-Studio: V519 Переменной has_avx512vl дважды подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 500, 501. driver-i386.c 501
Дважды подряд в переменную has_avx512vl записываются разные значения. Это не имеет смысла. Я просмотрел код и нашел переменную has_avx512ifma. Скорее всего, его следует инициализировать выражением ebx & bit_AVX512IFMA. Тогда правильный код должен быть следующим:
has_avx512vl = ebx & bit_AVX512VL; has_avx512ifma = ebx & bit_AVX512IFMA;
Опечатка
Я продолжу проверять вашу внимательность. Посмотрите на код и попытайтесь найти ошибку, не обращая внимания на предупреждение ниже.
static bool
ubsan_use_new_style_p (location_t loc)
{
if (loc == UNKNOWN_LOCATION)
return false;
expanded_location xloc = expand_location (loc);
if (xloc.file == NULL || strncmp (xloc.file, "\1", 2) == 0
|| xloc.file == '\0' || xloc.file[0] == '\xff'
|| xloc.file[1] == '\xff')
return false;
return true;
}
Предупреждение PVS-Studio: V528 Странно, что указатель на тип char сравнивается со значением \ 0. Наверное имелось ввиду: * xloc.file == ‘\ 0’. ubsan.c 1472
Программист случайно забыл разыменовать указатель в выражении xloc.file == ‘\ 0’. В результате указатель просто сравнивается с 0, т.е. с NULL. Это не дает никакого эффекта, потому что такая проверка уже была сделана ранее: xloc.file == NULL.
Хорошо то, что программист записал нулевой терминал как \ 0. Это помогает нам быстрее понять, что здесь есть ошибка и как ее следует исправить. Я также писал об этом в книге (см. Главу N9: Используйте литерал \ 0 для терминального нулевого символа).
Правильный вариант кода:
if (xloc.file == NULL || strncmp (xloc.file, "\1", 2) == 0
|| xloc.file[0] == '\0' || xloc.file[0] == '\xff'
|| xloc.file[1] == '\xff')
return false;
Хотя, давайте еще больше улучшим код. Я рекомендую форматировать выражение так:
if ( xloc.file == NULL
|| strncmp (xloc.file, "\1", 2) == 0
|| xloc.file[0] == '\0'
|| xloc.file[0] == '\xff'
|| xloc.file[1] == '\xff')
return false;
Обратите внимание: теперь, если вы сделаете ту же ошибку, шанс ее заметить немного выше:
if ( xloc.file == NULL
|| strncmp (xloc.file, "\1", 2) == 0
|| xloc.file == '\0'
|| xloc.file[0] == '\xff'
|| xloc.file[1] == '\xff')
return false;
Возможное разыменование нулевого указателя
Эту часть также можно было бы назвать «Пример номер одна тысяча, почему макросы плохи». Я действительно не люблю макросы и всегда призываю людей по возможности избегать их использования. Макросы затрудняют чтение кода, вызывают ошибки и усложняют работу статических анализаторов. Насколько я могу судить, из краткого взаимодействия с кодом GCC, авторы - большие поклонники макросов. Я действительно устал смотреть, до чего расширяются макросы, и, возможно, пропустил довольно много интересных ошибок. Признаюсь, временами ленился. Но все же продемонстрирую пару ошибок, связанных с макросами.
odr_type
get_odr_type (tree type, bool insert)
{
....
odr_types[val->id] = 0;
gcc_assert (val->derived_types.length() == 0);
if (odr_types_ptr)
val->id = odr_types.length ();
....
}
Предупреждение PVS-Studio: V595 Указатель odr_types_ptr использовался до того, как он был проверен на соответствие nullptr. Проверить линии: 2135, 2139. ipa-devirt.c 2135
Вы видите здесь ошибку? Думаю, что нет, и предупреждение анализатора не очень помогает. Дело в том, что odr_types - это не имя переменной, а макрос, который объявлен следующим образом:
#define odr_types (*odr_types_ptr)
Если развернуть макрос и удалить все, что не имеет отношения к коду, мы получим следующее:
(*odr_types_ptr)[val->id] = 0; if (odr_types_ptr)
Сначала указатель разыменовывается, а затем проверяется. Приведет ли это к неприятностям или нет, сказать сложно. Все зависит от ситуации, действительно ли указатель будет равен nullptr. Если такая ситуация невозможна, то эту избыточную проверку следует удалить, иначе она введет в заблуждение людей, поддерживающих код, а также анализатор кода. Если указатель может быть нулевым, это серьезная ошибка, требующая еще большего внимания и ее следует исправить.
Рассмотрим еще один похожий случай:
static inline bool
sd_iterator_cond (sd_iterator_def *it_ptr, dep_t *dep_ptr)
{
....
it_ptr->linkp = &DEPS_LIST_FIRST (list);
if (list)
continue;
....
}
Предупреждение PVS-Studio: V595 Указатель «list» использовался до того, как он был проверен на соответствие nullptr. Проверить строки: 1627, 1629. sched-int.h 1627
Мы должны снова показать макрос, чтобы увидеть ошибку:
#define DEPS_LIST_FIRST(L) ((L)->first)
Давайте развернем макрос и получим:
it_ptr->linkp = &((list)->first); if (list) continue;
Некоторые из вас могут сказать: «Эй, подождите! Здесь нет ошибки. Мы просто получаем указатель на член класса. Разыменования нулевого указателя нет. Да, возможно, код неточный, но ошибки нет! »
Но все не так просто, как может показаться. У нас здесь неопределенное поведение. То, что такой код работает, просто удача. На самом деле мы не можем так писать. Например, оптимизирующий компилятор может удалить флажок if (list), если он сначала видит list- ›. Если мы выполняем оператор - ›, предполагается, что указатель не равен nullptr. Если это так, то проверять указатель не следует.
Я написал целую статью на эту тему: « Разыменование нулевого указателя вызывает неопределенное поведение ». Там я рассматриваю похожий случай. Прежде чем начинать какие-либо споры, внимательно прочтите эту статью.
Однако эта ситуация действительно сложна и не совсем очевидна. Могу предположить, что могу ошибаться и здесь нет ошибки. Однако до сих пор никто не мог мне это доказать. Будет интересно увидеть комментарии разработчиков GCC, если они прочитают эту статью. Они должны знать, как работает компилятор, и следует ли интерпретировать этот код как ошибочный или нет.
Использование разрушенного массива
static void
dump_hsa_symbol (FILE *f, hsa_symbol *symbol)
{
const char *name;
if (symbol->m_name)
name = symbol->m_name;
else
{
char buf[64];
sprintf (buf, "__%s_%i", hsa_seg_name (symbol->m_segment),
symbol->m_name_number);
name = buf;
}
fprintf (f, "align(%u) %s_%s %s",
hsa_byte_alignment (symbol->m_align),
hsa_seg_name(symbol->m_segment),
hsa_type_name(symbol->m_type & ~BRIG_TYPE_ARRAY_MASK),
name);
....
}
Предупреждение PVS-Studio: V507 Указатель на локальный массив buf хранится за пределами этого массива. Такой указатель станет недействительным. hsa-dump.c 704
Строка формируется во временном буфере buf. Адрес этого временного буфера хранится в переменной name и используется в дальнейшем в теле функции. Ошибка в том, что после записи буфера в переменную name сам буфер будет уничтожен.
Мы не можем использовать указатель на разрушенный буфер. Формально мы имеем дело с неопределенным поведением. На практике этот код может работать вполне успешно. Корректная работа программы - один из проявлений неопределенного поведения.
В любом случае в этом коде есть ошибка и ее нужно исправить. Код может работать из-за того, что компилятор может счесть ненужным использовать временный буфер для дальнейшего хранения других переменных и массивов. Тогда, хотя массив, созданный в стеке, считается уничтоженным, он не будет использоваться, и функция будет работать правильно. Но эта удача может закончиться в любой момент, и код, который работал 10 лет, может внезапно начать странно вести себя при обновлении до новой версии компилятора.
Чтобы исправить эту ошибку, мы должны объявить массив buf в той же области, что и указатель name:
static void
dump_hsa_symbol (FILE *f, hsa_symbol *symbol)
{
const char *name;
char buf[64];
....
}
Выполнение аналогичных действий вне зависимости от состояния
Анализатор обнаружил фрагмент кода, который я не могу назвать ошибочным со 100% уверенностью. Однако действительно подозрительно проводить проверку, а затем, независимо от результата, выполнять одни и те же действия. Конечно, он может работать корректно, но этот фрагмент кода обязательно стоит доработать.
bool
thread_through_all_blocks (bool may_peel_loop_headers)
{
....
/* Case 1, threading from outside to inside the loop
after we'd already threaded through the header. */
if ((*path)[0]->e->dest->loop_father
!= path->last ()->e->src->loop_father)
{
delete_jump_thread_path (path);
e->aux = NULL;
ei_next (&ei);
}
else
{
delete_jump_thread_path (path);
e->aux = NULL;
ei_next (&ei);
}
....
}
Предупреждение PVS-Studio: V523 Оператор then эквивалентен оператору else. tree-ssa-threadupdate.c 2596
Если в этом коде есть ошибка, сложно сказать, как ее исправить. Это тот случай, когда необходимо ознакомиться с проектом, чтобы исправить его.
Избыточное выражение вида (A == 1 || A! = 2)
static const char *
alter_output_for_subst_insn (rtx insn, int alt)
{
const char *insn_out, *sp ;
char *old_out, *new_out, *cp;
int i, j, new_len;
insn_out = XTMPL (insn, 3);
if (alt < 2 || *insn_out == '*' || *insn_out != '@')
return insn_out;
....
}
Предупреждение PVS-Studio: V590 Рассмотрите возможность проверки этого выражения. Выражение является чрезмерным или содержит опечатку. gensupport.c 1640
Нас интересует условие: (alt ‹2 || * insn_out ==‘ * ’|| * insn_out! =‘ @ ’).
Его можно сократить до: (alt ‹2 || * insn_out! =‘ @ ’).
Рискну предположить, что оператор ! = следует заменить на ==. Тогда код будет более понятным:
if (alt < 2 || *insn_out == '*' || *insn_out == '@')
Обнуление неправильного указателя
Рассмотрим функцию, освобождающую ресурсы:
void
free_original_copy_tables (void)
{
gcc_assert (original_copy_bb_pool);
delete bb_copy;
bb_copy = NULL;
delete bb_original;
bb_copy = NULL;
delete loop_copy;
loop_copy = NULL;
delete original_copy_bb_pool;
original_copy_bb_pool = NULL;
}
Предупреждение PVS-Studio: V519 Переменной bb_copy два раза подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 1076, 1078. cfg.c 1078
Взгляните на эти 4 строки кода:
delete bb_copy; bb_copy = NULL; delete bb_original; bb_copy = NULL;
Случайно указатель bb_copy дважды обнуляется. Вот правильная версия:
delete bb_copy; bb_copy = NULL; delete bb_original; bb_original = NULL;
Утверждаю, что ничего не проверяет
Недопустимое условие, являющееся аргументом макроса gcc_assert, не повлияет на правильность работы программы, но усложнит поиск ошибок, если таковой имеется. Рассмотрим код:
static void
output_loc_operands (dw_loc_descr_ref loc, int for_eh_or_skip)
{
unsigned long die_offset
= get_ref_die_offset (val1->v.val_die_ref.die);
....
gcc_assert (die_offset > 0
&& die_offset <= (loc->dw_loc_opc == DW_OP_call2)
? 0xffff
: 0xffffffff);
....
}
Предупреждение PVS-Studio: V502 Возможно, оператор ?: работает не так, как ожидалось. Оператор ‘?:’ Имеет более низкий приоритет, чем оператор ‘‹ = ’. dwarf2out.c 2053
Приоритет тернарного оператора ?: ниже, чем у оператора сравнения ‹=. Это означает, что мы имеем дело с таким условием:
die_offset > 0 &&
((die_offset <= (loc->dw_loc_opc == DW_OP_call2)) ?
0xffff : 0xffffffff);
Таким образом, второй операнд оператора && может принимать значение 0xffff или 0xffffffff. Оба эти значения верны, поэтому это выражение можно упростить до:
(die_offset > 0)
Это явно не то, что планировал получить программист. Чтобы исправить это, вы должны добавить пару круглых скобок:
gcc_assert (die_offset > 0
&& die_offset <= ((loc->dw_loc_opc == DW_OP_call2)
? 0xffff
: 0xffffffff));
Оператор?: Очень коварен, и его лучше не использовать в сложных выражениях. Ошибиться очень легко. Мы собрали большое количество примеров подобных ошибок, которые PVS-Studio обнаруживал в различных open source проектах. Я также подробно написал об операторе ?: в книге, о которой я упоминал ранее (см. Главу N4: Остерегайтесь оператора?: И заключите ее в круглые скобки).
Забытая «цена»
Структура alg_hash_entry объявляется следующим образом:
struct alg_hash_entry {
unsigned HOST_WIDE_INT t;
machine_mode mode;
enum alg_code alg;
struct mult_cost cost;
bool speed;
};
Программист решил проверить, есть ли в функции synth_mult нужный объект. Для этого ему нужно было сравнить поля структуры. Однако, похоже, здесь есть ошибка:
static void synth_mult (....)
{
....
struct alg_hash_entry *entry_ptr;
....
if (entry_ptr->t == t
&& entry_ptr->mode == mode
&& entry_ptr->mode == mode
&& entry_ptr->speed == speed
&& entry_ptr->alg != alg_unknown)
{
....
}
Предупреждение PVS-Studio: V501 Слева и справа от оператора «&&» есть идентичные подвыражения ‘entry_ptr-› mode == mode ’. expmed.c 2573
Режим проверяется дважды, но стоимость никак не проверяется. Возможно, одно из этих сравнений следует убрать, но есть вероятность, что мы должны сравнить стоимость. Мне сложно сказать, но код надо поправить.
Дублированные задания
На мой взгляд, следующие фрагменты кода не представляют опасности для жизни программы, и кажется, что дублированное назначение можно просто удалить.
Фрагмент N1
type_p
find_structure (const char *name, enum typekind kind)
{
....
structures = s; // <=
s->kind = kind;
s->u.s.tag = name;
structures = s; // <=
return s;
}
Предупреждение PVS-Studio: V519 Переменной «структуры» дважды подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 842, 845. gengtype.c 845
Фрагмент N2
static rtx
ix86_expand_sse_pcmpistr (....)
{
unsigned int i, nargs;
....
case V8DI_FTYPE_V8DI_V8DI_V8DI_INT_UQI:
case V16SI_FTYPE_V16SI_V16SI_V16SI_INT_UHI:
case V2DF_FTYPE_V2DF_V2DF_V2DI_INT_UQI:
case V4SF_FTYPE_V4SF_V4SF_V4SI_INT_UQI:
case V8SF_FTYPE_V8SF_V8SF_V8SI_INT_UQI:
case V8SI_FTYPE_V8SI_V8SI_V8SI_INT_UQI:
case V4DF_FTYPE_V4DF_V4DF_V4DI_INT_UQI:
case V4DI_FTYPE_V4DI_V4DI_V4DI_INT_UQI:
case V4SI_FTYPE_V4SI_V4SI_V4SI_INT_UQI:
case V2DI_FTYPE_V2DI_V2DI_V2DI_INT_UQI:
nargs = 5; // <=
nargs = 5; // <=
mask_pos = 1;
nargs_constant = 1;
break;
....
}
Предупреждение PVS-Studio: V519 Переменной nargs два раза подряд присваиваются значения. Возможно, это ошибка. Проверить строки: 39951, 39952. i386.c 39952
Фрагмент N3
Последний фрагмент кажется более странным, чем другие. Возможно, здесь есть какая-то ошибка. Переменной steptype значение присваивается 2 или 3 раза. Это очень подозрительно.
static void
cand_value_at (....)
{
aff_tree step, delta, nit;
struct iv *iv = cand->iv;
tree type = TREE_TYPE (iv->base);
tree steptype = type; // <=
if (POINTER_TYPE_P (type))
steptype = sizetype; // <=
steptype = unsigned_type_for (type); // <=
....
}
Предупреждение PVS-Studio: V519 Переменной steptype два раза подряд присваиваются значения. Возможно, это ошибка. Проверьте строки: 5173, 5174. tree-ssa-loop-ivopts.c 5174
Вывод
Я рад, что смог написать эту статью. Теперь мне будет что сказать в ответ на комментарии типа «PVS-Studio не нужен, потому что GCC выдает такие же предупреждения». Как видите, PVS-Studio - очень мощный инструмент, который превосходит GCC по диагностическим возможностям. Я не отрицаю, что у GCC отличная диагностика. Этот компилятор, если он правильно настроен, действительно вызывает множество проблем в коде. Но PVS-Studio - это специализированный и быстро развивающийся инструмент, а это значит, что он также будет лучше обнаруживать ошибки в коде, чем компиляторы.
Предлагаю взглянуть на анализ других open-source проектов и посетить этот раздел нашего сайта. Также те, кто пользуется Twitter, могут подписаться на меня @Code_Analysis. Я регулярно размещаю ссылки на интересные статьи о программировании на C и C ++, а также рассказываю о достижениях нашего анализатора.
