Pull to refresh

Comments 35

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

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

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

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

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

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

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

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

Ну, разрешено ведь, вот же
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 придумали какие-то яйцеголовые умники, чтобы мешать ему программировать.

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


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 остался — он ничего не выдает, а хотелось бы.

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

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

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

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

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


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

UFO just landed and posted this here

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

При чем тут это? Можно подумать на любом другом ООП языке нельзя добиться такого-же эффекта?
Что-нибудь в таком вот духе написанное на 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))

Питон плохой пример, потому что весь питон полон "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++, в котором нет "авось прорвёмся" и шапкозакидательства. Каждый сложный и тонкий вопрос разобран и решён.

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

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

Ох. Уже 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;
}

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

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

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

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

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

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

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

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

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

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

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

Помнится была пара статей о том, что статические анализаторы уже не просто ищют потенциальные ошибки по шаблонам или regexp, но и умеют анализировать поток данных по коду программы. К сожалению написать туда не получается, по этому пишу сюда — как то всё таки примитивно этот поток данных анализируется, вот упрощенный код примера того, что в нескольких местах вижу в своём коде: godbolt.org/z/r1vqjr
Это не такой уж и простой случай для отслеживания взаимосвязи на самом деле :).
А почему вы считаете, что это false positive? Объект используется после того, как он был перемещён, и вообще возможно он находится в нежизнеспособном, «заклиненном» состоянии.
Вот в следующем случае, можно считать проявлением false positive, предупреждения об использовании «заклиненного» объекта в перемещающем конструкторе(или операторе присваивания):
class Foo
{
    ...
};

class Boo: public Foo
{
    int val;
public:
    Boo(Boo&& src): Foo(std::move(src)), val(src.val) {}
};
После выполнения операции перемещения объект должен находиться в «valid but unspecified state», поэтому в общем случае нет ничего, что запрещало бы его использовать, просто это нужно делать аккуратно. Его вполне можно очистить и использовать заново, или переместить в него какой-то другой объект. Ничего страшного в этом нет. Само по себе использование объекта после перемещения — не криминал.
Само по себе использование опустошённого перемещением объекта, не криминал конечно, но сам факт этого действия, за исключением некоторого узкого круга use case-ов, это явно очень серьёзная заявка на неприятности, о чём собственно и предупреждения.
Если подключить здравый смысл, то опустошённый объект, годится лишь ради того, что бы в него скопировать или перенести значение из другого объекта. Любое другое использование очень вероятно является ошибкой, или тенденцией к ошибке. Исключением будет разве что приведённый мной пример с конструктором или оператором перемещения.
На самом деле на опустошённом объекте можно использовать любую функцию или метод, у которой нет в контракте предусловий на состояние этого объекта. Тогда поведение будет defined.
Пример такой функции: std::vector::clear. Любое состояние объекта переводит в состояние «пустой вектор», работа с которым дальше определена. И таких функций на самом деле полно.
Как уже отметили, по талмудустандарту перемещение должно оставлять объект в неуказанном, но валидном состоянии. Иными словами, строка может быть пустой, а может и не быть, если SSO, но «заклиненной» она быть не может точно.
Любые операции, не полагающиеся на предыдущее состояние объекта (явная очистка, присвоение нового значения) — валидны.

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