Andrey2008 Apr 29 2019 at 16:52

Находим баги в LLVM 8 с помощью анализатора PVS-Studio

25 min

7.3K

PVS-Studio corporate blogOpen source*C++*Compilers*DevOps*

+46

Comments 23

KanuTaH Apr 29 2019 at 18:16

В начале содержимое объекта перемещается, а затем он используется как ни в чём не бывало

Это не всегда плохо. В случае конкретно «умных указателей» — да, использование после перемещения может свидетельствовать о проблеме, но в общем случае это не так. Перемещенный объект вполне можно переиспользовать заново (это ведь просто default constructed объект как правило), это обычная оптимизация и далеко не всегда ошибка.

-3

Andrey2008 Apr 29 2019 at 19:40

> Это не всегда плохо.
Анализтор предупреждает только об аномальном использовании.

KanuTaH Apr 29 2019 at 21:34

Там, кстати, довольно забавный баг, такой, я бы сказал, механический. В LLVM 7 setNewProgram() принимал просто указатель, и внутри просто присваивал его переменной-члену класса, при этом никак не меняя сам указатель, то есть использовал его наподобие shared_ptr. В LLVM 8 глобально перешли с голых указателей на умные, и это место как-то «механически» переделали через unique_ptr, а логика осталась вся старая :) Какой-то «баг массового механического переписывания».

Livid Apr 30 2019 at 08:21

Извините, но ЕМНИП по стандарту использование объекта после std::move небезопасно (кроме операций, не зависящих от состояния объекта). Потому что состояние объекта после перемещения стандартом определено только как "valid but unspecified", i.e. там может быть всё что угодно, и сказать, что именно там будет, не зная деталей реализации компилятора и стандартной библиотеки, нельзя. Так что такая "оптимизация" — непереносимый код. Сюрпризы теоретически могут быть даже при смене версии компилятора.

-1

mayorovp Apr 30 2019 at 08:33

Поведение перемещения полностью задаётся кодом на С++, так что от компилятора оно не зависит. А вот от версии стандартной библиотеки — зависит.

Livid Apr 30 2019 at 15:04

Да, с компилятором это я загнул, как-то с утра пораньше у меня в голове "компилятор" и "стандартная библиотека" смешались.

KanuTaH Apr 30 2019 at 12:43

std::move() сам по себе ничего не делает. Это по сути всего лишь приведение типа для того, чтобы вызвался нужный метод (принимающий в качестве аргумента rvalue-ссылку). Если вы точно знаете, что делает вызываемый метод (например, он написан вами), то ничего "небезопасного" или "непереносимого" в этом нет.

mayorovp Apr 30 2019 at 13:42

Однако, использование объекта после перемещения все же повышает метрику wtf/мин, даже если не делает ничего небезопасного.

KanuTaH Apr 30 2019 at 13:46

Ну понятное дело, что типичный reuse перемещенного объекта должен выглядеть примерно так: прочли сериализованный объект из потока — проделали с ним какие-то дополнительные манипуляции — переместили, прочли в ту же переменную следующий объект — манипуляции — переместили, и так далее. Просто комментарий выше выглядит так, будто сам по себе std::move() что-то такое проделывает с объектом, после чего его состояние как-то меняется, причем зависимым от компилятора/stdlib образом, а это не так. Вот я товарища и поправил.

Livid Apr 30 2019 at 15:11

Ну понятно что не от самого std::move, а от конструктора перемещения или там присваивания с перемещением. Строгие формулировки мне с утра не удаются. Но что STL никаких гарантий касательно состояния объекта после перемещения не даёт кроме того что оно "valid" — факт. И сторонние библиотеки обычно не дают. А в своём коде конечно можно вообще семантику перемещения поменять как угодно, да вот только стоит ли?

technic93 Apr 30 2019 at 08:54

Они же проверяют свой ког clang анализатором? Т.е это наглядно показывает что pvs находит больше или как минимум что то новое я правильно понял посыл статьи?)

Andrey2008 Apr 30 2019 at 09:40

Да.

Andrey2008 Apr 30 2019 at 10:00

Начались правки кода: bugs.llvm.org/show_bug.cgi?id=30996

mayorovp Apr 30 2019 at 10:35

Наверное, вот эта ссылка будет более понятной: https://bugs.llvm.org/showdependencytree.cgi?id=41655

Occama Apr 30 2019 at 11:46

Это весьма типовая ситуация, когда ошибка прячется в обработчике ошибок, так как их никто не тестирует.

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

tagir_valeev May 1 2019 at 18:17

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

datacompboy Apr 30 2019 at 12:01

Что-то как-то мне всё меньше нравятся недавние единорожки… Сменили дизайнера? Что-то тут он как-то «Хопэ, парниша» напоминает позой :(

А вот кофейный расчудесен, не хватает его в календарике..!

«Подозрительный брейк» напоминает недавниее обсуждение тут с

 switch (x) {

 break; case Y: ...

 break; case Z: ...

 }

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

Cerberuser Apr 30 2019 at 14:11

Можно пояснить для не особо продвинутого кодера, а как оно должно работать?..

datacompboy Apr 30 2019 at 14:29

оно защищает от забытого бряка.
когда все кейсы пишутся в форме «break; case X:» потерять бряк становится тяжело.
в свежих плюсах это починили через [[fallthrough]] аннотацию.