Компилятор GCC написан с обильным использованием макросов. Очередная проверка кода GCC с помощью PVS-Studio вновь подтверждает мнение нашей команды, что макросы – это плохо. В таком коде тяжело разбираться не только статическому анализатору, но и программисту. Конечно, разработчики GCC уже привыкли к проекту и хорошо разбираются в нём. Но со стороны очень сложно что-то понять. Собственно, из-за макросов и не удалось полноценно выполнить проверку кода. Тем не менее, анализатор PVS-Studio, как всегда, показал, что может находить ошибки даже в компиляторах.
Время перепроверить код компилятора GCC
Предыдущий раз я проверял компилятор GCC четыре года назад. Время летит быстро и незаметно, и я как-то всё забывал вернуться к этому проекту и перепроверить его. Подтолкнуло вернуться к этой идее публикация "Static analysis in GCC 10".
Собственно, не секрет, что в компиляторах существуют свои собственные встроенные статические анализаторы кода и они тоже развиваются. Поэтому время от времени мы пишем статьи о том, что статический анализатор PVS-Studio может находить ошибки даже внутри компиляторов и что мы не зря едим хлеб :).
На самом деле, сравнивать классические статические анализаторы с компиляторами нельзя. Статические анализаторы – это не только поиск ошибок в коде, но и развитая инфраструктура. Например, это интеграция с такими системами, как SonarQube, PlatformIO, Azure DevOps, Travis CI, CircleCI, GitLab CI/CD, Jenkins, Visual Studio. Это развитые механизмы массового подавления предупреждений, что позволяет быстро начать использовать PVS-Studio даже в большом старом проекте. Это рассылка уведомлений. И так далее, и так далее. Однако, всё равно первый вопрос, который задают: «Может ли PVS-Studio найти такое, что не могут найти компиляторы?». А значит, мы будем вновь и вновь писать статьи о проверке этих самих компиляторов.
Вернёмся к теме проверки проекта GCC. Нет нужды останавливаться на этом проекте и рассказывать, что он из себя представляет. Давайте лучше поговорим, что внутри этого проекта.
А внутри огромное количество макросов, которые мешают проверке. Во-первых, анализатор PVS-Studio выдаёт большое количество ложных срабатываний. В этом нет ничего страшного, но вот так просто взять и начать изучать выданный им отчёт не получается. По-хорошему, нужно проделать работу по подавлению ложных предупреждений в макросах. В противном случае, полезные предупреждения просто тонут в потоке шума. Подобная настройка выходит за границы задачи написания статьи. Собственно, буду совсем честен — мне было просто лень этим заниматься, хотя ничего сложного в этом нет. Из-за шума просмотр отчёта носил достаточно поверхностный характер.
Во-вторых, мне, как человеку, не знакомому с проектом, очень сложно понимать код. Макросы, макросы… Надо смотреть, во что они разворачиваются, чтобы понять, почему анализатор выдаёт предупреждения. Очень тяжело. Не люблю макросы. Кто-то может сказать, что без макросов в C не обойтись. Но GCC давно написан вовсе не на C. Да, файлы по историческим причинам имеют расширение .c, но заглядываешь туда, а там:
// Файл alias.c .... struct alias_set_hash : int_hash <int, INT_MIN, INT_MIN + 1> {}; struct GTY(()) alias_set_entry { alias_set_type alias_set; bool has_zero_child; bool is_pointer; bool has_pointer; hash_map<alias_set_hash, int> *children; }; ....
Это явно не C, а C++.
В общем, макросы и стиль написания кода очень усложняют изучение отчёта анализатора. Так что в этот раз я не порадую длинным списком разнообразнейших ошибок. Я с трудом и используя несколько чашек кофе, выписал 10 интересных фрагментов, и на этом силы оставили меня :).
10 подозрительных фрагментов кода
Фрагмент N1, Кажется, неудачный Copy-Paste
static bool try_crossjump_to_edge (int mode, edge e1, edge e2, enum replace_direction dir) { .... if (FORWARDER_BLOCK_P (s->dest)) s->dest->count += s->count (); if (FORWARDER_BLOCK_P (s2->dest)) s2->dest->count -= s->count (); .... }
Предупреждение PVS-Studio: V778 Two similar code fragments were found. Perhaps, this is a typo and 's2' variable should be used instead of 's'. cfgcleanup.c 2126
На самом деле, я не уверен, что это именно ошибка. Однако, у меня есть сильное подозрение, что этот код написан с помощью Copy-Paste, и во втором блоке в одном месте забыли заменить s на s2. То есть, мне кажется второй блок кода должен быть таким:
if (FORWARDER_BLOCK_P (s2->dest)) s2->dest->count -= s2->count ();
Фрагмент N2, Опечатка
tree vn_reference_lookup_pieces (....) { struct vn_reference_s vr1; .... vr1.set = set; vr1.set = base_set; .... }
Предупреждение PVS-Studio: V519 The 'vr1.set' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3448, 3449. tree-ssa-sccvn.c 3449
Очень странно, что в одну и туже переменную дважды подряд записываются разные значения. Это явная опечатка. Рядом в этом файле есть вот такой код:
vr1.set = set; vr1.base_set = base_set;
Скорее всего, и в подозрительном коде должно было быть написано точно так же.
Фрагмент N3, Присваивание переменной самой себе
static omp_context * new_omp_context (gimple *stmt, omp_context *outer_ctx) { omp_context *ctx = XCNEW (omp_context); splay_tree_insert (all_contexts, (splay_tree_key) stmt, (splay_tree_value) ctx); ctx->stmt = stmt; if (outer_ctx) { ctx->outer = outer_ctx; ctx->cb = outer_ctx->cb; ctx->cb.block = NULL; ctx->local_reduction_clauses = NULL; ctx->outer_reduction_clauses = ctx->outer_reduction_clauses; // <= ctx->depth = outer_ctx->depth + 1; } .... }
Предупреждение PVS-Studio: V570 The 'ctx->outer_reduction_clauses' variable is assigned to itself. omp-low.c 935
Очень странно присваивать переменную самой себе.
Фрагмент N4. 0,1,2, Фредди заберёт тебя.
Недавно я опубликовал статью "Ноль, один, два, Фредди заберёт тебя". Мне кажется, следующий фрагмент кода продолжает коллекцию ошибок, рассмотренных в этой статье.
#define GET_MODE(RTX) ((machine_mode) (RTX)->mode) .... static int add_equal_note (rtx_insn *insns, rtx target, enum rtx_code code, rtx op0, rtx op1, machine_mode op0_mode) { .... if (commutative_p && GET_MODE (xop0) != xmode0 && GET_MODE (xop1) != xmode1 && GET_MODE (xop0) == xmode1 && GET_MODE (xop1) == xmode1) std::swap (xop0, xop1); .... }
Предупреждение PVS-Studio: V560 A part of conditional expression is always false:
((machine_mode)(xop1)->mode) == xmode1. optabs.c 1053
Обратите внимание на эти два подвыражения:
- GET_MODE (xop1) != xmode1
- GET_MODE (xop1) == xmode1
Над результатами этих подвыражений выполняется операция AND, что явно не имеет практического смысла. Собственно, если начало выполняться второе подвыражение, то заранее известно, что она даст в результате false.
Скорее вс��го, здесь опечатка в нулях и единицах, и на самом деле условие должно был быть таким:
&& GET_MODE (xop0) != xmode0 && GET_MODE (xop1) != xmode1 && GET_MODE (xop0) == xmode1 && GET_MODE (xop1) == xmode0
Конечно, я не уверен, что правильно изменил код, так как не ориентируюсь в проекте.
Фрагмент N5. Подозрительное изменение значения аргумента
bool ipa_polymorphic_call_context::set_by_invariant (tree cst, tree otr_type, HOST_WIDE_INT off) { poly_int64 offset2, size, max_size; bool reverse; tree base; invalid = false; off = 0; // <= .... if (otr_type && !contains_type_p (TREE_TYPE (base), off, otr_type)) return false; set_by_decl (base, off); return true; }
Предупреждение PVS-Studio: V763 Parameter 'off' is always rewritten in function body before being used. ipa-polymorphic-call.c 766
Значение аргумента off сразу заменяется на 0. Более того, нет поясняющего комментария. Всё это очень подозрительно. Иногда такой код появляется в процессе отладки. Программисту было нужно посмотреть, как поведёт себя функция в определённом режиме, поэтому он временно изменил значение аргумента, а удалить эту строчку потом забыли. В результате в коде появляется ошибка. Конечно, возможно здесь всё правильно, но этот код явно нуждается в проверке и поясняющем комментарии, чтобы в будущем подобные вопросы не возникали.
Фрагмент N6. Мелочь
cgraph_node * cgraph_node::create_clone (....) { .... new_node->icf_merged = icf_merged; new_node->merged_comdat = merged_comdat; // <= new_node->thunk = thunk; new_node->unit_id = unit_id; new_node->merged_comdat = merged_comdat; // <= new_node->merged_extern_inline = merged_extern_inline; .... }
Предупреждение PVS-Studio: V519 The 'new_node->merged_comdat' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 406, 409. cgraphclones.c 409
Случайно продублировано присваивание. Скорее всего ничего страшного. Однако, всегда есть риск, что в действительности здесь забыли выполнить какое-то другое присваивание.
Фрагмент N7. Код, который выглядит опасно
static void complete_mode (struct mode_data *m) { .... if (m->cl == MODE_COMPLEX_INT || m->cl == MODE_COMPLEX_FLOAT) alignment = m->component->bytesize; else alignment = m->bytesize; m->alignment = alignment & (~alignment + 1); if (m->component) .... }
Предупреждение PVS-Studio: V595 The 'm->component' pointer was utilized before it was verified against nullptr. Check lines: 407, 415. genmodes.c 407
В начале указатель m->component разыменовывается в одной из веток оператора if. Я имею в виду это выражение: m->component->bytesize.
Далее оказывается, что этот указатель может быть нулевым. Это следует из проверки: if (m->component).
Этот код необязательно ошибочен. Вполне возможно, ветка с разыменованием выполняется только в том случае, если указатель не нулевой. То есть существует косвенная взаимосвязь между значением переменной m->cl и m->component. Но выглядит такой код в любом случае очень опасным. И нет никаких поясняющих комментариев.
Фрагмент N8. Двойная проверка
void pointer_and_operator::wi_fold (value_range &r, tree type, const wide_int &lh_lb, const wide_int &lh_ub, const wide_int &rh_lb ATTRIBUTE_UNUSED, const wide_int &rh_ub ATTRIBUTE_UNUSED) const { // For pointer types, we are really only interested in asserting // whether the expression evaluates to non-NULL. if (wi_zero_p (type, lh_lb, lh_ub) || wi_zero_p (type, lh_lb, lh_ub)) r = range_zero (type); else r = value_range (type); }
Предупреждение PVS-Studio: V501 There are identical sub-expressions 'wi_zero_p(type, lh_lb, lh_ub)' to the left and to the right of the '||' operator. range-op.cc 2657
Какая-то странная проверка. Функция wi_zero_p вызывается дважды с одним и тем же набором фактических аргументов. Можно заподозрить, что на самом деле при втором вызове должны быть использованы принятые извне аргументы: rh_lb, rh_ub. Но нет, эти аргументы помечены как неиспользуемые (ATTRIBUTE_UNUSED).
Поэтому мне непонятно, почему бы не написать проверку проще:
if (wi_zero_p (type, lh_lb, lh_ub)) r = range_zero (type); else r = value_range (type);
Или здесь какая-то опечатка? Или логическая ошибка? Не знаю, но код очень странный.
Фрагмент N9. Опасный доступ к массиву
struct algorithm { struct mult_cost cost; short ops; enum alg_code op[MAX_BITS_PER_WORD]; char log[MAX_BITS_PER_WORD]; }; static void synth_mult (struct algorithm *alg_out, unsigned HOST_WIDE_INT t, const struct mult_cost *cost_limit, machine_mode mode) { int m; struct algorithm *alg_in, *best_alg; .... /* Cache the result. */ if (!cache_hit) { entry_ptr->t = t; entry_ptr->mode = mode; entry_ptr->speed = speed; entry_ptr->alg = best_alg->op[best_alg->ops]; entry_ptr->cost.cost = best_cost.cost; entry_ptr->cost.latency = best_cost.latency; } /* If we are getting a too long sequence for `struct algorithm' to record, make this search fail. */ if (best_alg->ops == MAX_BITS_PER_WORD) return; .... }
Предупреждение PVS-Studio: V781 The value of the 'best_alg->ops' variable is checked after it was used. Perhaps there is a mistake in program logic. Check lines: 3157, 3164. expmed.c 3157
Давайте сократим код, чтобы стало понятнее, что не нравится анализатору:
if (!cache_hit) { entry_ptr->alg = best_alg->op[best_alg->ops]; } if (best_alg->ops == MAX_BITS_PER_WORD)
В начале переменная best_alg->ops используется для индексирования массива. И только потом происходит проверка этой переменной на граничное значение. Теоретически может произойти выход за границу массива (классическая разновидность ошибки CWE-193: Off-by-one Error).
Действительно ли это настоящая ошибка? И как это постоянно происходит в этой статье, я не уверен :). Возможно, есть взаимосвязь между значением этого индекса и переменной cache_hit. Возможно, ничего не кэшируется, если индекс равен максимуму (MAX_BITS_PER_WORD). Код функции большой, и я не разобрался.
В любом случае, этот код лучше всего проверить. И даже если он окажется правильным, я бы рекомендовал сопроводить рассмотренный участок программы комментарием. Он может сбить с толку не только меня или PVS-Studio, но и ещё кого-то.
Фрагмент N10. Код, который за 4 года не исправили
Ещё в прошлой статье я обратил внимание вот на этот код:
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 There are identical sub-expressions '!strcmp(a->v.val_vms_delta.lbl1, b->v.val_vms_delta.lbl1)' to the left and to the right of the '&&' operator. dwarf2out.c 1481
Две функции strcmp сравнивают одни и те же указатели. То есть выполняется явно избыточная проверка. В предыдущей статье я предположил, что это опечатка, и на самом деле должно быть написано:
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));
Однако за 4 года этот код так и не был исправлен. При этом мы сообщали авторам о подозрительных участках кода, которые мы описали в статье. Теперь я уже не так уверен, что это ошибка. Возможно, это просто избыточный код. В этом случае, выражение можно упростить:
return (!strcmp (a->v.val_vms_delta.lbl1, b->v.val_vms_delta.lbl1));
Посмотрим, изменят ли разработчики GCC этот фрагмент кода после новой статьи.
Заключение
Напоминаю, что для проверки открытых проектов вы можете использовать вот этот бесплатный вариант лицензии. Кстати, есть и другие варианты бесплатного лицензирования PVS-Studio, в том числе даже для закрытых проектов. Они перечислены здесь: "Бесплатные варианты лицензирования PVS-Studio".
Спасибо за внимание. И приходите читать наш блог. Там много интересного.
Другие наши статьи про проверку компиляторов
- Проверка LLVM (Clang) (август 2011), вторая проверка (август 2012), третья проверка (октябрь 2016), четвёртая проверка (апрель 2019)
- Проверка GCC (август 2016)
- Проверка Huawei Ark Compiler (декабрь 2019)
- Проверка .NET Compiler Platform («Roslyn») (декабрь 2015), вторая проверка (апрель 2019)
- Проверка Roslyn Analyzers (август 2019)
- Проверка PascalABC.NET (март 2017)
Если хотите по��елиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Checking the GCC 10 Compiler with PVS-Studio.
