Компилятор GCC написан с обильным использованием макросов. Очередная проверка кода GCC с помощью PVS-Studio вновь подтверждает мнение нашей команды, что макросы – это плохо. В таком коде тяжело разбираться не только статическому анализатору, но и программисту. Конечно, разработчики GCC уже привыкли к проекту и хорошо разбираются в нём. Но со стороны очень сложно что-то понять. Собственно, из-за макросов и не удалось полноценно выполнить проверку кода. Тем не менее, анализатор PVS-Studio, как всегда, показал, что может находить ошибки даже в компиляторах.
Предыдущий раз я проверял компилятор 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 интересных фрагментов, и на этом силы оставили меня :).
Фрагмент 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
Обратите внимание на эти два подвыражения:
Над результатами этих подвыражений выполняется операция 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".
Спасибо за внимание. И приходите читать наш блог. Там много интересного.