Comments 49
В последнее время, когда я радостно выкрикивал «АГА!!!», собираясь уличить PVS в ложных срабатываниях, после более тщательного изучения находил в коде собственные косяки, баги и опечатки. Без PVS я бы их тоже нашёл, когда-нибудь, после очередного непонятного падения или некорректной работы программы.
int *p = NULL;
if (p)
{
*p = 1;
}
А как быть, если программа многопоточная и if находится, например, в цикле, в ожидании подарка
Да и в любом случае нельзя так писать (переменная даже не volatile).
И да, строки присвоения и проверки ведь не обязаны стоять рядом,Если строки присваивания и проверки не стоят рядом, то это совсем другой код. И рассуждать вот так абстрактно бессмысленно.
Если хотите, давайте рассматривать и обсуждать конкретные примеры. Напишите код, который Вас интересует, и мы обсудим будут ли там полезные/ложные срабатывания и почему.
Анализатор PVS-Studio это сложные алгоритмы, пытающиеся вычислять возможные значения переменных, пути следования и так далее. Поэтому важна любая мелочь в коде. А вот так, «если бы да кабы» — неконструктивно.
int *p = NULL;
global_var_prt = &p;
do{ //ждём реакции другого потока
if (p)
{
*p = 1;
break;
}
}while(1);
вариант
int *p = NULL;
global_var_prt = &p;
while(!p) ;
*p=1;
Или даже так (такое вряд-ли увидишь, но совсем я бы не поручился)
int *p = NULL;
global_var_prt = &p;
retp:
if (p)
{
*p = 1;
}
else goto retp;
И да, забыл уточнить, что вопрос тут не к разыменованию, а к
Однако, срабатывает исключение, что разыменование находится в коде, который никогда не выполняется
По факту это вариант спинлока, и непонятно, чем так этот пример всем не понравился.
int *p = NULL;
глобальная, то и передавать дополнительно указатель на неё никуда не нужно. просто
*p = NULL;
while(!p) ;
*p=1;
Вы правда не видите разницы между int *p = NULL и *p = NULL? :-)
Это spin-loop и на него в любом случае надо выдавать предупреждение: без volatile его компилятор вправе оптимизировать в while(true);. А даже с volatile так делать некрасиво, потому что неэффективно. Надо вставлять в цикл архитектурно-зависимые хинты, например, pause на x86. В Java 9 для этого сделали специальный интринсик Thread.onSpinWait(), который на разных архитектурах транслируется в подходящие инструкции процессора.
А вот здесь «while(!p);» должно быть предупреждение про потенциальный вечный цикл, так как 'p' это не volatile и компилятор вправе выбросить проверку.
Когда я размышлял над приоритетом предупреждений статического анализатора, я вводил ещё и третью ось помимо severity/certainity: простота исправления. Скажем, есть старый неэффективный API-метод и новый несколько более эффективный. У вас старый код, вы используете старый метод повсеместно. Анализатор с хорошей вероятностью находит его вызовы (certainity высокая), но severity низкая: программа работает корректно, просто может чуть медленнее, чем могла бы (на фоне тормозов в других местах это может быть совсем незаметно). Стоит ли выдавать тысячу варнингов? Но третья ось подсказывает, что исправить ситуацию легко, просто пройдясь поиском с заменой по коду (лёгкость исправления высокая), поэтому вполне можно и предупредить.
Конечно, любой статический анализатор существенно украшает возможность автоматического исправления хотя бы некоторых проблем. У вас нету планов квик-фиксы прикрутить? ;-)
Конечно, любой статический анализатор существенно украшает возможность автоматического исправления хотя бы некоторых проблем. У вас нету планов квик-фиксы прикрутить?Нет. Часто даже человек не сразу может понять, как нужно править. Что уж тут от программы хотеть. Будет много неверных рекомендаций. А ведь найдутся программисты, которые применят рекомендации без изучения, а потом будет ругаться, что анализатор сломал код и какие мы плохие.
Ничего. Когда-то вы и на линукс не планировали выходить. Дозреете :-)
V571 Recurring check. The 'if (pos == npos)' condition was already verified in line 1530. CryString.h 1539
Предложите, как можно исправить код?
//! Find last single character.
// \return -1 if not found, distance from beginning otherwise.
template<class T>
inline typename CryStringT<T>::size_type CryStringT<T >::rfind(value_type ch, size_type pos) const
{
const_str str;
if (pos == npos)
{
// find last single character
str = _strrchr(m_str, ch);
// return -1 if not found, distance from beginning otherwise
return (str == NULL) ? (size_type) - 1 : (size_type)(str - m_str);
}
else
{
if (pos == npos)
{
pos = length(); // Код никогда не выполняется
}
if (pos > length())
{
return npos;
}
value_type tmp = m_str[pos + 1];
m_str[pos + 1] = 0;
str = _strrchr(m_str, ch);
m_str[pos + 1] = tmp;
}
return (str == NULL) ? (size_type) - 1 : (size_type)(str - m_str);
}
Зависит от концепции продукта. Наш продукт предлагает удалить мёртвый код. Иногда это разумно, но наши пользователи понимают, что такой фикс принимать вслепую смысла нет. Некоторые репорты предлагают несколько фиксов на выбор. Если ваша концепция предлагать фиксы только там, где уверенность в их правильности высокая, то здесь фикс не нужен. А вот для V597, например, легко сделать адекватный фикс — автоматом заменять на правильный метод.
— замена str.find(«c»); на str.find('c');
— замена strstr(«c») на strchr('c');
— удаление объявленых, но не используемых переменных
— замена const Type var на const Type& var в параметрах функций
— что-то ещё было, но сейчас не могу вспомнить
Раз код не выполняется, то ошибки никак не проявят себя. Поэтому многие диагностики не применяются к невыполняемому коду.
Не знаю, конечно, как устроена PVS-Studio внутри, но я писал аналогичные продукты и могу предположить, что это не специально сделанное исключение, а особенность работы data-flow-анализа. Про содержимое тела if выведены два противоречащих друг другу факта: p == NULL и p != NULL. Не существует состояния памяти машины, которое бы привело к исполнению этого кода, поэтому внутри вести любой анализ бессмысленно: анализ всегда применяется в контексте некоторого состояния памяти. Скажем, if(a == 5 && a == 10 && a > 7) — надо ли ругаться на a > 7? Понятно, что если a равно 10, то это условие всегда истинно. А вот если a равно 5, то это условие всегда ложно. Нет ни одного состояния памяти, которое бы привело к выполнению этого кода, потому что a не может быть одновременно равно 5 и 10. Поэтому ругаться надо только на a == 10, что это условие всегда ложно, а до a > 7 анализ дойти не может. Пусть программист сперва с a == 10 разберётся, тогда будем дальше смотреть. Даже если б там было a > 2 (которое всегда истинно вне зависимости от того, a равно 5 или 10), про него ничего сказать нельзя.
Массовое подавление предупреждений, возникающих из-за макроса. Это также осуществляется с помощью специальных комментариев.
А можно поподробнее эту тему развить. У меня проблема с Q_ASSERT'ом. Этот макрос определён как
#if !defined(Q_ASSERT)
# if defined(QT_NO_DEBUG) && !defined(QT_FORCE_ASSERTS)
# define Q_ASSERT(cond) do { } while ((false) && (cond))
# else
# define Q_ASSERT(cond) ((!(cond)) ? qt_assert(#cond,__FILE__,__LINE__) : qt_noop())
# endif
#endif
Поэтому если в коде есть строчки типа
void foo()
{
Q_ASSERT(false);
}
то выдаётся предупреждение на функцию foo вида:
full_filename.cpp (400): error V501: There are identical sub-expressions to the left and to the right of the '&&' operator: (false) && (false)
Как оказалось, в коде таких мест оказалось неприлично много. Хотелось бы как-то едино их погасить, без добавления по всему репозиторию комментариев. Т.е. чтобы можно было написать что-то типа:
//-V501:Q_ASEERT
внутри парочки конфиг-файлов, которые во все проекты включаются, чтобы внутри макроса Q_ASSERT такого ворнинга не возникало.
Ну и на самом деле я удивлён, как вам программисты-qt'шники ещё не жаловались на такое.
Они не жаловались, потому что в документации явно написано, как подавлять предупреждения: // -V:Q_ASSERT:501. Синтаксис, правда, странноват: с ошибкой, описываемой как “error V501” (причём не только в списке ошибок, а везде) ожидаешь, что V является неотъемлемой частью идентификатора ошибки.
А всё же, почему -V отделили от номера?
Нелогично. В базе ошибок у вас они все показываются как Vxxx, не как xxx. Можно было оставить V с номером ошибки, а после дефисоминуса поставить что‐то ещё. Так и искать легче, не будут искаться левые числовые литералы.
Когда как. К примеру, посмотреть «не часто ли мы игнорируем 501, может её вообще отрубить?». Или показать новичку на примерах в каких случаях 501 можно игнорировать. Нет, я и так найду, но полностью правильная регулярка больно уж сложная выйдет, а минимальная что‐то вроде V(?:.+\b)?501 вместо просто V501 (указал регулярки из расчёта «также найти упоминания в комментариях»).
Если вдруг надо подавлять много предупреждений в разных макросах или нельзя менять заголовочные файлы, то можно собрать всё это в pvsconfig файлы. См. в документации о подавлении ложных срабатываний раздел «Массовое подавление ложных предупреждений с помощью файлов конфигурации диагностик pvsconfig». Соответственно, раз всё собрано в одном месте, опять-таки будет легко найти всё что, нужно.
Сейчас анализатор умеет присматривать за printf-like функциями. Но предположим, у меня есть функция
void WriteLog(const char* format, ...);
и как оказалось, у меня с ней тоже есть проблемы. То к %s пихается std::string, то char* же пихается в %S. Хотелось бы возможность комментарием включать проверку использования этой функции.
Т.е. либо как так:
//vXXX: addFunc: foo
или используя аннотации:
С_«void WriteLog(...»
ADD(F_PRINTF)
А __attribute__((format(…))) поддерживается, чтобы два раза не писать? И что произойдёт для собственных форматных символов (что‐то вроде %r, которое выведет myfunc(mystructpointer))?
Кстати, в подразделе «Распечатка адреса» ошибка:
Очень часто значение указателя пытаются распечатать, используя следующий код:
int *ptr = new int[100]; printf("0x%0.8X\n", ptr);
Этот код ошибочен, поскольку будет работать только в тех системах, где размер указателя совпадает с размером типа 'int'. А, например, в Win64 этот код уже распечатает только младшую часть указателя 'ptr'. Корректный вариант кода:
int *ptr = new int[100]; printf("0x%p\n", ptr);
Во‐первых, 0x при использовании %p выдаётся уже самой glibc и в результате будет 0x0xHHHHHHH (правда, по стандарту другие libc могут городить что угодно: во что указатель превращается является «implementation‐defined»). Во‐вторых, как минимум у меня ведущие нули не печатаются. Как это исправить правильно зависит полностью от того, зачем авторам понадобился .8, но как минимум 0x лучше убрать, а ещё лучше использовать PRIxPTR (printf("0x%08" PRIxPTR, (uintptr_t)ptr)). Правда сильно подозреваю, что на Windows обязательно вылезут какие‐то проблемы вроде отсутствия PRIxPTR.
Как и почему статические анализаторы борются с ложными срабатываниями