Комментарии 108
Для начала я проверил небольшую часть операционной системы (3.3%) и выписал около 900 предупреждений, указывающих на настоящие ошибки. Если экстраполировать результаты, то получается, что наша команда способна выявить и устранить в Tizen около 27000 ошибок.
Если честно, странная логика.
Проверив всего 1 строку программы анализатор не нашёл в ней ошибок.
Если экстраполировать результаты, то получается, что программа не содержить ошибок!
Почему то вспомнился анекдот, про жену председателя и доярку, которые в среднем обе б@$ди…
В крупных продуктах код может (и будет) неоднороден, и, в данном случае, на мой взгляд, произведённая вами экстраполяция неверна.
P.S. Мне нравится ваш продукт и ваш подход к его продвижению. И статью я прочитал, как и почти все остальные.
Вы тоже работаете в этой области, или просто теоретизируете?
ps. И экстраполяция по определению не может быть верной. Она может быть допустимой.
Я работаю в области написания этих самых продуктов (в том числе крупных).
И бывает, что какую-то часть проекта пишет один человек/команда, а другую часть — другой человек/команда. И качество кода может отличаться если не на порядки, то в разы.
А потому экстраполировать результаты проверки 3.3% проекта на весь проект нельзя. Вполне возможна ситуация, когда в выборку (в статье не указана методика выборки, мы не знаем была ли она репрезентативной) попала только часть кода, написанная условно хорошо, тогда как другая часть написана условно хуже. Или наоброт.
Вряд ли мне «повезло» взять столько проектов, относящихся к одной команде. Почти наверняка, эти проекты создаются как минимум несколькими командами.
Вы раньше сами писали, что плотность ошибок на 1000 строк кода непостоянная, и зависит от проекта.
А здесь вдруг приняли её за константу.
Так навскидку, какой доверительный интервал у этой оценки? Тысяча наберется?
Вообще, молодцы и те и другие — вы, что проверяете, они, что занимаются рефакторингом (пускай и создавая новые баги :D)
Поясните, пожалуйста, одну:
Указатель на строку argv[2] является небезопасным источником данных, так как это аргумент командной строки. При формировании строки в буфере temp может произойти выход за границу массива.Что argv[2] может быть любым, понятно. Но там же snprintf с правильным размером буфера. Как можно выйти за границу temp?
upd. Ой, невнимательно прочитал, там snprinf. Тогда присоединяюсь к вопросу.
P.S. Возможно и ещё что-то окажется не ошибкой при тщательном изучении. Но с другой стороны, при тщательном анализе и наоборот выяснится, что я пропускал некоторые ошибки. Например, я поленился изучать V730 (очень трудоёмко). А там наверняка несколько ошибок скрывается.
Зачем этот кактус с ручным выделением памяти и работой с указателями и си-строками на протяжении в 72 миллиона строчек кода? Может лучше сделайте им презентацию про std::unique_ptr, std::srting и std::vector. Из названий переменных видно что даже какой то шттп на голом си у них работает.
Функция malloc, если не может выделить память, возвращает NULL. Оператор new при нехватке памяти генерирует исключение std::bad_alloc.У меня на паре личных проектов
new
возвращает NULL
, чтобы избежать исключений. По стандарту это, конечно, неправильно, но я и не собираюсь писать свою стандартную библиотеку. Просто я выделяю память по-своему, и оператор new
выглядит красивее, чем какой-нибудь my_memory_alloc(sizeof(*foo));
Учитывая, что вы проверяете ОС — там такая перегрузка операторов очень даже возможна.
А почему не используете RAII?
this
не NULL
, то объект находится в стеке, а иначе ему нужно выделить место в куче… Если бы это не был личный проект, коллеги бы меня придушили.Возможно, пример не очень удачный (тут, в лучших традициях ООП, «отжатая» память сама должна являться объектом), но глобальная перегрузка оператора
new
запросто может встретиться в коде ОС.Кстати, по стандарту есть new(std::nothrow)
, который как раз возвращает nullptr
. Никогда сам его не использовал, правда, но работает: https://ideone.com/Z43jcF
Если требуется, чтобы оператор new возвращал nullptr, следует использовать nothrow версию оператора:Но мне это требуется вообще везде, во всем коде. И с моим выделением памяти это работать не будет.
А зачем так надо? Потому что кажется что если код не рассчитан на то, что вернётся nullptr
, то там плохо возвращать nullptr
, и уж лучше сразу std::terminate()
если исключения нельзя. А если код пишется самостоятельно, то что мешает использовать nothrow
-версию?
Во-вторых, я использую
new
, потому что он проще (размер считается автоматически) и короче. Если я начну писать везде (std::nothrow)
, то в чем будет заключаться выигрыш?Кстати, практически любую стандартную библиотеку можно собрать без исключений. Например, вот реализация из LLVM libcxx: (обратите внимание на
_LIBCPP_NO_EXCEPTIONS
)__attribute__((__weak__, __visibility__("default")))
void *
operator new(std::size_t size)
#if !__has_feature(cxx_noexcept)
throw(std::bad_alloc)
#endif
{
if (size == 0)
size = 1;
void* p;
while ((p = ::malloc(size)) == 0)
{
// If malloc fails and there is a new_handler,
// call it to try free up memory.
std::new_handler nh = std::get_new_handler();
if (nh)
nh();
else
#ifndef _LIBCPP_NO_EXCEPTIONS
throw std::bad_alloc();
#else
break;
#endif
}
return p;
}
__attribute__((__weak__, __visibility__("default")))
void*
operator new(size_t size, const std::nothrow_t&) _NOEXCEPT
{
void* p = 0;
#ifndef _LIBCPP_NO_EXCEPTIONS
try
{
#endif // _LIBCPP_NO_EXCEPTIONS
p = ::operator new(size);
#ifndef _LIBCPP_NO_EXCEPTIONS
}
catch (...)
{
}
#endif // _LIBCPP_NO_EXCEPTIONS
return p;
}
Но я так не делаю, потому что теряется вся идея переносимости стандартной библиотеки.
Получается что libcxx
без исключений не соблюдает стандарты? В отличие от libstdc++
new_handler handler = std::get_new_handler ();
if (! handler)
_GLIBCXX_THROW_OR_ABORT(bad_alloc());
handler ();
Ровно такое поведение я ожидал бы увидеть если exceptions запрещены.
Видимо отклонение от стандарта позволяет писать код быстрее, но потом не надо ожидать что его будет правильно понимать PVS-Studio, когда для семантики return nullptr on fail
есть специальный синтаксис, который можно использовать везде вместо обычного new
. Так что мне кажется что они в своём праве всегда на это ругаться.
По сравнению с malloc
выигрыш в вызове конструктора и (судя по С++ 17) учёту extra type alignment.
И я правильно понимаю что с вашим определением operator new
стандартные контейнеры совсем не используются? Потому что вроде они никак нулевой указатель обрабатывать не станут, и действительно лучше было сразу std::terminate
делать.
Видимо отклонение от стандарта позволяет писать код быстрее, но потом не надо ожидать что его будет правильно понимать PVS-StudioЕще раз напоминаю, что проверяется код ОС, где можно встретить все, что угодно. Даже чтение-запись по NULL-указателю не должны вас удивлять.
И я правильно понимаю что с вашим определением operator new стандартные контейнеры совсем не используются?С каких это пор стандартные контейнеры стали прибивать гвоздями к operator new? Там же сплошное мета-программирование, вся работа с памятью идет через отдельные абстракции, которые в итоге могут раскрыться во все, что угодно.
действительно лучше было сразу std::terminate делатьКак пользователь, прошу вас, никогда так не делайте! Уже достало, когда нехватка памяти рушит всю программу, вместо отмены запрошенной операции. Например, Tor Browser падает постоянно, хотя было бы достаточно убить пару вкладок. Встречаются графические редакторы, которые также завершаются при попытке импорта больших файлов, хотя достаточно отменить операцию импорта и вывести сообщение об ошибке.
Еще раз напоминаю, что проверяется код ОС, где можно встретить все, что угодно. Даже чтение-запись по NULL-указателю не должны вас удивлять.
Я очень удивлюсь если увижу любую реализацию libstdc++ в которой operator new()
возвращает валидный адрес 0x0
.
А в остальном: во-первых, спасибо за то что было не лень мне отвечать! Интересно подумать что же на самом деле вроде может работать, а что должно быть очень опасно.
TLDR: либо в коде должны быть написано new (std::nothrow)
, либо он не никогда не должен вернуться к исполнению после неуспеха выделения памяти в operator new
.
Безусловно можно в случае нехватки памяти вместо terminate()
попытаться сходить осободить кеши и ресурсы, или отменить текущую операцию. Но первое странно выносить за пределы работы с памятью, то есть в приципе это всё можно сделать внутри нашего operator new()
. А для второго надо либо реализовывать какой-то вариант корутин (чтобы можно было выкинуть кусок стека который про это ничего не знает), либо код, который это использует, должен быть готов к этому.
Cтандартные контейнеры не готовы в целом к тому что allocate()
вернёт невалидный указатель. Тот же самый std::list
обязательно попытается слинковать ноды, записав что-то в память.
Наверное, можно попытаться возвращать в allocate()
одинковый для всех, но хотя бы writable и хорошо выравненный кусочек памяти. А для std::vector
можно внутри MyAllocator::construct()
проверять была ли проблема с выделением памяти и если была то ничего не иницаильнизировать. Но тем самым нарушается всё больше и больше контрактов, и на каком-то этапе это может взорваться.
Лучший способ продемнострировать то, что код готов к невалидному указателю — написать лишние 15 символов на каждый new. И всем сразу всё ясно.
А если часть кода этого не ожидает, то надо прервать выполнение текущего стека. Это можно сделать разными способами, но я не могу придумать примеров когда это нужно делать за пределами operator new()
, сначала вернув оттуда невалидный указатель.
Я очень удивлюсь если увижу любую реализацию libstdc++ в которой operator new() возвращает валидный адрес 0x0.Я очень удивлюсь, если ОС будет использовать обычную user-space библиотеку. Для меня
operator new
— это, в первую очередь, конструкция языка, и лишь потом одна из реализаций.Безусловно можно в случае нехватки памяти вместо terminate() попытаться сходить осободить кеши и ресурсы <...> Но первое странно выносить за пределы работы с памятью <...>Как раз для этого есть
std::new_handler
. Он вызывается, когда памяти перестает хватать.А для второго надо либо реализовывать какой-то вариант корутинОбычный паттерн «команда» + какая-нибудь арена, чтобы разом прибить память, затраченную на операцию.
Cтандартные контейнеры не готовы в целом к тому что allocate() вернёт невалидный указатель.Получается, Google уже давно должен был бы упасть в segmentation fault? Вот как раз кусок из их реализации арены:
pointer allocate(size_type n,
std::allocator<void>::const_pointer /*hint*/ = 0) {
assert(arena_ && "No arena to allocate from!");
return reinterpret_cast<T*>(arena_->AllocAligned(n * sizeof(T),
kAlignment));
}
AllocAligned
вернет NULL
, если что-то пошло не так.protobuf не выглядит как библиотека которая использует стандартные контейнеры.
Как раз для этого есть std::new_handler
. Он вызывается, когда памяти перестает хватать.
Ну да, но это происходит в рамках аллокации, пока что управление находится внутри operator new()
, то есть невалидный указатель нам никто не возвращает
Обычный паттерн «команда» + какая-нибудь арена, чтобы разом прибить память, затраченную на операцию.
То есть вы сами пишете все строки где вызывается operator new() и можно там везде вызвать именно nothrow-версию?
AllocAligned вернет NULL, если что-то пошло не так.
Это вот этот код?
Любопытный пример.
AllocAligned()
вроде бы является по семантике (см комментарии в arena.h) просто malloc
, то есть в конце концов он там ссылается на malloc. Но:
- В
AllocNewBlock
(цепочкаAllocAligned()->GetMemory()->GetMemoryFallback()->AllocNewBlock()
) есть вызов new std::vector и скорее всего это обычный глобальный new, из которого может прийтиbad_alloc
- Потом там же есть
вызов aligned_malloc
и проверка на то что вернулось ненулевое значение. - И там же есть вызов обычного
malloc
, уже безо всяких проверок.
То есть такое впечатление что про это просто не думали c таким ранообразием сценариев обработки этого события. Свидетельством этого же является попытка записать что-то без проверки получилось ли выделить память в классе Gladiator.
Вы уверены что оно там не падает в segmentation fault когда память заканчивается?
Сейчас нет времени разбираться, но падать не должно. Этот класс, на сколько я знаю, используется также в других проектах Google.
Ну вот стек gdb в момент SIGSEGV
Program received signal SIGSEGV, Segmentation fault.
0x00000000004044ae in ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena>::construct (this=0x7fffffffda30, p=0x4) at arena-inl.h:109
109 new(reinterpret_cast<void*>(p)) T();
#0 0x00000000004044ae in ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena>::construct (this=0x7fffffffda30, p=0x4) at arena-inl.h:109
#1 0x0000000000404340 in std::allocator_traits<ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena> >::_S_construct<int> (__a=..., __p=0x4)
at /usr/include/c++/5/bits/alloc_traits.h:256
#2 0x000000000040417c in std::allocator_traits<ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena> >::construct<int> (__a=..., __p=0x4)
at /usr/include/c++/5/bits/alloc_traits.h:402
#3 0x0000000000403f41 in std::__uninitialized_default_n_a<int*, unsigned long, ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena> > (__first=0x0,
__n=9999999998, __alloc=...)
at /usr/include/c++/5/bits/stl_uninitialized.h:623
#4 0x0000000000403d13 in std::vector<int, ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena> >::_M_default_append (this=0x7fffffffda30, __n=9999999999)
at /usr/include/c++/5/bits/vector.tcc:565
#5 0x0000000000403ab3 in std::vector<int, ctemplate::ArenaAllocator<int, ctemplate::UnsafeArena> >::resize (this=0x7fffffffda30, __new_size=9999999999)
at /usr/include/c++/5/bits/stl_vector.h:676
#6 0x00000000004035bf in test_vector () at main.cc:27
#7 0x000000000040374c in main () at main.cc:55
Что очень логично — resize
сразу же начинает конструировать объекты, в данном случае по адресу nullptr
.
Наверное у Google очень много памяти) Что более реально — большой набор своих контейнеров.
Наверное у Google очень много памяти)Вообще, этот класс был создан, чтобы эффективно выделять очень маленькие куски памяти. Но ведь какая-то другая часть программы может выделить ее столько, что этот маленький кусочек уже не поместится…
Переведите ВСЕ материалы на качественный Корейский. И съездите на какую нибудь конференцию в Сеул(даже самую маленькую) по Tizen с докладом на корейском(желательно чтобы Samsung ее хостил). Вас тогда с руками и ногами оторвут. Все попытки что то сделать на английском и тем более на русском сразу летят в корзину у корейско говорящего менеджера, ибо у него нет времени переводить.
Я думаю, что серьезной фирме, которой нужен софт для проверки софта, глубоко… начхать на логотип компании, чей софт они собираются лицензировать.
Человеческий фактор — он может решать. Года назад были свои терки на хабре (=агрессивные владельцы, отсюда и ниже, про "закрытие статей и школу").
Если сейчас они изменились к лучшему, то проблем быть не должно с тем, чтобы продать продукт, ибо имейдж не страдает.
то второе условие всегда ложно и функция никогда не вернёт значение «es_US»
Так "рыжий" же деньги на строительство стены выделяет. Скоро и по стране es_US будет не найти.
Вы анализируете разные проекты… А отклик-то есть вообще или это чисто для юзеров хабра?
Интересует например как отреагировали в Qt…
Как-то прочитав очередную вашу статью (я их почти всегда читаю), я решил, что пора. Поскольку пишу для себя на скриптовых ЯПах, нашел стат. анализатор для JS, загрузил туда свой код… и получил просто тонны ошибок и предупреждений. Буквально на каждую строчку (а то и не по одному разу).
В общем, я так и не смог заставить уйти хотя бы часть этих вещей, потому что при исправлении кода согласно предупреждениям (не все получалось исправить, да), у меня ломалось поведение в браузере.
Вопрос простой: что делать?
Многие делают частую ошибку полагая, что чем больше сообщений выдаст анализатор кода, тем лучше. Вот хороший пример, почему это не так.
Теперь очень крепко подумаю, и скорее всего куплю… хм, LG тормозят, Panasonic российской плохой сборки, ломаются… остаётся Sony. Но их диспели и дизайн что-то не впечатляют. Эх, дилема.
извиняюсь за оффтоп, для меня выбор ТВ это боль.
Анализ неплох, подумаю над тем чтобы прогнать свои проекты через анализатор.
Анализаторы должны использоваться регулярно и тогда многие, в том числе и критические, ошибки будут обнаруживаться на самом раннем этапе
и
У меня ушло около недели на то, чтобы отобрать предупреждения, которые, по моему мнению, указывают на настоящие ошибки(и это жалкие 3% от общей кодовой базы).
Вот это противоречие слегка мешает внедрять стат. анализ на постоянной основе, особенно для проектов с огромным легаси, для которых порог вхождения высотой с гору.
Я думаю для проекта можно проанализировать все, а потом анализировать только изменения (новый/измененный код).
Было интересно почитать, спасибо!
> Надо понимать, что рассмотренный код очень опасен и достаточно, чтобы в одном из классов появился конструктор/деструктор или был добавлен член сложного типа (например std::string), как всё сразу сломается окончательно.
Появление конструктора в sockaddr_* это из области невероятной фантастики — это гарантированно плоские структуры данных.
Код может быть тут сколько угодно формально некорректным, исходя из формальности типа «если new, то вероятен конструктор», но сама эта формальность неуместна.
Более того, если бы это случилось, то удаление объекта через указатель на базовый класс с виртуальным деструктором — нормальная ситуация: если B и C — потомки A, а A имеет виртуальный деструктор, то код вида
A* a1 = new B(); A* a2 = new C(); ... delete a1; delete a2;
законен.
Так что тут ваша проверка некорректна, рекомендую исправить. Для семейства sockaddr лучше вообще добавить исключение (работа через sockaddr* — общее правило BSD sockets API), а в общем случае ловить удаление через базовый класс без виртуального конструктора.
А вот что я бы рекомендовал добавить — именно для sockaddr_* (всех адресных семейств) — это или использование calloc(), или заполнение структуры нулевыми байтами перед использованием. Отсутствие такого заполнения может приводить к странным эффектам при переносе между ОС: например, для BSD семейства (включая MacOS) sin_len (отсутствующее в Linux) должно быть или 0, или актуального размера.
Также это касается ряда других интерфейсных структур — с ходу вспоминается sigaction. Можно вообще поставить общим правилом для ядерных структур.
> Для таких случаев есть стандартный макрос M_PI, который вдобавок раскрывается в более точное значение.
M_PI не определён ни в стандарте C, ни в базовом Posix; есть в Posix расширении XSI и в Microsoft SDK. Утверждение, что он стандартен, распространено, но неверно. Эта диагностика также требует исправления.
> snprintf(buf + strlen(buf), sizeof(buf),
Против такого лучший идиоматический приём выглядит примерно так:
// удобно, потому что blimit уже сдвигаться не будет const char *blimit = buf + sizeof buf; // а вот этот едет каждый раз const char *bpos = buf; ... r = snprintf(bpos, blimit - bpos, порция1); // привет Шлемиэлю, но его подвиг мы не повторяем // также компилятор может соптимизировать blimit-bpos в одно вычисление bpos += min(r, blimit - bpos); r = snprintf(bpos, blimit - bpos, порция2); bpos += min(r, blimit - bpos); ...
Хотя для короткого буфера и двух записей хватит и такого strlen.
> Здесь забыты фигурные скобки.
Вообще я бы рекомендовал настаивать на включении тел if, else, while в фигурные скобки в принудительном порядке везде и всегда. Новые языки (Swift, Go) это уже делают в основах синтаксиса.
> argv2[i] = NULL;
> Не будут обнулены указатели.
> NULL будет записан за границу массива.
Обнуление не обязательно. NULL, скорее всего, не будет записан за границу, потому что массив argv в Unix-стиле передачи аргументов другим процессам всегда заканчивается ещё одним NULL, не указанным в arg_count (argc — для main).
Тут жалоба, я уверен, ложна, хотя определить это по самому коду может быть слишком сложно для анализатора.
> The 'strncat' function call could lead to the 'dd_info->object_uri' buffer overflow.
И опять strlen (см. выше), но strncat это вообще диверсия почти всегда.
В случае Unix мира лучше рекомендовать переходить на strlcat, а в общем случае — на strncat_s.
> Предупреждение PVS-Studio: V591 Non-void function should return a value.
Нормальный компилятор это вообще должен считать за ошибку. Этот код, подозреваю, вообще не компилируют.
Теперь про разные моменты.
1) sockaddr_
Не важно, появится там конструктор или нет. Код неправильный. Нельзя создавать объект одного размера, а уничтожать как объект другого размера. Да везёт, что пока всё это работает и я не могу продемонстрировать обратное (быть может кто-то поможет). Сбой может произойти по разным причинам. Например, менеджер памяти реализован так, что хранит информацию об объектах разного размера в разных таблицах и использовать разные пулы памяти. И тогда, он может не найти запись об выделенной памяти в соответствующей таблице.
Или менеджер памяти особо-секьюрный и обнуляет освобождаемую память. А тут ему подсовывают объект не того размера.
Так что анализатор ведёт себя правильно и ничего в нём исправлять не надо. Надо исправлять код программы.
Вы приводите пример и говорите об наследовании с виртуальными деструкторами. Это совсем другая ситуация и в подобном случае естественно анализатор промолчит. Я ведь специально отметил в статье, что эти 3 класса никак не связаны между собой.
2) M_PI
Согласен, местами его нет. Но раз в других местах используется M_PI, то не вижу смысла городить велосипед. Первый попавшийся M_PI в C-файле (проект org.tizen.myplace-1.0.1, файл myplace-suggestedview.c):
static inline double __rad_2_deg(double radians) { return radians * (180.0 / M_PI); }
3) argv2[i] = NULL;
Это не тот argv, который приходит в main. Это нечто самобытное:
.... argc = malloc(sizeof(int)); argv = malloc(sizeof(gchar *) *max_argc); argv2 = malloc(sizeof(gchar *) *max_argc); if (!argc || !argv || !argv2) goto ERROR; memset(argv, 0, sizeof(gchar *) *max_argc); memset(argv2, 0, sizeof(gchar *) *max_argc); ....
4) Non-void function should return a value
Я неприятно удивлю, но компиляторы до сих пор проглатывают такой код. Примеры.
Да, это аргумент (хотя такой реализации для C/C++ ни разу вживую не видел).
Тогда это надо переделать на malloc/free. Использование же через общий sockaddr* остаётся основным методом.
> Так что анализатор ведёт себя правильно и ничего в нём исправлять не надо.
Но про контроль обнуления структур рекомендую таки подумать.
> Это не тот argv, который приходит в main. Это нечто самобытное:
Я и не говорил, что это входной argv данного процесса. Это, скорее всего, выходной, для передачи в какой-то из exec*(). И он обязан завершаться NULLʼом, иначе дочерний процесс не запустится правильно.
> Я неприятно удивлю, но компиляторы до сих пор проглатывают такой код.
MSVC, да?
Либо вы не понимаете, что стоит за словами data-flow-analysis, либо просто нагло врете.
Просто ради интереса загрузил с вашего сайта анализатор и декомпилировал первой попавшийся под руку тулзой.
Никакого анализа потока данных у вас не вижу абсолютно.
Ожидал, что хоть хваленный механизм виртуальных значений будет честно строится, но нет…
Все работает крайне наивным способом, разбито на какие-то частные случаи, проходах по AST, и с кучей дублирования кода.
Я не буду тут приводить участки кода, просто скажу, что после увиденного, рекомендовать и уж тем более покупать ваш продукт отпало сразу.
Я еще лет двадцать назад взял за правило писать код так, чтобы он компилировался с /Wall (у меня MS VC++). Для этого, как правило, требуется простая аккуратность, иногда приходится отключать отдельные предупреждения локально, а глобально — только самые малополезные, вроде «function not inlined».
Чужой же код в большинстве случаев неспособен компилироваться даже с /W4, не говоря уже о /Wall. Более того, в среде «чистых сишников» преобладает убеждение, что повышенный уровень предупреждений — для новичков или тупиц, а «настоящий сишник» попросту обязан использовать различные трюки, на которые обычно и возбуждается компилятор.
Что интересно, часть ошибок и подозрительных мест, обнаруживаемых статическими анализаторами, успешно обнаруживают и современные компиляторы с выкрученным на максимум уровнем предупреждений.
Да, это так. Однако помимо того, что статические анализаторы мощнее, есть ещё одно важное отличие. Они больше внимания уделяют борьбе с ложными срабатываниями, пытаясь угадать, используется особый стиль написания кода, или это действительно ошибка.
Поэтому часто бывает, что диагностика вроде одна и та-же, но компилятор даёт так много шума, что проще её выключить. А анализатор на том же самом коде может давать вполне вменяемый результат, который возможно изучить, найти ошибки, а затем донастроить, чтобы ложных срабатывания вообще пропали.
На какие «особые стили» компилятор может давать так много шума, что проще выключить предупреждения, считая стиль более ценным?
Да, такие есть. Имя им – легион.
Мне сложно сходу приводить примеры. Первое, что вспоминается, это диагностика C4265 (class has virtual functions, but destructor is not virtual), которая отключена по умолчанию. Можно конечно сказать, что диагностика сделана плохо, но формально она звучит правильно. Однако, если используется паттерн примесь (подмешивание), то эта диагностика мешает. Подробности здесь.
Собственно, можно взять любой большой проект, который не рассчитан на /Wall, скомпилировать его с /Wall и получить массу предупреждений. И вот как только возникнет мысль, что не хочу вот тут править код, а проще выключить предупреждения, так вот это и есть тот самый случай, когда стиль важнее дурацкого предупреждения. :)
Когда уже есть проект, не рассчитанный на /Wall, вопросов к его продолжателям нет. :) Вопрос к разработчикам проекта о том, почему в нем слишком много кода, вызывающего предупреждения.
Почему диагностика сделана плохо?
Потому, что на практике она даёт столь много бесполезных сообщений, что её выключили.
Если они столь бесполезны — для чего ваш анализатор их выдает?
Я многократно отмечал в статьях и комментариях, что если мы и делаем диагностики схожие с диагностиками компилятора, то только в том случае, если можем сделать их чем-то лучше/полезнее/удобнее. Просто повторять смысла конечно нет.
Приведённый пример, был как раз одним из них. Прошу познакомиться с ссылкой, которую я привёл ранее. Там разобрано, как работает диагностика Visual C++ и наша.
Диагностика V599 PVS-Studio обладает низким процентом false positives и хорошо находит ошибки, на которые ориентирована. А если хочется посмотреть на все классы без виртуальных деструкторов (но содержавших другие виртуальные функции), то можно включить C4265 и наслаждаться.
Ещё пример.
#define M1 100
#define M2 100
int F(int a)
{
if (a == M1 || a == M2)
a = 42;
return a;
}
Очень частая ситуация. Числа объявлены через макросы и обозначают разные сущности, но по факту имеют одно и тоже числовое представление. Анализатор Visual C++ выдаёт здесь
warning C6287: Redundant code: the left and right sub-expressions are identical.
Что приводит к сильному шуму в некоторых проектах. Мы же учитываем подобные ситуации и специально молчим. Это было одно из первых исключений в диагностике V501 (всего их там сейчас 35).
Андрей выше утверждал, что компиляторы «генерируют шум», а ваш анализатор этого не делает, но как минимум в трех вышеперечисленных случаях он дублирует предупреждения компилятора.
Я ни в коем случае не подвергаю сомнению полезность PVS в плане обнаружения ошибок. Речь о том, что логично сперва включить расширенные предупреждения компилятора и избавиться хотя бы от большинства из них, а затем уже напускать на код специальные анализаторы. Разбираемый здесь код Tizen, судя по всему, компилируется в стандартном режиме с умеренными предупреждениями, отчего PVS и наловил в нем столько ошибок. То есть, разработчики не слишком критично относятся к качеству кода.
Компиляторы… Так мы использование неинициализированной переменной и в компиляторе можем найти. Вот, например, мы нашли такую ошибку в Clang:
static Error mapNameAndUniqueName(....) {
....
size_t BytesLeft = IO.maxFieldLength();
if (HasUniqueName) {
.....
if (BytesNeeded > BytesLeft) {
size_t BytesToDrop = (BytesNeeded - BytesLeft);
size_t DropN = std::min(N.size(), BytesToDrop / 2);
size_t DropU = std::min(U.size(), BytesToDrop - DropN);
....
}
} else {
size_t BytesNeeded = Name.size() + 1;
StringRef N = Name;
if (BytesNeeded > BytesLeft) {
size_t BytesToDrop = std::min(N.size(), BytesToDrop); // <=
N = N.drop_back(BytesToDrop);
}
error(IO.mapStringZ(N));
}
....
}
PVS-Studio: V573 Uninitialized variable 'BytesToDrop' was used. The variable was used to initialize itself. typerecordmapping.cpp 73
Предупреждение «Expression is Always True/False»… Компиляторы… Пфф… :-) Вот, пожалуйста, вновь Clang:
static bool containsNoDependence(CharMatrix &DepMatrix,
unsigned Row,
unsigned Column) {
for (unsigned i = 0; i < Column; ++i) {
if (DepMatrix[Row][i] != '=' || DepMatrix[Row][i] != 'S' ||
DepMatrix[Row][i] != 'I')
return false;
}
return true;
}
PVS-Studio. V547 Expression is always true. Probably the '&&' operator should be used here. LoopInterchange.cpp 208
Но я говорил не об этом, а о том, что маловероятно получить от приличного компилятора в режиме, аналогичном /Wall, столь большое количество шума, что «аж руки опускаются», и при этом получить от PVS в основном дельные предупреждения. Если код достаточно хорош, то и компилятор, и PVS выдадут разумное количество предупреждений. Если же количество предупреждений компилятора огромно, то и код, скорее всего, написан халтурно, на «лишь бы работало». PVS в этом случае точно так же завалит разработчика предупреждениями, и ему останется либо махнуть рукой, или таки заняться массовой чисткой, а не отдельными косметическими правками.
Например, если в проекте используются только умные указатели, то виртуальные деструкторы уже как бы и не нужны — у умных указателей свои механизмы.
Предложения без явных цен вполне нормальны для чего-то необычного, эксклюзивного, изготавливаемого по заказу, у Вас же предлагается готовый продукт.
Если подобную позицию займут другие компании, предлагающие различные готовые продукты — программы, книги, фильмы, музыкальные записи, автомобили и т.п., у Вас, при желании приобрести такой продукт, не возникнет удивления или недовольства?
А если таким образом будут предлагаться продукты в супермаркете? Не совсем массовые, но готовые, не требующие какой-либо доработки под приобретателя?
В текущем состоянии продукта дискомфорт вызывает не подразумеваемо высокая цена, а именно ее неопределенность.
github.com/mwhagedorn/SoundTouch/blob/master/SoundTouch/AAFilter.cpp#L52
Ну и чтоб добить танцем, константа AI_MATH_PI_F из этой либы:
assimp.sourceforge.net/lib_html/defs_8h.html
#define ONE_OVER_2PI 0.159154942f — не ругаемся, т.к. анализтор ничего не знает про эту магическую константу. Вот константы, с которыми знаком анализтор: M_E, M_LOG2E, M_LOG10E, M_LN2, M_LN10, M_PI, M_PI_2, M_PI_4, M_1_PI, M_2_PI, M_2_SQRTPI, M_SQRT2, M_SQRT1_2.
3.141592655357989
3.14159265357989
А во второй ошибка в вычислении, см. комментарий.
Я к тому, что если ловить такие ошибки, то стоит проверять их не только по имени, но и по мат.разнице с правильной константой.
То есть в AI_MATH_PI_F последняя цифра должна быть 6 а не 8:
3.1415926538f
3.1415926536f
float ac = N * (float)acos(saturate(VDIR.y)) / 3.141592657f;
// правильно:
float ac = N * (float)acos(saturate(VDIR.y)) / 3.141592654f;
27000 ошибок в операционной системе Tizen