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

16 Апр 2020

Теги:

#Cpp #Compiler

Проверка компилятора GCC 10 с помощью PVS-Studio

16 Апр 2020

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

Время перепроверить код компилятора GCC
10 подозрительных фрагментов кода
Заключение
Другие наши статьи про проверку компиляторов

Компилятор 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".

Спасибо за внимание. И приходите читать наш блог. Там много интересного.