Продолжение: обидно за мнения про статические анализаторы кода

    image1.png

    Планировалось, что, написав статью "Обидно за мнения про статические анализаторы кода", мы выговоримся и спокойно отпустим тему. Но неожиданно эта статья вызвала бурный отклик. К сожалению, обсуждение пошло не туда, и сейчас мы сделаем вторую попытку объяснить своё видение ситуации.

    Анекдот-аналогия


    Итак, всё началось со статьи "Обидно за мнения про статические анализаторы кода". Её начали активно обсуждать на некоторых ресурсах и это обсуждение очень напоминает следующий старый анекдот.
    Купили как-то суровым сибирским лесорубам японскую бензопилу.
    Собрались в кружок лесорубы, решили её испытать.
    Завели её, подсунули ей деревце.
    "Вжик" — сказала японская пила.
    "У, бля..." — сказали лесорубы.
    Подсунули ей деревце потолще. "Вж-ж-жик!" — сказала пила.
    "Ух, бля!" — сказали лесорубы.
    Подсунули ей толстенный кедр. "ВЖ-Ж-Ж-Ж-Ж-Ж-Ж-ЖИК!!!" — сказала пила.
    "Ух ты, бля!!" — сказали лесорубы.
    Подсунули ей железный лом. "КРЯК!" — сказала пила.
    "Ага, бля!!!" — укоризненно сказали суровые сибирские лесорубы! И ушли рубить лес топорами...
    История один в один. Люди, посмотрели на код:

    if (A[0] == 0)
    {
      X = Y;
      if (A[0] == 0)
        ....
    }

    И начали придумывать ситуации, когда он может быть оправданным, и значит, предупреждение анализатора PVS-Studio является ложно-позитивным. В ход пошли рассуждения про изменение памяти между двумя проверками, возникающее из-за:

    • работы параллельных потоков;
    • обработчиков сигналов/прерываний;
    • переменная X является ссылкой на элемент A[0];
    • аппаратного обеспечения, например, выполнения DMA-операциям;
    • и так далее.

    И обсудив, что не все ситуации анализатор может понять, ушли рубить лес топорами. То есть нашли оправдание, почему можно и дальше не использовать статический анализатор кода в своей работе.

    Наше видение ситуации


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

    Да, любой статический анализатор выдаёт ложно-позитивные срабатывания. И с этим ничего нельзя поделать. Однако, эта беда сильно преувеличивается. На практике статические анализаторы можно настроить и использовать различные способы подавления и работы с ложными срабатываниями (см. 1, 2, 3, 4). Плюс здесь уместно вспомнить про статью "False positives are our enemies, but may still be your friends".

    Впрочем, даже это не главное. Частные случаи экзотического кода нет смысла вообще рассматривать! Можно сложным кодом запутать анализатор? Да, можно. Однако, на один такой случай, будет приходиться сотни полезных срабатываний анализатора. Можно найти и исправить множество ошибок на самом раннем этапе. А одно-два ложных срабатываний можно спокойно подавить и больше не обращать на них внимание.

    И вновь PVS-Studio прав


    Здесь статью можно было-бы и закончить. Однако, некоторые могут посчитать предыдущий раздел не рациональными соображениями, а попытками скрыть слабости и недостатки инструмента PVS-Studio. Поэтому придётся продолжить.

    Рассмотрим конкретный компилируемый код, включающий объявление переменных:

    void SetSynchronizeVar(int *);
    
    int foo()
    {
        int flag = 0;
        SetSynchronizeVar(&flag);
    
        int X, Y = 1;
    
        if (flag == 0)
        {
            X = Y;
            if (flag == 0)
                return 1;
        }
        return 2;
    }

    Анализатор PVS-Studio обоснованно выдаёт предупреждение: V547 Expression 'flag == 0' is always true.

    И анализатор совершенно прав. Если кто-то начнёт разглагольствовать, что переменная может поменяться в другом потоке, в обработчике сигнала и так далее, то он просто не понимает язык C и C++. Так писать нельзя.

    Компилятор в целях оптимизации вправе выбросить вторую проверку и будет абсолютно прав. С точки зрения языка, переменная измениться не может. Её фоновое изменение- это не что иное, как Undefined behavior.

    Чтобы проверка осталась на месте, переменная должна быть объявлена как volatile:

    void SetSynchronizeVar(volatile int *);
    
    int foo()
    {
        volatile int flag = 0;
        SetSynchronizeVar(&flag);
        ....
    }

    Анализатор PVS-Studio знает про это и уже не выдаёт предупреждение на такой код.

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

    Примечание для самых дотошных читателей


    Кто-то из читателей может вернуться к синтетическому примеру из первой статьи:

    char get();
    int foo(char *p, bool arg)
    {
        if (p[1] == 1)
        {
            if (arg)
                p[0] = get();
            if (p[1] == 1)          // Warning
                return 1;
        }
        // ....
        return 3;
    }

    И добавить volatile:

    char get();
    int foo(volatile char *p, bool arg)
    {
        if (p[1] == 1)
        {
            if (arg)
                p[0] = get();
            if (p[1] == 1)          // Warning :-(
                return 1;
        }
        // ....
        return 3;
    }

    После чего, справедливо заметить, что анализатор по-прежнему выдаёт предупреждение V547 Expression 'p[1] == 1' is always true.

    Ура, наконец показано, что анализатор всё-таки бывает неправ :). Это ложное срабатывание!

    Как видите, мы не скрываем какие-то недоработки. При анализе потока данных для элементов массива этот злосчастный volatile потерялся. Недоработка уже найдена и исправлена. Исправление будет доступно в следующей версии анализатора. Ложного срабатывания не будет.

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

    Почему код нереален? Во-первых, на практике между двумя проверками будет какая-то функция синхронизации или задержки. Во-вторых, никто в здравом уме без крайней необходимости не создаёт массивы, состоящие из volatile-элементов. Работа с таким массивом- это колоссальное падение производительности.

    Подытожим. Можно легко создать примеры, где анализатор ошибается. Но с практической точки зрения выявляемые недоработки практически не сказываются на качестве анализа кода и количестве выявленных настоящих ошибок. Ведь код реальных приложений- это просто код понятный одновременно анализатору и человеку, а не ребус или пазл. Если код — это пазл, то тут уж не до анализаторов :).

    Спасибо за внимание.


    Дополнительные ссылки





    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. Part 2: Upsetting Opinions about Static Analyzers.
    PVS-Studio
    Статический анализ кода для C, C++, C# и Java

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

      +5
      «нереальный» код зачастую возникает в процессе разработки. Ну то есть сначала было только одно условие, затем добавили присваивание, и чуть позже словили баг, выяснили, что присваивание должно срабатывать не всегда и добавили ещё одно условие, вот и получили неоправданно усложнённый код. Иногда полученный код называют «Rube Goldberg Code» — тут только периодический рефакторинг, ну или дотошный ревью поможет избавиться от таких участков. Статический анализатор, указывающий на такие места, конечно помогает.
        +4
        Кажется, что многие не понимают принципа «пишите скучно».
        Экзотические варианты есть, но каждый такой вариант должен быть в осознаным и вызывать вопрос — а нужно ли так делать, если это вызывает такие проблемы?
        А главное — иногда ты пишешь скучно, а хак получается из-за ошибки. И вот тогда супер полезно обратить на него пристальное внимание.
          +4

          Я могу ошибаться, прошу пояснить. В вашем последнем примере volatile относится к указателю или к массиву, на который передаётся указатель?


          Up: Сам спросил, сам отвечу :) да, если бы volatile был после *, то относился бы к самой переменной. Так что пример корректен.

            –3
            Чтение по спирали это одна из самых моих нелюбимых вещей в С. Для перестраховки я предпочитаю писать что-то типа volatile * volatile uint8_t (или дважды const), чтобы уж наверняка.
              +3

              Это некорректно, нужно понимать, что вы делаете. Чтобы было легче и не путать указатель на константу с константным указателем могу посоветовать такой хинт (не для продакшена): объявляйте все типы через using. А потом поиграйтесь в IDE (например CLion) с заменой типов — сразу будет понятно что и как раскрывается.
              Вообще меня ещё в техникуме сильно гоняли на первом курсе по этому поводу, чтобы выработать понимание. Но с появлением auto и развитием STL всё это стало ненужно.

                +1
                1. Я пишу на С.
                2. Я понимаю, что это и как работает. Я не понимаю, что и как осыпается при оптимизациях компилятора.
                  –1
                  GCC не замечает проблем на реальных адресах данных, он работает с ассоциациями — именами переменных, их типами, свойствами, и атрибутами.
                  Ссылки — зло. Используйте объединения.
                    0

                    Ну тут всё просто. Если утрировать, то в си пофиг на const в большинстве случаев. Это больше для программиста подсказки. За исключением ситуаций когда объект лежит в readonly памяти. Если вы в такое упрётесь, то скорее всего вы это уже будете знать :)
                    Про volatile. Если вы в теле метода не меняете значение обычной переменной, то компилятор может вычислить все выражения с ней заранее, например при вызове функции; а всё что в циклах уже игнорировать и подставлять значения. Когда же переменная не volatile то компилятор не будет делать таких допущений. Грубо говоря такой код:
                    void foo(int i){
                    I = 1;
                    While(I != 0){...}
                    }
                    Превратится в такой:
                    void foo(int i){
                    I = 1;
                    While(true){...}
                    }
                    А с точки зрения ассемблера, какой нибудь cmp заменится на безусловный goto. Что будет куда более дружелюбно для конвейера.
                    Вот чтобы запретить компилятору такие фокусы, используется volatile. Ну или если вы что-то низкоуровневое программируете и напрямую читаете память.

                      0
                      Вот чтобы запретить компилятору такие фокусы, используется volatile.
                      Даже с -О3 такое упрощение не будет проведено? Не помню этот кусок стандарта.
                      За исключением ситуаций когда объект лежит в readonly памяти. Если вы в такое упрётесь, то скорее всего вы это уже будете знать :)
                      Ну, обычно знаю :) По-моему, регистры иначе как константный указатель на волатильную переменную и не объяснить. Ещё константные массивы как член структуры очень удобно объявлять, когда играем в ООП.
                        +1

                        Даже с O3, если переменная помечена как volatile. Для того он собственно и создавался. Представьте, что у вас где-то в памяти лежат данные с какой-либо железяки, которые туда попадают допустим через DMA. Без volatile было бы очень проблемно отслеживать их изменение.

                          0
                          Кстати, а не существует прагмы justdoit, чтобы стока или блок не могли быть удалёнными компилятором? Всё-таки извращения, когда нужно забить нулями в целях безопасности память, которая в функции больше использоваться не будет, не очень способствуют читаемости кода.
            +4
            Даже если вторая проверка действительно нужна, код лучше переписать по возможности, так что анализатор вполне справедливо кинул предупреждение.
            Если на проекте слишком много такого кода, то, боюсь, проблема не в анализаторе.
              0
              а были статьи с описанием, как ваш анализатор устроен? Как происходит сам процесс анализа, используется поиск определенных шаблонов или что-то ещё?
              мне любопытно, но сам не могу полностью понять как такая штука работает.
              +3

              Мне кажется, что вы под влиянием вполне понятных эмоций недооцениваете опасение разработчиков. А именно: в C++ нет простого и очевидного способа удостоверится в том, что у вызваной функции нет нежелательных побочных эффектов. Причем не обязательно эффектов, которые завязаны на многопоточность, чтение I/O портов или какой-то другой хардкорной специфики.


              Вот простой пример, в котором все работает в однопоточном режиме:


              class Registry {
                  int * ptr_{nullptr};
              public:
                  void register_pointer(int * ptr) { ptr_ = ptr; }
                  void remove_pointer() { ptr_ = nullptr; }
                  void increment() { if(ptr_) ++(*ptr_); }
              };
              
              Registry registry;
              
              void func_with_side_effects() {
                  registry.increment();
              }
              
              int demo() {
                  int a[3]{ 0, 1, 2 };
              
                  struct RegistryModificator {
                      RegistryModificator(int * ptr) {
                          registry.register_pointer(ptr);
                      }
                      ~RegistryModificator() {
                          registry.remove_pointer();
                      }
                  } registry_modificator{a};
              
                  if(0 == a[0]) {
                      func_with_side_effects();
                      if(0 == a[0]) return -1;
                  }
              
                  return 0;
              }
              
              int main() {
                  return demo();
              }

              После вызова func_with_side_effects повторная проверка имеет смысл. Но PVS-Studio скажет, что она избыточна.


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


              Если это будет человек, который не знает тонкостей работы func_with_side_effects и не имеет возможности (желания) разобраться с этим вызовом, то он может просто поверить статическому анализатору и удалить "лишнюю" проверку. Что затем приведет к потере работоспособности участка кода.


              И да, здесь нет аналогии с бензопилой и ломом. Вполне себе обычный код, без каких-либо хитрых трюков. Т.к. С++ является императивным языком, разрешающим различные побочные эффекты, к выдаче предупреждений о том, что какие-то проверки не имееют смысла, следует относится с большой осторожностью.

                +2

                С другой стороны, такой пример попахивает не очень хорошо, и если анализатор подсвечивает этот code smell, в этом тоже есть определенная польза, кто-то может захотеть отрефакторить его более очевидным способом.
                Потому что кто-то может удалить проверку, которая выглядит избыточной, и без подсказки анализатора.

                  +2
                  Подобный паттерн на самом деле достаточно распространён. Например, для глобальной регистрации тест кейсов движке юнит-тестов такое будет. Или ещё какой-то реестр объектов, по результатам обхода которого формируется состояние или отчёт.
                    +3
                    Проблема в том, что здесь мы играем в «мой data-flow-анализ против твоего data-flow-анализа», т.е. в «сможет статический анализатор использовать всю информацию, которая доступна компилятору, или нет». Понятно, что без встраивания очень глубоко в сам компилятор ответ будет «нет, не сможет», но даже тот факт, что анализатор на этот код указывает как на странный — это уже благо, потому что код этот воняет, и может сломаться от случайного рефакторинга, как уже выше написали до меня. False positive намного лучше true negative, и намного лучше незнания о том, что код может перестать работать в случайного изменения в другом месте, а вы потом потратите две недели в попытках понять, что вообще произошло, и почему у вас результат сборки в дизассемблере так сильно отличается от модели этого результата в голове.
                      0
                      потому что код этот воняет

                      И как же его переписать, чтобы не вонял?

                        0
                        Перепишите проверку if (0 == a[0]) так, чтобы компилятор не смог использовать оптимизацию sequential read/compare elimination, например положите его внутрь no-inline-функции с побочными эффектами, таким же образом, как вы уже сделали с func_with_side_effects(). Повторное сравнение одного и того же элемента non-volatile-массива с константой само по себе не меняет состояние абстрактной машины, которой оперирует стандарт, и потому может быть пропущено в полностью конформной программе на С и С++ благодаря as-if rule. Хотите сделать так, чтобы сравнение это туда не попало — скажите компилятору об этом явно.
                          0
                          Перепишите проверку if (0 == a[0]) так, чтобы компилятор не смог использовать оптимизацию sequential read/compare elimination

                          А почему вы решили, что компилятор здесь может использовать эту оптимизацию?

                            +2
                            Уже объяснил, потому что повторная проверка этого условия сама по себе не меняет состояние абстрактной машины, которой оперирует стандарт. Пока ваша func_with_side_effects() трогает a[] — все скорее всего будет работать, как только при очередном рефакторинге перестанет, компилятор праве сразу же выбросить повторную проверку и начать возвращать -1 прямо из первой.

                            Если это для вас ожидаемое поведение — отлично, только вот для большинства пользователей статического анализатора это большой сюрприз, и сообщение об этой повторной проверке для них — это не ложное срабатывание, а указание на то, что осторожно, здесь в засаде сидит медведь, если вы его туда посадили сами — классно, но если нет — обратите внимание, он там.
                              +1
                              как только при очередном рефакторинге перестанет

                              Как только перестанет, это уже будет другой код. И о другом коде можно будет вести другой разговор. Пока речь идет о вполне себе корректном случае, где анализатор вводит в заблуждение.


                              Можно оправдываться тем, что от false positive никто не застрахован. Вполне понятная позиция.

                                +1
                                Ваша позиция тоже понятная, вопросов больше не имею. Меня спросили почему код воняет — я ответил так, как я сам это понимаю. Меня в детстве безопасники покусали, и теперь я предпочитаю дуть на воду, чем обжигаться на молоке, особенно в проектах, над которыми работает больше сотни людей каждый день, а баги в нем стоят миллионы настоящих долларов.
                                  +1
                                  особенно в проектах, над которыми работает больше сотни людей каждый день, а баги в нем стоят миллионы настоящих долларов.

                                  Звучит как "сперва добейся".

                                    +3
                                    Не приведи рандом добиваться этого. Хотите считать это «сперва добейся» — ваше право, я делюсь тем опытом, который имею, и теми условиями, в которых приходится работать, если у вас другие условия — отлично, это не значит что вам нужно стремиться к моим, или мне — к вашим. В моем мире не просто «с анализатором лучше», а давно уже «без анализатора полный конец обеда», если у вас не так — я реально рад.
                                      +3
                                      В моем мире не просто «с анализатором лучше», а давно уже «без анализатора полный конец обеда», если у вас не так — я реально рад.

                                      Я наивно полагаю, что целью маркетинговой программы PVS-Studio является продвижение своего продукта не только в нишу mission-critical, где анализаторы — это и так уже устоявшаяся практика, но и в нишу обычного говнокодинга, где нет ни бюджетов в сотни миллионов долларов, ни возможностей нанимать выпускников MTI, ни проектных команд в 100500 человек.


                                      Так что можно сделать замечательный дефектоскоп, который отыскивает микротрещены в карбоновых корпусах болидов Формулы-1, а механики формулических команд будут с удовольствием рассказывать насколько этот дефектоскоп облегичил им жизнь (и все это будет чистой правдой)… Но ссылки на опыт Формулы-1 будут мало чего значить для владельца автомастерской в условном Урюпинске.

                                        +1
                                        КМК встраивание анализатора в среду разработки и прививание привычки не допускать потенциальных багов никому не помешает, даже владельцу автомастерской, скорее странно что это до сих пор не стало нормой. Но когда-то же программировали и без отладчика.

                                        А PVS-Studio допускает бесплатное использование для некоммерческих проектов. И если некий разработчик возьмёт на свою совесть скормить анализатору кусочек коммерческого кода, в котором подозревает подвох, КМК никто не будет в обиде.
                                          +1

                                          Тут речь не о том, нужен ли автомастерской в условном Урюпинске дефектоскоп или нет (скорее всего нужен). А о том, подойдет ли опыт механика из Формулы-1 в реалиях урюпинской автомастерской. Ведь одно дело обнаружить микротрещину на носовом обтекателе болида Формулы-1, где эта микротрещина может реально привести к разрушению в условиях гонки. И другое дело обнаружить микротрещину на капоте Лады Калина, с которой эта самая Лада Калина пробегает еще лет 10.


                                          Ну и как следствие: в mission-critical софте false-posivite от анализатора может восприниматься как благо, как указание на потенциальную угрозу жизни пользователей софта. Тогда как в провинциальном говнокодинге аналогичный false-posivite — это отвлечение внимания разработка и удлинение сроков разработки (соответственно, увеличение стоимости).

                                            +1
                                            Разгребание false-positive улучшает и кодовую базу, и самих разработчиков даже в Урюпинске. Понятно, что если вас код плюс-минус устраивает, и он сам по себе не является целью или ценностью, а только средством решения каких-то других проблем, тратить ресурсы на внедрение статического анализа нужно с умом.

                                            Правда же сермяжная состоит в том, что чем раньше ошибку настоящую удалось заметить, тем дешевле ее починить, и в Урюпинске это не чуть ни менее верно, чем в Зеленограде, Берлине, Тель-Авиве, или Сан-Франциско.

                                            Анализатор в существующий CI/CD-пайплайн добавляется без существенных затрат, и сам он стоит далеко не как самолет (а с разработчиками еще и на скидку можно договориться гораздо чаще, чем нет), и при этом он снижает нагрузку на все последующие этапы производства ПО, потому что ловит тупые ошибки программирования вроде копи-пасты неудачной прямо на излете, еще до попадания их в репозиторий или основную ветку.

                                            Если действительно учли сроки и бюджет, изучили количество ложных срабатываний на своем коде, и выяснили, что для вас лично анализатор не нужен или даже вреден — честь вам и хвала, снимаю шляпу.
                                –1
                                Уже объяснил, потому что повторная проверка этого условия сама по себе не меняет состояние абстрактной машины, которой оперирует стандарт. Пока ваша func_with_side_effects() трогает a[] — все скорее всего будет работать

                                А вы можете определиться с тем "скорее всего будет работать" или все-таки "сама по себе не меняет состояние"?

                                  +1
                                  Вы понимаете выражение «сама по себе»? У вас в вопросе дихотомия ложная, потому что это одновременно и «скорее всего будет работать» и «проверка сама по себе не меняет состояние», потому что состояние меняет не проверка, а другая функция.
                                    –1
                                    Вы понимаете выражение «сама по себе»?

                                    Я не понимаю причинно-следственных связей между двумя вашими предложениями, а именно:


                                    Уже объяснил, потому что повторная проверка этого условия сама по себе не меняет состояние абстрактной машины, которой оперирует стандарт.

                                    Звучит как будто вы пишете о том, что вы понимаете. Т.е. здесь вы в себе уверены.


                                    Пока ваша func_with_side_effects() трогает a[] — все скорее всего будет работать

                                    А здесь уже сомнения: "скорее всего будет". Т.е. здесь уже не уверены.


                                    Как из уверенности проистекает неуверенность мне не понятно.


                                    Поэтому было бы интересно узнать: приведенный код корректен и будет работать (просто потому, что корректен) или же он может не работаеть, т.к. некорректен?

                                      +2
                                      Приведенный код корректен и будет работать как задумано пока побочный эффект функции func_with_side_effects() приводит к невозможности применения компилятором sequential read/compare elimination. Вот этот инвариант, и подобные ему, чаще всего существует только в голове у автора кода, и только пока он не забыл его. Дальнейшее сохраниение этого инварианта в условиях постоянной работы над этим кодом людьми, которые не знают о нем ничего — дело исключительно случая, и потому целесообразно с точки зрения безопасника считать что он не выполняется уже сейчас. Поэтому и «скорее всего». Если у вас код пишется одним программистом и не изменяется потом вообще никогда, и все инварианты этот отличный программист поддерживает своими силами — смело считайте срабатывание статического анализатора на этом коде ложным. Я же со своей стороны так сделать не могу и не стану, прошу искреннего пардона.
                                        0
                                        Приведенный код корректен и будет работать как задумано пока побочный эффект функции func_with_side_effects() приводит к невозможности применения компилятором sequential read/compare elimination.

                                        Т.е. по факту:


                                        • код корректен;
                                        • анализатор ошибся.

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

                                        Чаще всего такой код приходится писать потому, что лучшего решения силами существующей команды, в условиях существующих сроков и бюджетов, просто не представлялось возможным найти.


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


                                        например положите его внутрь no-inline-функции с побочными эффектами, таким же образом, как вы уже сделали с func_with_side_effects()

                                        Т.к. вы предложили способ удовлетворить анализатор, а не способ предолеть изначальную хрупкость кода (хрупкость проистекает из-за использования функций с побочными эффектов).


                                        И тут ж хотелось бы определится: мы решаем проблему кода (что может быть крайне сложно, т.к. есть ограничения команды, сроков, бюджетов) или же мы удовлетворяем анализатор.


                                        С моей точки зрения анализатор здесь себя повел неадекватно. У него была информация о том, что неконстантная ссылка на локальные данные утекает куда-то во внешний контекст, а внутри функции вызваются внешние функции с хз какими побочными эффектами. И в таких условиях говорить разработчику "чувак, у тебя здесь ошибка" вместо "чувак, а ты точно уверен, что здесь нужна проверка, т.к. локальных модификаций я здесь не вижу" — это (как по мне) введение разработчика в заблуждение.


                                        Так что в эту игру можно играть и вдвоем: разработчики анализаторов рассказывают на примерах как анализаторы отлавливают ошибки, а пользователи анализаторов на других примерах рассказывают, как анализатор берет на себя слишком много.

                                          +3
                                          И в таких условиях говорить разработчику «чувак, у тебя здесь ошибка» вместо «чувак, а ты точно уверен, что здесь нужна проверка, т.к. локальных модификаций я здесь не вижу» — это (как по мне) введение разработчика в заблуждение.
                                          Мне кажется, мы с вами принципиально по разному подходим к интерпретации сообщений анализатора. Для меня они всегда указывают не на реальные ошибки, а на потенциальные, и все равно человек в итоге принимает решение, ошибка это или нет. Уже писал, что для меня ложные срабатывания анализатора — это уже сигнал, что с кодом что-то не так, и стоило бы посмотреть на него внимательнее, и возможно улучшить так, чтобы у анализатора не было претензий, но и блокировать релиз для всего этого я тоже не стану.
                                +2
                                Отдельным комментарием отвечу на возможный аргумент про то, что так не бывает, и это все теория без практики. Согласным с этим фактом рекомендую прочитать и осознать вот этот отличный note про memset_s
                                memset may be optimized away (under the as-if rules) if the object modified by this function is not accessed again for the rest of its lifetime (e.g. gcc bug 8537). For that reason, this function cannot be used to scrub memory (e.g. to fill an array that stored a password with zeroes). This optimization is prohibited for memset_s: it is guaranteed to perform the memory write. Third-party solutions for that include FreeBSD explicit_bzero or Microsoft SecureZeroMemory.
                                Незнание as-if rule ведет к огромному количеству реальных уязвимостей в коде на С. Если вы все еще пишите критический для безопасность код С, уважайте это правило, пожалуйста.
                              0

                              Не использовать глобальную переменную.

                                0

                                Да, но нет.


                                class Registry {
                                    int * ptr_{nullptr};
                                public:
                                    void register_pointer(int * ptr) { ptr_ = ptr; }
                                    void remove_pointer() { ptr_ = nullptr; }
                                    void increment() { if(ptr_) ++(*ptr_); }
                                };
                                
                                void func_with_side_effects(Registry & registry) {
                                    registry.increment();
                                }
                                
                                int demo() {
                                    Registry registry;
                                    int a[3]{ 0, 1, 2 };
                                
                                    struct RegistryModificator {
                                        Registry & registry_;
                                        RegistryModificator(Registry & registry, int * ptr)
                                            : registry_{registry}
                                        {
                                            registry_.register_pointer(ptr);
                                        }
                                        ~RegistryModificator() {
                                            registry_.remove_pointer();
                                        }
                                    } registry_modificator{registry, a};
                                
                                    if(0 == a[0]) {
                                        func_with_side_effects(registry);
                                        if(0 == a[0]) return -1;
                                    }
                                
                                    return 0;
                                }
                                
                                int main() {
                                    return demo();
                                }

                                Никаких глобальных переменных. Тоже самое сообщение об ошибке.

                        0
                        А PVS-Studio точно выдаст предупреждение на этом коде?
                          +1

                          Перед тем, как его сюда запостить, я проверил его на их on-line демонстраторе. Может, конечно, делал что-то не так, но диагностику от PVS-Studio получил.

                        +3
                        Andrey2008, а Windows XP проверять будете? ))
                          +1

                          А смысл его проверять, это старый код, который уже скорее всего не используется. А если и да, предъявить ошибки всё равно некому (у Андрея может быть другое мнение)

                            +2
                            который уже скорее всего не используется

                            Вот это вы зря. Куча кода ядра по крайней мере имеет идентичный или слегка изменённый код при дизасемблировании. Да и просто интересно же, чем жили 20 лет назад. Хотя там старый компилятор с нестандартными расширениями и прочими приключениями, и я даже не знаю, как прикрутить туда анализатор, что бы посмотреть самому (( Поэтому и прошу знающего человека.
                              +1

                              Если речь именно о коде ядра, то может там и не сильно всё меняется, но маловероятно, что в коде ядра прямо так уж много багов, т.к. на этом коде основано всё остальное, включая весь прикладной софт, и ошибки бы выявились раньше (если это прямо чистой воды ошибка). Хуже если это не ошибка, а какая-то неоптимальность, но найдет ли ее анализатор?

                                0

                                С МС вполне станется баг не фиксить, потому что другой софт использует его как недокоментированную фичу и не работает без этого бага. Вроде даже они сами это рассказывали

                                  0

                                  Они рассказывали, что у них есть слой совместимости, который может имитировать поведение (возможно, косячное) определенных системных вызовов для определенных версий программ, если данные версии некорректно работают при исправленном поведении системного вызова.

                        0
                        Позвольте уточнить, компилятор может позволить себе заоптимизировать даже если в функции будет явное изменение?
                        Например:
                        void SetSynchronizeVar(int * z){(*z)++;}
                        
                        int main()
                        {
                            int flag = 0;
                            SetSynchronizeVar(&flag);
                            int X, Y = 1;
                            if (flag == 0)
                            {
                                X = Y;
                                if (flag == 0)
                                    return 1;
                            }
                            return 2;
                        }
                          0
                            0
                            Но ведь мы говорим о том что компилятор должен НЕ учесть изменения внутри функции, а потому заоптимизировать так как буд-то flag всегда равен 0. На godbolt он приведенный мной код он оптимзирует наоборот увидев что flag не будет 0.
                              0
                              Этого «должен НЕ учесть» в вашем оригинальном комментарии не было. Наоборот, он это учтет, и выкинет все дальнейшие проверки как always false. PVS-Studio, кстати, в compiler explorer'е есть тоже, и она действительно выдает неверное предупреждение
                              11:1: error: V547 Expression 'flag == 0' is always true на вторую проверку.
                              Вот хороший пример того, что data-flow-анализ у компилятора оказался лучше, чем у анализатора, и вот это предупреждение — действительно ложное срабатывание.
                                0

                                Таки это предупреждение верно, и в этом месте условие действительно всегда верно. Если бы этот код мог быть выполнен, да.
                                Другое дело, что компилятор смог убедиться, что условие с самого начала всегда ложно — скорее всего он предподсчитал flag и смог выкинуть весь if.

                          +7

                          Зря вы вторую часть написали, щас начнется по новой (ну, вон, уже началось).


                          Сам я хоть PVS-Studio и не пользуюсь (пишу на C/C++ настолько редко и настолько мало, что в этом нет смысла), но возможностями статических анализаторов, встроенных в IDE семейства IntelliJ IDEA, пользуюсь постоянно, на разных языках. Ложные срабатывания бывают, да. Но в подавляющем большинстве случаев ложное срабатывание заставляет переписать код более по-человечески, так, что не только анализатору, но и человеку код понятнее. А если это не тот случай — ну, что ж, замьютить — это хоткей ткнуть (и багрепорт написать, по-хорошему. Многое правят).

                            +1

                            Сугубо субъективно. Заранее извините.


                            Думать об анализаторах как о средстве поиска ошибок мне кажется не правильно, скорее это средство поиска потенциально проблемных мест.


                            Когда анализатор работает он говорит: "Чувак, я думаю тут лажа — проверь", но он не говорит "Чувак, тут ошибка — исправь". Поэтому анализаторы должны работать с паре с программистом, который знает о системе больше анализатора и сможет принять решение. Может там вообще умудрились подпаяться к транзисторам внутри проца и в любой момент поменять состояние системы (фантазия), анализатор не может знать об этом — для этого нужен программист.


                            С DMA пример был вообще из фантастики. Что планируется ожидать при чтении из буфера, с которым в текущий момент работает DMA? Судя по примеру проверчется элемент, в который DMA еще не записал данные. Для подобных проверок существуют спец. флаги в регистрах, которые нужно смотреть, но не значения в буфере.

                              +1
                              Регистры эти часто memory mapped, а следовательно спец. флаги проверяются точно так же, как значения в буфере — чтением определённого адреса памяти. Не эту переменную требуется объявить volatile, так другую.
                              +2
                              Такой подход контрпродуктивен. Неидеальный инструмент вполне может быть полезен, а его использование экономически целесообразным.

                              Согласен с этим утверждением. Но и, признаться честно, технический евангелизм как практика у меня тоже особых симпатий не вызывает.
                                0
                                Из комментариев узнал что у PVS-studio есть онлайн-проверка, скормил ей одну из ошибок, допущенную мной когда-то. Он ее не увидел:
                                #include <stdio.h>
                                #include <inttypes.h>
                                
                                #define FIFO_BITS 8
                                #define FIFO_SIZE (1<<FIFO_BITS)
                                #define FIFO_MASK (FIFO_SIZE-1)
                                
                                typedef struct{
                                  uint8_t st, en;
                                  uint8_t buf[FIFO_SIZE-1];
                                  uint8_t some_data;
                                }fifo_t;
                                
                                void fifo_reset(fifo_t *f){
                                  f->st = 0; f->en = 0; f->some_data = 100;
                                }
                                unsigned int fifo_size(fifo_t *f){
                                  return (f->st - f->en) & FIFO_MASK;
                                }
                                void fifo_push(fifo_t *f, uint8_t data){
                                  if(fifo_size(f)==1)return;
                                  f->buf[f->en] = data;
                                  f->en++;
                                  f->en &= FIFO_MASK;
                                }
                                int16_t fifo_pop(fifo_t *f){
                                  if(fifo_size(f)==0)return -1;
                                  uint8_t tmp;
                                  tmp = f->buf[f->st];
                                  f->st++;
                                  f->st &= FIFO_MASK;
                                  return (int16_t)tmp;
                                }
                                
                                int main(){
                                  fifo_t fifo;
                                  fifo_reset(&fifo);
                                  for(int i=0; i<10; i++)fifo_push(&fifo, i);
                                  for(int i=0; i<10; i++)fifo_pop(&fifo);
                                  for(int i=0; i<1000; i++){
                                    fifo_push(&fifo, i);
                                  }
                                  for(int i=0; i<10000; i++){
                                    int16_t res = fifo_pop(&fifo);
                                    printf("%i: %i (%i)\n", i, (int)res, fifo.some_data);
                                    if(res < 0)break;
                                  }
                                }

                                Если что, ошибка не в форматах printf (данный пример я воспроизводил чуть ли не по памяти), а в выходе за пределы массива.
                                  –3

                                  Задолбали со своим нытьем. Если людей не устраивает отсутствие гарантий со стороны анализатора (который к тому же скорее всего ещё и платный), значит проблема не в людях.


                                  Не нужно учить хабаровчан как правильно любить ваш продукт. Пожалуйста, воздержитесь от написания третей статьи.

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

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