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

    PVS-Studio vs 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".

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

    Другие наши статьи про проверку компиляторов


    1. Проверка LLVM (Clang) (август 2011), вторая проверка (август 2012), третья проверка (октябрь 2016), четвёртая проверка (апрель 2019)
    2. Проверка GCC (август 2016)
    3. Проверка Huawei Ark Compiler (декабрь 2019)
    4. Проверка .NET Compiler Platform («Roslyn») (декабрь 2015), вторая проверка (апрель 2019)
    5. Проверка Roslyn Analyzers (август 2019)
    6. Проверка PascalABC.NET (март 2017)



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Checking the GCC 10 Compiler with PVS-Studio.
    PVS-Studio
    Static Code Analysis for C, C++, C# and Java

    Комментарии 27

      +2
      По поводу «это все же С++ код а не С».

      Могу ошибаться, но когда в 4.9 кажется? gcc стал использовать С++ в своих исходниках, в рассылке обсуждалось полиси для этого — что в основном будут использовать С++ для применения шаблонов для сокращения повторов кода и кодогенерации. Использовать все возможности языка вроде как не планировалось. Кто сейчас активно участвует в контрибьюте в проект, меня поправит. так что там даже не «С с классами» а какая-то странная разновидность «C with benefits».
        +1

        Самое интересное — это багрепорт в GNU и реакция разработчиков GCC на найденные ошибки.

          +9
          Я бы снизил ожидания насчёт реакции, а то разрабы не очень эмоциональный народ)
          Возможная реакция

            0
            Кстати, оказывается даже proof есть!
            Вот баг в GCC, отписанный коллегой в 2017. Баг подтверждён: gcc.gnu.org/bugzilla/show_bug.cgi?id=80051 И по ныне присутствует в коде. Вот и вся реакция. :)
              0

              Пофиксили, но этот баг еще не закрыли.

                0

                Точнее уже закрыли.

                  0
                  Видимо кто-то читает Habr :)
              +3

              Заsubmitел №94629

                0

                В сухом остатке: всё пофиксили (вроде-бы, не вычитывал), поправили часть предупреждений от cppcheck, а затем решили немного улучшить собственный анализатор.

              +2
              Какая-то у вас гну на КПДВ анорексичная.
              Намекаете что в опен-сорсе денег нет?

              На логотипе GNU она вполне себе упитанная.
              www.gnu.org
                +2
                Ну она же вылупляется из яйца, птенцы обычно щупленькие. Птенцы антилоп, да…
                  +1
                  Вовсе не анорексичная, просто на фоне широко открытых глаз и нашего толстячка-добрячка единорога выглядит чуть худее, чем обычно :).
                  +4
                  Ну давайте теперь CPython)
                    –3
                    Я бы не стал подходить так критично, вы сами заметили, что разработчики «своего» продукта сами хорошо его знают. И это хорошо.
                    Но так же давно перестал читать мантры про макросы, goto и т.д. Это нормально, а тем более когда знаешь что ты делаешь. И это часто встречается в исходных кодах и известных крупных компаний.
                        +1
                        А чего не в самой статье?
                        0

                        В чём состоит проблема со статическим анализом макросов? Они же раскрываются по простым правилам.

                          0
                          Проблема в том, что с точки зрения анализатора, раскрытый код не имеет смысла/аномален. Например, всегда истинные/ложные условия, неиспользуемые переменные и т.д.
                            0

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

                          0

                          По опыту, в PVS сравнительно больше проверок для выявление copy-paste ошибок.


                          Тем не менее, сам я пользуюсь Coverity Scan по ряду веских причин:


                          • доступен для OpenSource бесплатно и без танцев с бубном;
                          • есть workflow для сгенерированных предупреждений с хранением состояния в БД, тут стоит пояснить:
                            • "fix submited" позволяет автоматически закрывать issue при их реальном исправлении.
                            • предупреждения помеченные единожды как "false-positive" или "intentional" не будут больше мозолить глаза (но останутся в "облаке" и их можно пересмотреть).
                            • игнорируемые предупреждения не будут всплывать в других проектах использующих этот (даже для git-subtree и амальгамированного кода).
                          • анализ и результаты в облаке с web-интерфейсом, что идеально для opensource.
                            +1
                            На всякий случай прокомментирую, чтобы была ясность и одинаковая трактовка вопросов.
                            доступен для OpenSource бесплатно и без танцев с бубном;
                            Мне кажется, «танцев с бубном» это преувеличение. Достаточно запросить ключ и можно пользоваться: "Бесплатный PVS-Studio для тех, кто развивает открытые проекты".
                            Некоторый танец с бубном есть для закрытых проектов (добавление комментариев в код). Но так Coverity Scan для закрытых проектов вообще ничего не предоставляет.

                            есть workflow для сгенерированных предупреждений с хранением состояния в БД, тут стоит пояснить:
                            Похожее есть и в PVS-Studio. Можно использовать базу разметки предупреждений.
                              0

                              Короче, буду пробовать для libmdbx.
                              В любом случае лучше опираться на отечественного поставщика и сокращать уязвимые зависимости.

                              +2

                              По моему личному опыту там ещё и качество предупреждений повыше, а их спектр — поинтереснее.


                              Эх, где-то в черновиках у меня валялась статья про лонгитюдное исследование и сравнение coverity/clang static-analyzer/pvs studio, дописать, что ли.

                              0
                              Если есть возможность проверьте пожалуйста мой проект.
                              github.com/SharMisha/MSH

                            Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

                            Самое читаемое