Анализатор кода не прав, да здравствует анализатор

    Foo(std::move(buffer), line_buffer - buffer.get());

    Совмещать много действий в одном выражении языка C++ плохо, так как такой код тяжело понимать, тяжело поддерживать, так в нём еще и легко допустить ошибку. Например, создать баг, совмещая различные действия при вычислении аргументов функции. Мы согласны с классической рекомендацией, что код должен быть прост и понятен. И сейчас рассмотрим интересный случай, когда формально анализатор PVS-Studio не прав, но с практической точки зрения код всё равно стоит изменить.

    Порядок вычисления аргументов


    То, что будет сейчас рассказано, — это продолжение старой истории о порядке вычисления аргументов, про которую мы писали в статье "Глубина кроличьей норы или собеседование по C++ в компании PVS-Studio".

    Краткая суть заключается в следующем. Порядок вычисления аргументов функции — это неуточненное поведение. Стандарт не регламентирует, в каком именно порядке разработчики компиляторов обязаны произвести вычисление аргументов. Например, слева направо (Clang) или справа налево (GCC, MSVC). До стандарта C++17, когда при вычислении аргументов возникали побочные эффекты, это могло приводить к неопределённому поведению.

    С появлением стандарта C++17 ситуация изменилась в лучшую сторону: теперь вычисление аргумента и его побочные эффекты начнут выполняться лишь с того момента, как будут выполнены все вычисления и побочные эффекты предыдущего аргумента. Однако, это не значит, что теперь нет места для ошибки.

    Рассмотрим простую тестовую программу:

    #include <cstdio>
    int main()
    {
      int i = 1;
      printf("%d, %d\n", i, i++);
      return 0;
    }

    Что распечатает этот код? Ответ, по-прежнему, зависит от компилятора, его версии и настроения. В зависимости от компилятора может быть распечатано как "1, 1", так и "2, 1". И действительно, воспользовавшись Compiler Explorer я получит следующие результаты:

    • программа, скомпилированная с помощью Clang 11.0.0, выдаёт "1, 1".
    • программа, скомпилированная с помощью GCC 10.2, выдаёт "2, 1".

    В этой программе нет неопределённого поведения, но есть неуточнённое поведение (порядок вычисления аргументов).

    Код из проекта CSV Parser


    Вернёмся к фрагменту кода из проекта CSV Parser, о котором я упоминал в статье "Проверка коллекции header-only C++ библиотек (awesome-hpp)".

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

    std::unique_ptr<char[]> buffer(new char[BUFFER_UPPER_LIMIT]);
    ....
    this->feed_state->feed_buffer.push_back(
        std::make_pair<>(std::move(buffer), line_buffer - buffer.get()));

    Предупреждение PVS-Studio: V769 The 'buffer.get()' pointer in the 'line_buffer — buffer.get()' expression equals nullptr. The resulting value is senseless and it should not be used. csv.hpp 4957

    На самом деле, мы оба неправы, и никакой ошибки нет. Про нюансы будет дальше, а пока начнём с простого.

    Итак, давайте разберёмся, почему опасно писать код следующего вида:

    Foo(std::move(buffer), line_buffer - buffer.get());

    Я думаю, вы догадываетесь про ответ. Результат зависит от последовательности вычисления аргументов. Рассмотрим это на следующем синтетическом коде:

    #include <iostream>
    #include <memory>   
    
    void Print(std::unique_ptr<char[]> p, ptrdiff_t diff)
    {
        std::cout << diff << std::endl;
    } 
    
    void Print2(ptrdiff_t diff, std::unique_ptr<char[]> p)
    {
        std::cout << diff << std::endl;
    } 
    
    int main()
    {
        {
            std::unique_ptr<char[]> buffer(new char[100]);
            char *ptr = buffer.get() + 22;
            Print(std::move(buffer), ptr - buffer.get());
        }
        {
            std::unique_ptr<char[]> buffer(new char[100]);
            char *ptr = buffer.get() + 22;
            Print2(ptr - buffer.get(), std::move(buffer));
        }
        return 0;
    }

    Вновь воспользуемся Compiler Explorer и посмотрим результат работы этой программы, собранной разными компиляторами.

    Компилятор Clang 11.0.0. Результат:

    23387846
    22

    Компилятор GCC 10.2. Результат:

    22
    26640070

    Результат ожидаем, и писать так нельзя. О чём, собственно, и предупреждает анализатор PVS-Studio.

    На этом бы хотелось поставить точку, но всё немного сложнее. Дело в том, что речь идёт о передаче аргументов по значению, а при инстанцировании шаблона функции std::make_pair всё будет иначе. Продолжим погружаться в нюансы и узнаем, почему PVS-Studio в данном случае неправ.

    std::make_pair


    Обратимся к сайту cppreference и посмотрим, как менялся шаблон функции std::make_pair.

    Until C++11:
    template< class T1, class T2 >
    std::pair<T1,T2> make_pair( T1 t, T2 u );
    Since C++11, until C++14:
    template< class T1, class T2 >
    std::pair<V1,V2> make_pair( T1&& t, T2&& u );
    Since C++14:
    template< class T1, class T2 >
    constexpr std::pair<V1,V2> make_pair( T1&& t, T2&& u );
    Как видите, когда-то давным-давно std::make_pair принимал аргументы по значению. Если бы в те времена существовал std::unique_ptr, то рассмотренный выше код действительно был некорректным. Работал бы ли этот код или нет, зависело от везения. На практике, конечно, такая ситуация бы никогда не возникла, так как std::unique_ptr появился в C++11 как замена std::auto_ptr.

    Вернёмся в наше время. Начиная с версии стандарта C++11, конструктор начал использовать семантику перемещения.

    Здесь есть тонкий момент в том, что std::move на самом деле ничего не перемещает, а всего-навсего производит преобразование объекта к rvalue-ссылке. Это позволит std::make_pair передать указатель новому std::unique_ptr, оставив nullptr в исходном умном указателе. Но эта передача указателя не произойдет, пока мы не попадём внутрь std::make_pair. К тому времени мы уже вычислим line_buffer — buffer.get(), и всё будет хорошо. То есть, вызов функции buffer.get() не может вернуть nullptr в момент, когда он вычисляется, вне зависимости от того, когда именно это произойдёт.

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

    Король умер, да здравствует король!


    Мы разобрались, что срабатывание, описанное в статье, оказалось ложным. Спасибо одному нашему читателю, который обратил наше внимание на особенность реализации std::make_pair.

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

    Здесь уместно вспомнить статью "False positives are our enemies, but may still be your friends". Публикация не наша, но мы с ней согласны.

    Это, пожалуй, тот самый случай. Пусть предупреждение ложное, но оно указывает на место, где лучше провести рефакторинг. Достаточно написать что-то вроде этого:

    auto delta = line_buffer - buffer.get();
    this->feed_state->feed_buffer.push_back(
      std::make_pair(std::move(buffer), delta));

    А можно в данной ситуации сделать код еще лучше, воспользовавшись методом emplace_back:

    auto delta = line_buffer - buffer.get();
    this->feed_state->feed_buffer.emplace_back(std::move(buffer), delta);

    Такой код создаст итоговый объект std::pair в контейнере "по месту", минуя создание временного объекта и его перемещение в контейнер. Кстати, анализатор PVS-Studio, предлагает сделать такую замену, выдавая предупреждение V823 из набора правил по микрооптимизациям кода.

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

    Да, в данном случае повезло, и ошибки нет. Но вряд автор при написании этого кода держал в голове всё то, что мы обсудили. Скорее всего, сыграло именно везение. А другой раз может и не повезти.

    Заключение


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

    Код вида:

    Foo(std::move(buffer), line_buffer - buffer.get());

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

    Пишите код проще!


    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. The Code Analyzer is wrong. Long live the Analyzer!.
    PVS-Studio
    Статический анализ кода для C, C++, C# и Java

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

      0

      Немного не в тему, но хотелось бы, чтобы в этом примере ваш анализатор выдавал предупреждение.
      https://godbolt.org/z/v3GP6v

        +1
        Я не отношусь к команде PVS, но…

        Не совсем понял на что вы хотите предупреждение? На то что вы явно делаете некорректный reinterpret_cast??? Так на то он и reinterpret_cast чтобы позволять кастовать что угодно в что угодно, просто надо думать головой и делать или static_cast или dynamic_cast. Вообще говоря на такой reinterpret_cast наверно даже компиляторы должны ругаться если включить нужные ворнинги…
          0

          Да, на reinterpret_cast.
          Не ругается никто, реальная бага из продуктового софта, точнее не бага, но шаг вправо, шаг в лево и бага будет. Только там вместо reinterpret_cast использовался Сишный каст.
          Проблема в том, что очевидно, что это UB. Самое скверное код работает верно до момента пока, кто то не добавит еще одну виртуальную функцию.
          Тут просто так вышло, что в таблице виртуальных методов один метод и он как раз тот, что надо.
          Не ругается никто, ни компилятор, ни стат анализаторы. А ошибка скрытая есть.

            0
            Еще раз. Проблема в использовании reinterpret_cast как такогвого. Перечитал вики, такое использование разрешено стандартом. Подразумевается, что человек понимает что делает, т.к. по каким-то неведомым для компилятора причинам — «это правильно»! Может специально эти 2 разных класса имеют идентичный memory footprint по каким-то «политическим причинам» и нужно скатовать чтобы дальше компилятор правильно работал.

            Статический анализатор точно-также не может сказать что программист использовал reinterpret_cast неправильно, потому что такое использование явно разрешено стандартом! Кончено можно выдавать warning на каждый reinterpret_cast, но какой в этом смысл.

            Чтобы избегать таких ошибок нужно просто запретить reinterpret_cast кроме каках-то конкретных разрешенных случаев которые разрешит коллегия архитекоторов/сеньеров.
              +3

              Это не разрешено стандартом.
              https://en.cppreference.com/w/cpp/language/reinterpret_cast
              Не подходит ни под одно разрешенное действие с reintrpret_cast.
              Человек не понимает, что он делает. Вы вот поняли, что там происходит?
              А если виртуальные функции будут реализованы другим механизмом, отличным от таблицы виртуальных методов?

                +2
                Ну, разрешено ведь, вот же
                6) An lvalue expression of type T1 can be converted to reference to another type T2. The result is an lvalue or xvalue referring to the same object as the original lvalue, but with a different type. No temporary is created, no copy is made, no constructors or conversion functions are called. The resulting reference can only be accessed safely if allowed by the type aliasing rules (see below)

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

                Тем не менее, иногда всё же можно, и можно включить соответствующие предупреждения компилятора, а заодно и предупреждать о сишных кастах godbolt.org/z/Y9EKdr.

                Правда, -fstrict-aliasing не всем подходит. Так, например, широко известный говнокод с kernel.org с этой опцией не соберётся, потому что некто Торвальдс считает, что правила strict aliasing придумали какие-то яйцеголовые умники, чтобы мешать ему программировать.
                  0

                  Все верно вы сказали, к такой ссылке нельзя обращаться:


                  Whenever an attempt is made to read or modify the stored value of an object of type DynamicType through a glvalue of type AliasedType, the behavior is undefined unless one of the following is true:
                  • AliasedType and DynamicType are similar.
                  • AliasedType is the (possibly cv-qualified) signed or unsigned variant of DynamicType.
                  • AliasedType is std::byte (since C++17), char, or unsigned char: this permits examination of the object representation of any object as an array of bytes.
                  • Informally, two types are similar if, ignoring top-level cv-qualification:
                    — they are the same type; or
                    — they are both pointers, and the pointed-to types are similar; or
                    — they are both pointers to member of the same class, and the types of the pointed-to members are similar;

                  И в примере как раз все не так.


                  За ключи предупреждений спасибо! Но вопрос к PVS остался — он ничего не выдает, а хотелось бы.

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

                    Если хотите, мы можем на заказ сделать диагностику, которая будет ругаться на каждый reintrpret_cast :). А так просто делать — неубедительно. Такой рекомендации по поиску reintrpret_cast нет даже в параноидальной MISRA :). Есть только для кастов в стиле Си: V2533. MISRA. C-style and functional notation casts should not be performed.
                      +1
                      Когда кто-то использует reintrpret_cast, то он как-бы говорит, разойдитесь, я знаю, что делаю

                      Нет, и предупреждение компилятора это подтверждает.
                      А так просто делать — неубедительно. Такой рекомендации по поиску reintrpret_cast нет даже в параноидальной MISRA

                      Крайне странный вывод. Очевидно, он базируется на некоем пиетете перед MISRA, что недопустимо в инженерном деле. А ведь правильная цепочка логических заключение подсказывает противоположный вывод: reinterpret_cast может приводить к ошибкам -> MISRA ничего об этом не говорит -> репутация этого стандарта кодирования получает «минус»
                        0

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


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

          +2
          Вернёмся в наше время. Начиная с версии стандарта C++11, конструктор начал использовать семантику перемещения.

          Мне кажется это фраза как-то не совсем корректна. Конструктор ничего не начал использовать, там стоят форвардинг референсы, т.е он может как перемещать так и копировать в зависимости от того как его вызывают. Так что тут скорее «конструктор стал поддерживать передачу rvalue ссылки».
            –1

            30% всего, о чём вы пишите, должно входить в приговор С++. Закопать и забыть. Дело не в языке, а в его дефолтах, которые старательно поддерживают традицию Си — ошибающегося подтолкни.

              +1
              При чем тут это? Можно подумать на любом другом ООП языке нельзя добиться такого-же эффекта?
              Что-нибудь в таком вот духе написанное на python-е конечно всегда будет работать одинаково (потому что только один интерпритатор), но это не значит что так как вы ожидаете:
              class Test:
                  pass
              
              def modify(a):
                  a.val = a.val + 1
                  return a
              
              def dummy(a, b):
                  print("a: %d, b: %d" % (a.val, b.val))
              
              b = Test()
              b.val = 1
              dummy(modify(b), modify(b))
              
                +1

                Питон плохой пример, потому что весь питон полон "edge cases", про которые никто не подумали.


                Если вы возьмёте Rust (собственно, C++ с адекватными дефолтами), то там такой финт ушами не прокатит. Если кто-то borrow ссылку или даже take ownership, то следующему ничего не достанется.


                Вот пример:


                struct Foo(i32);
                
                fn bar(a:Foo, b:Foo){}
                
                fn main(){
                    let x = Foo(1);
                    bar(x, x);
                }

                Бумс, прямо вот этот случай и покрывается:


                error[E0382]: use of moved value: `x`
                 --> src/main.rs:7:11
                  |
                6 |     let x = Foo(1);
                  |         - move occurs because `x` has type `Foo`, which does not implement the `Copy` trait
                7 |     bar(x,x );
                  |         - ^ value used here after move
                  |         |
                  |         value moved here

                При этом вы можете сказать, что


                #[derive(Clone, Copy)]
                struct Foo(i32);

                И тогда всё сработает.


                Rust — это C++, в котором нет "авось прорвёмся" и шапкозакидательства. Каждый сложный и тонкий вопрос разобран и решён.

                  +3
                  Каждый сложный и тонкий вопрос разобран и решён.

                  Это только на первый взгляд в довольно простой ситуации. В более сложных ситуациях все может быть гораздо более печально.
                    +2

                    Ох. Уже 10 минут пытаюсь понять что происходит. Спасибо.


                    Ой. а почему вот так вот можно?


                    fn aux<'a, 'b, T>(_: &'a &'b i32, arg: &'b T) -> &'a T
                    {
                        return arg;
                    }

                    Причём вот так вот нельзя:


                    fn aux<'a, 'b, T>(_: &'a i32, arg: &'b T) -> &'a T
                    {
                        return arg;
                    }

                    Я в растеряности. Выглядит как баг. Это известный баг или мне его репортить?

                      0

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

                        +1

                        О, спасибо. Я пойду зарепорчу им это.
                        https://github.com/rust-lang/rust/issues/79632

                          0

                          Я смотрю, про возможность манипулировать лайфтаймами (в различных вариациях, включая возможность создания висячих ссылок на уже уничтоженную переменную с automatic storage duration, как в моем примере) регулярно репортят аж с 2015 года, а воз и ныне там.

                            0

                            Да, закрыли как duplicate. Но в целом, это всё-таки баг, а не фича. Я надеюсь.

                          0

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

                            0

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

                              0

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

                                0

                                Ну так в примере из моего комментария выше как раз такая ситуация. При выполнении функции aux (через прослойку из указателя на функцию f в foo) лайфтайм b (ссылка на автоматическую переменную в стеке) получается меньше, чем лайфтайм a (static), но компилятор, по-видимому, считает, что такого быть не может (раз тут же имеется двойная ссылка & 'a &' b), и, возможно, не проверяет лайфтайм b совсем, а также при возврате ссылки из функции спокойно допускает замену b на a. Возможно, набор выполняемых проверок лайфтаймов почему-то недостаточен именно при использовании указателей на функцию, я не копался в кишках компилятора настолько глубоко.

                                  0

                                  Всё, спасибо, поковырялся с примером по ссылке — разобрался. Почему-то думал, что речь об обратном — что якобы есть ситуация, когда наличие двойной ссылки, у которой внутренний лайфтайм меньше внешнего, но компилятор её отвергает, теперь дошло, что на самом деле ситуация обратная. К слову, если вставить отладочную печать куда-нибудь в середину процесса (хоть в aux, хоть в foo) — вывод меняется на корректный, так что попахивает неучтённым UB.

                                    +1

                                    Корректным выводом в данном случае может быть только ошибка компиляции — borrow checker должен корректно рассчитывать лайфтаймы и не позволять продлевать лайфтайм ссылки на автоматическую переменную до static. Если этот код вообще компилируется и запускается — это уже само по себе проблема, вне зависимости от того, что он выводит. Выводить он может в теории что угодно, так как мы получаем висячую ссылку на область памяти в стеке, которую ранее занимала уже уничтоженная переменная a из bar(), и там может быть все что угодно.

                                      0

                                      Именно неучтённый UB там и есть, потому что в том коде при помощи серии трюков возвращается ссылка на локальную переменную.

                  +2
                  Помнится была пара статей о том, что статические анализаторы уже не просто ищют потенциальные ошибки по шаблонам или regexp, но и умеют анализировать поток данных по коду программы. К сожалению написать туда не получается, по этому пишу сюда — как то всё таки примитивно этот поток данных анализируется, вот упрощенный код примера того, что в нескольких местах вижу в своём коде: godbolt.org/z/r1vqjr
                    +1
                    Это не такой уж и простой случай для отслеживания взаимосвязи на самом деле :).
                    0
                    Вот вам еще немного false positive с std::move: godbolt.org/z/8GEd7x
                      –1
                      А почему вы считаете, что это false positive? Объект используется после того, как он был перемещён, и вообще возможно он находится в нежизнеспособном, «заклиненном» состоянии.
                      Вот в следующем случае, можно считать проявлением false positive, предупреждения об использовании «заклиненного» объекта в перемещающем конструкторе(или операторе присваивания):
                      class Foo
                      {
                          ...
                      };
                      
                      class Boo: public Foo
                      {
                          int val;
                      public:
                          Boo(Boo&& src): Foo(std::move(src)), val(src.val) {}
                      };
                      
                        0
                        После выполнения операции перемещения объект должен находиться в «valid but unspecified state», поэтому в общем случае нет ничего, что запрещало бы его использовать, просто это нужно делать аккуратно. Его вполне можно очистить и использовать заново, или переместить в него какой-то другой объект. Ничего страшного в этом нет. Само по себе использование объекта после перемещения — не криминал.
                          +1
                          Само по себе использование опустошённого перемещением объекта, не криминал конечно, но сам факт этого действия, за исключением некоторого узкого круга use case-ов, это явно очень серьёзная заявка на неприятности, о чём собственно и предупреждения.
                          Если подключить здравый смысл, то опустошённый объект, годится лишь ради того, что бы в него скопировать или перенести значение из другого объекта. Любое другое использование очень вероятно является ошибкой, или тенденцией к ошибке. Исключением будет разве что приведённый мной пример с конструктором или оператором перемещения.
                            +1
                            На самом деле на опустошённом объекте можно использовать любую функцию или метод, у которой нет в контракте предусловий на состояние этого объекта. Тогда поведение будет defined.
                            Пример такой функции: std::vector::clear. Любое состояние объекта переводит в состояние «пустой вектор», работа с которым дальше определена. И таких функций на самом деле полно.
                          0
                          Как уже отметили, по талмудустандарту перемещение должно оставлять объект в неуказанном, но валидном состоянии. Иными словами, строка может быть пустой, а может и не быть, если SSO, но «заклиненной» она быть не может точно.
                          Любые операции, не полагающиеся на предыдущее состояние объекта (явная очистка, присвоение нового значения) — валидны.

                          Более того, PVS уже содержит подобную логику, т.к. operator= предупреждения не вызывает. Речь о том, что список исключений из этого правила неполон.

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

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