Название статьи намекает разработчикам Visual Studio, что они могут получать пользу от использования статического анализатора кода PVS-Studio. В статье приводятся результаты анализа библиотек, входящих в состав недавно выпущенной версии Visual C++ 2017, и даются рекомендации по улучшению и устранению ошибок. Приглашаю читателей узнать, как разработчики Visual C++ Libraries отстреливают ноги: будет интересно и познавательно.Немного предыстории
Это уже не первый мой эксперимент по проверке библиотек, входящих в состав Visual C++. Про предыдущие проверки можно почитать здесь:
- Обнаружены ошибки в библиотеках Visual C++ 2012
- Тяп-ляп, проверил библиотеки Visual C++ 2013 (update 3)
После этих публикаций был долгий перерыв, и я не написал статью касательно VS2015. Было много интересных проектов, которые ждали проверки. Хотя, если быть честным, я просто забыл написать статью по этой тематике. К счастью, твит одного из разработчиков Visual C++ (@MalwareMinigun) напомнил мне про VS2017:
I'm surprised we don't have people yelling at us all the time for stuff you folks find in standard library headers.
Действительно, я не рассказал миру о багах в библиотеках Visual Studio 2017! Что ж, вызов принят!
Как видите, с момента твита (31 марта) прошёл месяц. Признаю, что затянул с ответом, но сейчас исправлюсь.
Что и как проверялось
Для проверки я использовал самую последнюю доступную мне версию анализатора PVS-Studio (6.15).
Проверял я C++ библиотеки, входящие в состав недавно выпущенной версии Visual Studio 2017. Проверялась версия библиотек, которые находились у меня на компьютере 12.04.2017. Впрочем, версия не так важна, так как этот текст не багрепорт, а статья, популяризующая методологию статического анализа вообще, и анализатора PVS-Studio в частности.
Признаюсь, я не утруждал себя полноценной проверкой библиотек, так как это сложно для меня.
Во-первых, мне пришлось откопировать все библиотеки в другую папку, иначе я не смог бы получить для них предупреждения. Анализатор не выдаёт сообщения на системные библиотеки. Копируя файлы в другую папку, я обманываю PVS-Studio, и получаю нужные мне предупреждения.
Кстати, вот и ответ на твит, приведённый выше. Именно по этой причине пользователи Visual C++ не пишут о предупреждениях PVS-Studio, касающихся библиотек. Нет смысла их выдавать, так как они будут только отвлекать людей. Заодно, это немного ускоряет анализ, так как не надо полноценно парсить и анализировать тела inline-функций.
Во-вторых, я не пытался по-честному собирать проекты. Я просто создал новый solution и добавил туда файлы из библиотек. В результате, часть файлов мне не удалось проверить. Но, с точки зрения написания статьи, — это не существенно. Материала для полноценного текста и так получилось вполне достаточно. Более тщательную и правильную проверку стоит выполнить уже самим разработчикам Visual C++, и я готов помогать им в этом.
Ложные срабатывания
В этот раз конкретных чисел и процентов я, к сожалению, привести не могу.
Могу сказать, что:
- Предупреждений общего назначения (GA) с достоверностью High я получил 433 шт.
- Предупреждений общего назначения (GA) с достоверностью Medium я получил 743 шт.
Однако, эти числа никак нельзя интерпретировать, и на их основе нельзя делать никаких выводов!
Напомню, что я проверил только часть файлов, да ещё и странным способом. Плюс с библиотеками есть необычный момент. Анализатор выдаёт много абсолютно правильных сообщений, которые при этом являются абсолютно ложными. Сейчас я объясню причину этого парадокса.
Вредно и опасно самостоятельно объявлять системные типы данных. Пример плохой идеи:
typedef unsigned long DWORD;Анализатор PVS-Studio выдаст предупреждение: V677 Custom declaration of a standard 'DWORD' type. The system header file should be used: #include <WinDef.h>.
Анализатор совершенно прав. Следует не объявлять тип «вручную», а подключить соответствующий заголовочный файл.
Как вы понимаете, к библиотекам Visual C++ такая диагностика не применима. Ведь там-то, как раз, и объявляются такие типы. И таких сообщений анализатор выдал более 250.
Или вот ещё один интересный пример. Анализатор PVS-Studio совершенно прав, когда критикует код, в котором указатель this проверяется на равенство NULL. Согласно современному стандарту языка C++, указатель this не может быть NULL.
У Visual C++ с этим большие проблемы. Видимо, в этом плане он никогда не будет соответствовать стандарту, ну или это произойдет очень нескоро. Дело в том, что библиотеки (например, MFC) архитектурно построены так, что this равный NULL — это типовая ситуация.
В коде библиотек присутствует большое количество функций, проверяющих указатель this. Вот пара таких функций:
_AFXWIN_INLINE CDC::operator HDC() const
{ return this == NULL ? NULL : m_hDC; }
_AFXWIN_INLINE HDC CDC::GetSafeHdc() const
{ return this == NULL ? NULL : m_hDC; }Анализатор PVS-Studio, естественно, выдаст для них предупреждения:
- V704 'this == 0' expression should be avoided — this expression is always false on newer compilers, because 'this' pointer can never be NULL. afxwin1.inl 314
- V704 'this == 0' expression should be avoided — this expression is always false on newer compilers, because 'this' pointer can never be NULL. afxwin1.inl 316
Таких сообщений более 40, при этом толку, конечно, от них нет. В ближайшие годы их можно считать ложными срабатываниями.
Как мы видим на примере сообщений V677 и V704, не все диагностики применимы к библиотекам Visual C++. Проблемы конечно никакой нет: эти и другие предупреждения можно просто выключить и, тем самым, сразу уменьшить количество сообщений на 300 штук.
Всё это я пишу, чтобы в очередной раз продемонстрировать, что очень часто бессмысленно обсуждать процент ложных срабатываний, если не провести предварительную настройку анализатора.
В общем, в этот раз без процентов, прошу прощения. Моё субъективное мнение — ложных срабатываний мало.
Что интересного нашел анализатор
Я решил пойти от безобидного к страшному. В начале я рассмотрю рекомендации по мелким улучшениям. Потом перейду к нестрашным ошибкам, а закончу повествование тем, что на мой взгляд, можно назвать «жутью». В общем, буду постепенно повышать градус. Итак, вперёд, спасать мир программ от багов!
Микрооптимизации
Анализатор предлагает выполнить ряд микрооптимизаций. То есть, всё что будет рассмотрено в этой главе не является ошибками, но код можно немножко улучшить.
Начнём с предупреждения V808, которое говорит о том, что некий объект создаётся, но не используется. Рассмотрим эту ситуации на примере двух функций.
void CMFCToolBarComboBoxButton::AdjustRect()
{
....
if (m_pWndEdit != NULL)
{
CRect rectEdit = m_rect;
const int iBorderOffset = 3;
m_pWndEdit->SetWindowPos(
NULL, m_rect.left + nHorzMargin + iBorderOffset,
m_rect.top + iBorderOffset,
m_rect.Width() - 2 * nHorzMargin - m_rectButton.Width() -
iBorderOffset - 3,
m_rectCombo.Height() - 2 * iBorderOffset,
SWP_NOZORDER | SWP_NOACTIVATE);
}
....
}Предупреждение PVS-Studio: V808 'rectEdit' object of 'CRect' type was created but was not utilized. afxtoolbarcomboboxbutton.cpp 607
Объект rectEdit после создания и инициализации нигде не используется. Он просто лишний и его можно смело удалить. Код станет чуть короче.
Ещё один случай:
BOOL CALLBACK AFX_EXPORT
CMFCToolBarFontComboBox::EnumFamPrinterCallBackEx(....)
{
....
CString strName = pelf->elfLogFont.lfFaceName;
pCombo->AddFont((ENUMLOGFONT*)pelf, FontType,
CString(pelf->elfScript));
return 1;
}V808 'strName' object of 'CStringT' type was created but was not utilized. afxtoolbarfontcombobox.cpp 138
Объект типа CString создаётся и инициализируется, но нигде не используется. Не знаю, догадается ли компилятор выбросить лишний код для создания и копирования строки. Возможно, и не догадается, так как CStirng — это сложный класс. Впрочем, это не важно, в любом случае объект strName следует удалить и, тем самым, сделать код проще.
И таких вот лишних объектов очень много. Помимо уже рассмотренных, анализатор выдал ещё 50 сообщений. Чтобы не загромождать текст статьи, я выписал эти сообщения в отдельный файл: vs2017_V808.txt.
Теперь пришло время лишних проверок.
TaskStack::~TaskStack()
{
if (m_pStack)
delete [] m_pStack;
}Предупреждение PVS-Studio: V809 Verifying that a pointer value is not NULL is not required. The 'if (m_pStack)' check can be removed. taskcollection.cpp 29
Оператору delete совершенно безопасно отдать на вход nullptr. Таким образом, проверка является лишней и код можно упростить:
TaskStack::~TaskStack()
{
delete [] m_pStack;
}Таких лишних проверок тоже очень много. Я выписал 68 сообщений в файл vs2017_V809.txt.
Следующая мелочь, которую можно изменить — это заменить постфиксный инкремент итераторов на префиксный. Пример:
size_type count(const key_type& _Keyval) const
{
size_type _Count = 0;
const_iterator _It = _Find(_Keyval);
for (;_It != end() && !this->_M_comparator(....); _It++)
{
_Count++;
}
return _Count;
}Предупреждение PVS-Studio: V803 Decreased performance. In case '_It' is iterator it's more effective to use prefix form of increment. Replace iterator++ with ++iterator. internal_concurrent_hash.h 509
Станет чуть лучше, если написать:
for (;_It != end() && !this->_M_comparator(....); ++_It)Вопрос, есть ли польза в таком рефакторинге, я рассматривал в статье: "Есть ли практический смысл использовать для итераторов префиксный оператор инкремента ++it, вместо постфиксного it++". Ответ: есть, хотя и не большая.
Если авторы библиотек решат, что исправления стоит сделать, то вот ещё 26 аналогичных предупреждений: vs2017_V803.txt.
Следующая микрооптимизация. Лучше очищать строку с помощью вызова функции str.Empty(), чем присваивать ей значение _T(""). Класс ведь не знает заранее, сколько памяти надо выделить под строку. Поэтому он начинает зря считать длину строки. В общем, лишние действия.
CString m_strRegSection;
CFullScreenImpl::CFullScreenImpl(CFrameImpl* pFrameImpl)
{
m_pImpl = pFrameImpl;
m_pwndFullScreenBar = NULL;
m_bFullScreen = FALSE;
m_bShowMenu = TRUE;
m_bTabsArea = TRUE;
m_uiFullScreenID = (UINT)-1;
m_strRegSection = _T("");
}Предупреждение PVS-Studio: V815 Decreased performance. Consider replacing the expression 'm_strRegSection = L""' with 'm_strRegSection.Empty()'. afxfullscreenimpl.cpp 52
Здесь лучше заменить строчку
m_strRegSection = _T("");на
m_strRegSection.Empty();Мелочь, но перфекционист будет рад такому улучшению кода.
Примечание. На самом деле это строчку вообще можно убрать, так как этот код находится в конструкторе и строка и так пустая.
Вот ещё 27 предупреждений на эту тему: vs2017_V815.txt.
И, напоследок, вот такой код:
HRESULT GetPropertyInfo(....)
{
....
for(ul=0; ul<m_cPropSetDex; ul++)
{
....
for(ULONG ulProp=0; ....)
{
....
pDescBuffer += (wcslen(L"UNKNOWN") + 1);
....
}Предупреждение PVS-Studio: V814 Decreased performance. The 'wcslen' function was called multiple times inside the body of a loop. atldb.h 2374
Обратите внимание, что функция wcslen будет вызываться многократно, так как находится внутри вложенных циклов. Было бы логичнее посчитать заранее и запомнить длину строки L«UNKNOWN».
Ещё одно сообщение: V814 Decreased performance. The 'wcslen' function was called multiple times inside the body of a loop. atldb.h 2438
Всё, с рекомендациями закончили. Давайте перейдём к чему-то более интересному.
Ошибки мелкого и среднего калибра
В заголовочных файлах неправильно подавляются предупреждения компилятора. Рассмотрим неправильный способ временного отключения предупреждений:
#ifdef _MSC_VER
#pragma warning(disable:4200)
#endif
typedef struct adpcmwaveformat_tag {
WAVEFORMATEX wfx;
WORD wSamplesPerBlock;
WORD wNumCoef;
#if defined( _MSC_VER )
ADPCMCOEFSET aCoef[];
#else
ADPCMCOEFSET aCoef[1];
#endif
} ADPCMWAVEFORMAT;
typedef ADPCMWAVEFORMAT *PADPCMWAVEFORMAT;
typedef ADPCMWAVEFORMAT NEAR *NPADPCMWAVEFORMAT;
typedef ADPCMWAVEFORMAT FAR *LPADPCMWAVEFORMAT;
#ifdef _MSC_VER
#pragma warning(default:4200)
#endifПредупреждение PVS-Studio: V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 2610, 2628. mmreg.h 2628
Я понимаю, что увидеть, в чем собственно ошибка — не просто, поэтому я выделю суть.
#pragma warning(disable:4200)
....
#pragma warning(default:4200)Сначала предупреждение компилятора отключается. Далее состояние предупреждения 4200 устанавливается в значение по умолчанию. Так делать нельзя. Предположим, пользователь вообще отключил диагностику 4200 для одного своего файла. К своему несчастью, в файле он написал:
#include <mmreg.h>В результате, предупреждение опять включится и будет выдаваться для кода пользователя.
Правильным решением будет сохранить состояние, а потом вернуть предыдущее:
#pragma warning(push)
#pragma warning(disable:4200)
....
#pragma warning(pop)Вот где ещё в заголовочных файлах неправильно используется pragma warning:
- V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 586, 601. workstealingqueue.h 601
- V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 1669, 1697. usbioctl.h 1697
- V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 1631, 1646. usbioctl.h 1646
- V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 1490, 1518. usbioctl.h 1518
- V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 986, 1002. usbioctl.h 1002
- V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 960, 978. usbioctl.h 978
- V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 913, 925. usbioctl.h 925
- V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 861, 876. usbioctl.h 876
- V665 Possibly, the usage of '#pragma warning(default: X)' is incorrect in this context. The '#pragma warning(push/pop)' should be used instead. Check lines: 860, 875. usbioctl.h 875
Есть такие ошибки и в *.cpp файлах, но я их не стал выписывать, так как они не представляют никакой опасности для кода пользователей Visual C++. Хотя, конечно, их тоже будет полезно поправить.
Теперь поговорим об операторе new.
inline HRESULT CreatePhraseFromWordArray(....)
{
....
SPPHRASEELEMENT *pPhraseElement = new SPPHRASEELEMENT[cWords];
if(pPhraseElement == NULL)
{
::CoTaskMemFree(pStringPtrArray);
return E_OUTOFMEMORY;
}
memset(pPhraseElement, 0, sizeof(SPPHRASEELEMENT) * cWords);
....
}Предупреждение PVS-Studio: V668 There is no sense in testing the 'pPhraseElement' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. sphelper.h 2973
Формально, этот код неправильный. В случае ошибки выделения памяти, оператор new должен сгенерировать исключение. В результате, мы не попадём внутрь тела оператора if и не будет вызвана функция CoTaskMemFree. И, вообще, программа начнет работать не так, как ожидал программист.
Я не уверен, что это настоящая ошибка. Есть вероятность, что проект слинкован с nothrownew.obj. В этом случае, оператор new не будет генерировать исключение. Такой возможностью, например, пользуются разработчики драйверов. Подробнее: new and delete operators. Так что, если это ложные срабатывания, то можно просто выключить предупреждение V668.
Но возможен другой сценарий. Этот код остался с древних времён, когда оператор new возвращал в случае ошибки значение NULL. Тогда всё плохо, так как анализатор выдал 112 таких предупреждений: vs2017_V668.txt.
Продолжим. Анализатор выдаёт много предупреждений V730, которое говорит, что в конструкторе инициализированы не все члены. Поясню суть предупреждения на двух примерах.
В начале рассмотрим класс CMFCScanliner. В нем объявлены следующие члены:
class CMFCScanliner
{
....
private:
LPBYTE m_line;
LPBYTE m_line_begin;
LPBYTE m_line_end;
size_t m_pitch;
DWORD m_start_row;
DWORD m_start_col;
DWORD m_rows;
DWORD m_cols;
long m_offset;
BYTE m_channels;
size_t m_height;
};Теперь посмотрим на конструктор:
CMFCScanliner()
{
empty();
}Собственно, смотреть в конструкторе нечего. Нам следует перейти к изучению функции empty:
void empty()
{
m_line = NULL;
m_pitch = 0;
m_start_row = 0;
m_start_col = 0;
m_rows = 0;
m_cols = 0;
m_offset = 0;
m_height = 0;
m_line_begin = NULL;
m_line_end = NULL;
}Предупреждение PVS-Studio: V730 It is possible that not all members of a class are initialized inside the constructor. Consider inspecting: m_channels. afxtoolbarimages.cpp 510
Инициализированы все члены, кроме m_channels. Согласитесь, это странно, так как этот член не выделяется ничем среди остальных. Это очень похоже на ошибку, хотя до конца я не могу быть уверенным, так как не знаком с принципами работы рассматриваемого класса.
Теперь рассмотрим структуру AFX_EVENT.
struct AFX_EVENT
{
enum
{
event, propRequest, propChanged, propDSCNotify
};
AFX_EVENT(int eventKind);
AFX_EVENT(int eventKind, DISPID dispid, ....);
int m_eventKind;
DISPID m_dispid;
DISPPARAMS* m_pDispParams;
EXCEPINFO* m_pExcepInfo;
UINT* m_puArgError;
BOOL m_bPropChanged;
HRESULT m_hResult;
DSCSTATE m_nDSCState;
DSCREASON m_nDSCReason;
};
AFX_INLINE AFX_EVENT::AFX_EVENT(int eventKind)
{
m_eventKind = eventKind;
m_dispid = DISPID_UNKNOWN;
m_pDispParams = NULL;
m_pExcepInfo = NULL;
m_puArgError = NULL;
m_hResult = NOERROR;
m_nDSCState = dscNoState;
m_nDSCReason = dscNoReason;
}Предупреждение PVS-Studio: V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_bPropChanged. afxpriv2.h 104
В этот раз неинициализированной осталась переменная m_bPropChanged.
В обоих случаях я не берусь судить нужно ли инициализировать эти переменные. Поэтому я предлагаю разработчикам изучить эти и другие подозрительные случаи, на которые обращает внимание анализатор PVS-Studio. Я выписал в файл vs2017_V730.txt ещё 183 предупреждения анализатора. Наверняка среди этих сообщений найдется несколько, которые указывают на настоящие серьезные ошибки. Если бы я был уверен, что члены точно надо инициализировать, я бы отнес все эти предупреждения к следующей главе этой статьи. Неинициализированные переменные крайне коварны, так как ошибки могут проявляться редко и нерегулярно.
Следующие предупреждения анализатора касаются бессмысленных проверок. Такие проверки следует или удалить, или заменить на какие-то другие.
HRESULT
SetDpiCompensatedEffectInput(....)
{
....
hr = deviceContext->CreateEffect(CLSID_D2D1DpiCompensation,
&dpiCompensationEffect);
if (SUCCEEDED(hr))
{
if (SUCCEEDED(hr))
{
....
}Предупреждение PVS-Studio: V571 Recurring check. The 'if (((HRESULT)(hr)) >= 0)' condition was already verified in line 881. d2d1_1helper.h 883
Два раза подряд проверяется значение переменной hr. Мы имеем дело или с лишним кодом, или это какая-то опечатка и нужно изменить второе условие.
void Append(_In_reads_(nLength) PCXSTR pszSrc, _In_ int nLength)
{
// See comment in SetString() about why we do this
UINT_PTR nOffset = pszSrc-GetString();
UINT nOldLength = GetLength();
if (nOldLength < 0)
{
// protects from underflow
nOldLength = 0;
}
....
}Предупреждение PVS-Studio: V547 Expression 'nOldLength < 0' is always false. Unsigned type value is never < 0. atlsimpstr.h 392
Переменная nOldLength имеет беззнаковый тип UINT, а значит не может быть меньше нуля.
Теперь поговорим о функции FreeLibrary.
extern "C"
BOOL WINAPI DllMain(HINSTANCE hInstance, DWORD dwReason, LPVOID)
{
....
::FreeLibrary(pState->m_appLangDLL);
....
}Предупреждение PVS-Studio: V718 The 'FreeLibrary' function should not be called from 'DllMain' function. dllinit.cpp 639
Вот что сказано в MSDN по поводу функции FreeLibrary: It is not safe to call FreeLibrary from DllMain. For more information, see the Remarks section in DllMain.
Благодаря везению, код может работать. Однако, все равно код плох и следует обратить на него внимание.
В заключении этой главы хочу обратить внимание вот на эту шаблонную функцию:
template<class _FwdIt>
string_type transform_primary(_FwdIt _First, _FwdIt _Last) const
{ // apply locale-specific case-insensitive transformation
string_type _Res;
if (_First != _Last)
{ // non-empty string, transform it
vector<_Elem> _Temp(_First, _Last);
_Getctype()->tolower(&*_Temp.begin(),
&*_Temp.begin() + _Temp.size());
_Res = _Getcoll()->transform(&*_Temp.begin(),
&*_Temp.begin() + _Temp.size());
}
return (_Res);
}Предупреждение PVS-Studio: V530 The return value of function 'tolower' is required to be utilized. regex 319
Я впервые вижу этот код и мне сложно сориентироваться. Поэтому, я не знаю прав ли анализатор, указывая на вызов функции tolower. Обычно результат функции tolower обязательно надо использовать, но я не знаю какая именно функция tolower здесь вызывается. Поэтому просто обращаю внимание разработчиков библиотеки на этот код и прошу его проверить.
Hardcore
Здесь начинается, на мой взгляд, самое интересное.
_AFXCMN_INLINE int CToolBarCtrl::GetString(
_In_ int nString,
_Out_writes_to_(cchMaxLen, return + 1) LPTSTR lpstrString,
_In_ size_t cchMaxLen) const
{
ASSERT(::IsWindow(m_hWnd));
return (int) ::SendMessage(m_hWnd, ...., (LPARAM)lpstrString);
lpstrString[cchMaxLen]=_T('\0');
}Предупреждение PVS-Studio: V779 Unreachable code detected. It is possible that an error is present. afxcmn2.inl 111
Явная ошибка: последняя строчка функции никогда не выполняется.
Следующий фрагмент кода содержит крайне подозрительный вызов оператора return внутри тела цикла:
HRESULT GetIndexOfPropertyInSet(
_In_ const GUID* pPropSet,
_In_ DBPROPID dwPropertyId,
_Out_ ULONG* piCurPropId,
_Out_ ULONG* piCurSet)
{
HRESULT hr = GetIndexofPropSet(pPropSet, piCurSet);
if (hr == S_FALSE)
return hr;
UPROPINFO* pUPropInfo = m_pUPropSet[*piCurSet].pUPropInfo;
for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++)
{
if( dwPropertyId == pUPropInfo[ul].dwPropId )
*piCurPropId = ul;
return S_OK;
}
return S_FALSE;
}Предупреждение PVS-Studio: V612 An unconditional 'return' within a loop. atldb.h 4837
Не понятно, зачем было делать цикл, если всё равно в этот цикл не сможет выполнить больше одной итерации. Код можно упросить. Однако, мне кажется, что здесь надо не упрощать код, а исправлять ошибку. У меня есть подозрение, что здесь забыты фигурные скобки и, на самом деле, функция должна выглядеть так:
HRESULT GetIndexOfPropertyInSet(
_In_ const GUID* pPropSet,
_In_ DBPROPID dwPropertyId,
_Out_ ULONG* piCurPropId,
_Out_ ULONG* piCurSet)
{
HRESULT hr = GetIndexofPropSet(pPropSet, piCurSet);
if (hr == S_FALSE)
return hr;
UPROPINFO* pUPropInfo = m_pUPropSet[*piCurSet].pUPropInfo;
for(ULONG ul=0; ul<m_pUPropSet[*piCurSet].cUPropInfo; ul++)
{
if( dwPropertyId == pUPropInfo[ul].dwPropId )
{
*piCurPropId = ul;
return S_OK;
}
}
return S_FALSE;
}Помимо рассмотренного цикла, стоит обратить внимание на пару операторов break, которые всегда прерывают циклы:
- V612 An unconditional 'break' within a loop. viewprev.cpp 476
- V612 An unconditional 'break' within a loop. iomanip 489
Теперь поговорим про Copy-Paste. Нельзя написать большой проект и не наделать ошибок, связанных с копированием и правкой блоков текста.
Кстати, предлагаю в начале самостоятельно попробовать найти ошибку, не читая статью дальше.
void CPaneContainerManager::RemoveAllPanesAndPaneDividers()
{
ASSERT_VALID(this);
POSITION pos = NULL;
for (pos = m_lstControlBars.GetHeadPosition(); pos != NULL;)
{
POSITION posSave = pos;
CBasePane* pWnd = DYNAMIC_DOWNCAST(
CBasePane, m_lstControlBars.GetNext(pos));
ASSERT_VALID(pWnd);
if (pWnd->IsPaneVisible())
{
m_lstControlBars.RemoveAt(posSave);
}
}
for (pos = m_lstSliders.GetHeadPosition(); pos != NULL;)
{
POSITION posSave = pos;
CBasePane* pWnd = DYNAMIC_DOWNCAST(
CBasePane, m_lstControlBars.GetNext(pos));
ASSERT_VALID(pWnd);
if (pWnd->IsPaneVisible())
{
m_lstSliders.RemoveAt(posSave);
}
}
}
Видите, что не так?
Готов поспорить, что многие сдались и решили читать статью дальше. Это хороший пример, почему статические анализаторы важны и нужны: они не ленятся и не устают.
Предупреждение PVS-Studio: V778 Two similar code fragments were found. Perhaps, this is a typo and 'm_lstSliders' variable should be used instead of 'm_lstControlBars'. afxpanecontainermanager.cpp 1645
Впрочем, даже прочитав предупреждение анализатора, не просто найти ошибку в коде. Поэтому я сокращу его, оставив важное для нас:
for (... m_lstControlBars ...)
{
CBasePane* pWnd = ... m_lstControlBars ...
m_lstControlBars.RemoveAt();
}
for (... m_lstSliders ...)
{
CBasePane* pWnd = ... m_lstControlBars ...
m_lstSliders.RemoveAt();
}В первом цикле обрабатывается контейнер m_lstControlBars, а во втором контейнер m_lstSliders.
С вероятностью 99%, второй цикл был написан с использованием технологии Copy-Paste: был взят первый цикл, скопирован, а затем в нем заменили имя m_lstControlBars на m_lstSliders. Но в одном месте заменить имя забыли!
Ошибка здесь: CBasePane* pWnd =… m_lstControlBars…
Это была хорошая ошибка, но следующая не уступает ей по красоте. Рассмотрим, как реализованы операторы инкремента/декремента в классе CMFCScanliner:
class CMFCScanliner
{
....
inline const CMFCScanliner& operator ++ ()
{
m_line += m_offset;
return *this;
}
inline const CMFCScanliner& operator ++ (int)
{
m_line += m_offset;
return *this;
}
inline const CMFCScanliner& operator -- ()
{
m_line -= m_offset;
return *this;
}
inline const CMFCScanliner& operator -- (int)
{
m_line += m_offset;
return *this;
}
....
};Предупреждение PVS-Studio: V524 It is odd that the body of '--' function is fully equivalent to the body of '++' function. afxtoolbarimages.cpp 656
Обратите внимание на реализацию самого последнего оператора. Забыли поменять += на -=. Это классика! Это "эффект последней строки" в действии!
Нашлось три места, где могут возникать утечки. Вот одно из них:
CSpinButtonCtrl* CMFCPropertyGridProperty::CreateSpinControl(
CRect rectSpin)
{
ASSERT_VALID(this);
ASSERT_VALID(m_pWndList);
CSpinButtonCtrl* pWndSpin = new CMFCSpinButtonCtrl;
if (!pWndSpin->Create(WS_CHILD | WS_VISIBLE | UDS_ARROWKEYS |
UDS_SETBUDDYINT | UDS_NOTHOUSANDS,
rectSpin, m_pWndList,
AFX_PROPLIST_ID_INPLACE))
{
return NULL;
}
....
}Предупреждение PVS-Studio: V773 The function was exited without releasing the 'pWndSpin' pointer. A memory leak is possible. afxpropertygridctrl.cpp 1490
Если функция Create будет выполнена с ошибкой, то не будет удалён объект, указатель на который хранится в переменной pWndSpin.
Аналогично:
- V773 The function was exited without releasing the 'pList' pointer. A memory leak is possible. afxribboncombobox.cpp 461
- V773 The function was exited without releasing the 'pButton' pointer. A memory leak is possible. afxvslistbox.cpp 222
Согласно стандарту языка C++, вызов оператора delete для указателя типа void* приводит к неопределённому поведению. И, как уже догадался читатель, это происходит в библиотеках Visual C++:
typedef void *PVOID;
typedef PVOID PSECURITY_DESCRIPTOR;
class CSecurityDescriptor
{
....
PSECURITY_DESCRIPTOR m_pSD;
....
};
inline CSecurityDescriptor::~CSecurityDescriptor()
{
delete m_pSD; // <= Член m_pSD имеет тип void *
free(m_pOwner);
free(m_pGroup);
free(m_pDACL);
free(m_pSACL);
}Предупреждение PVS-Studio: V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1039
Аналогичные проблемы можно найти здесь:
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1048
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1070
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. atlcom.h 1667
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. afxstate.cpp 265
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. dbcore.cpp 1240
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. dbcore.cpp 1250
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. doccore.cpp 1654
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. dockstat.cpp 343
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. filefind.cpp 43
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. filefind.cpp 49
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. sockcore.cpp 541
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. winfrm.cpp 145
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. winfrm.cpp 465
- V772 Calling a 'delete' operator for a void pointer will cause undefined behavior. mapiunicodehelp.h 168
Функция CMFCReBar::CalcFixedLayout принимает параметр bStretch, но не использует его. Вернее, перед первым использованием, в bStretch явно записывается 1. Чтобы продемонстрировать, что я ничего не напутал и не просмотрел, привожу эту функцию целиком.
CSize CMFCReBar::CalcFixedLayout(BOOL bStretch, BOOL bHorz)
{
ASSERT_VALID(this);
ENSURE(::IsWindow(m_hWnd));
// the union of the band rectangles is the total bounding rect
int nCount = (int) DefWindowProc(RB_GETBANDCOUNT, 0, 0);
REBARBANDINFO rbBand;
rbBand.cbSize = m_nReBarBandInfoSize;
int nTemp;
// sync up hidden state of the bands
for (nTemp = nCount; nTemp--; )
{
rbBand.fMask = RBBIM_CHILD|RBBIM_STYLE;
VERIFY(DefWindowProc(RB_GETBANDINFO, nTemp,
(LPARAM)&rbBand));
CPane* pBar = DYNAMIC_DOWNCAST(
CPane, CWnd::FromHandlePermanent(rbBand.hwndChild));
BOOL bWindowVisible;
if (pBar != NULL)
bWindowVisible = pBar->IsVisible();
else
bWindowVisible = (::GetWindowLong(
rbBand.hwndChild, GWL_STYLE) & WS_VISIBLE) != 0;
BOOL bBandVisible = (rbBand.fStyle & RBBS_HIDDEN) == 0;
if (bWindowVisible != bBandVisible)
VERIFY(DefWindowProc(RB_SHOWBAND, nTemp, bWindowVisible));
}
// determine bounding rect of all visible bands
CRect rectBound; rectBound.SetRectEmpty();
for (nTemp = nCount; nTemp--; )
{
rbBand.fMask = RBBIM_STYLE;
VERIFY(DefWindowProc(RB_GETBANDINFO, nTemp,
(LPARAM)&rbBand));
if ((rbBand.fStyle & RBBS_HIDDEN) == 0)
{
CRect rect;
VERIFY(DefWindowProc(RB_GETRECT, nTemp, (LPARAM)&rect));
rectBound |= rect;
}
}
// add borders as part of bounding rect
if (!rectBound.IsRectEmpty())
{
CRect rect; rect.SetRectEmpty();
CalcInsideRect(rect, bHorz);
rectBound.right -= rect.Width();
rectBound.bottom -= rect.Height();
}
bStretch = 1;
return CSize((bHorz && bStretch) ? 32767 : rectBound.Width(),
(!bHorz && bStretch) ? 32767 : rectBound.Height());
}Предупреждение PVS-Studio: V763 Parameter 'bStretch' is always rewritten in function body before being used. afxrebar.cpp 209
Строка «bStretch = 1;» выглядит так, как будто её кто-то написал для отладки, а потом забыл удалить. Возможно, именно так и произошло. Прошу разработчиков проверить и разобраться с этим странным кодом.
Рассмотрим, как объявлена функция AdjustDockingLayout в классе CBasePane и CDockSite.
class CBasePane : public CWnd
{
....
virtual void AdjustDockingLayout(HDWP hdwp = NULL);
....
};
class CDockSite : public CBasePane
{
....
virtual void AdjustDockingLayout();
....
};Предупреждение PVS-Studio: V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'AdjustDockingLayout' in derived class 'CDockSite' and base class 'CBasePane'. afxdocksite.h 94
Мне кажется, что в какой-то момент в базовом классе в объявлении функции добавили аргумент hdwp, а в классе-наследнике забыли. В результате, теперь это две разные функции.
Схожие ситуации:
- V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'CopyState' in derived class 'CPane' and base class 'CBasePane'. afxpane.h 96
- V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'CopyState' in derived class 'CDockablePane' and base class 'CPane'. afxdockablepane.h 184
- V762 It is possible a virtual function was overridden incorrectly. See second argument of function 'SizeToContent' in derived class 'CMFCLinkCtrl' and base class 'CMFCButton'. afxlinkctrl.h 50
- V762 It is possible a virtual function was overridden incorrectly. See first argument of function 'RecalcLayout' in derived class 'CMFCTasksPane' and base class 'CPane'. afxtaskspane.h 287
Раз уж мы заговорили о функциях в классах, давайте поговорим о виртуальных деструкторах, вернее, об их отсутствии. Для начала заглянем в класс CAccessToken:
class CAccessToken
{
....
mutable CRevert *m_pRevert;
};
inline bool
CAccessToken::ImpersonateLoggedOnUser() const throw(...)
{
....
delete m_pRevert;
m_pRevert = _ATL_NEW CRevertToSelf;
....
}Анализатор выдаёт предупреждение: V599 The virtual destructor is not present, although the 'CRevert' class contains virtual functions. atlsecurity.h 5252
Разберёмся, почему он это делает. Нас интересует член m_pRevert, который является указателем на объект типа CRevert. Класс используется полиморфно. К такому заключению можно прийти, посмотрев на эту строку кода:
m_pRevert = _ATL_NEW CRevertToSelf;Класс CRevertToSelf наследуется от класса CRevert. Давайте теперь познакомимся с этими классами:
class CRevert
{
public:
virtual bool Revert() throw() = 0;
};
class CRevertToSelf : public CRevert
{
public:
bool Revert() throw()
{
return 0 != ::RevertToSelf();
}
};Чего не хватает в этом классе CRevert? В нём не хватает виртуального деструктора.
Мы не только добавляем в анализатор PVS-Studio новые диагностики, но и усовершенствуем уже имеющиеся. Например, недавно диагностика V611 научилась искать проблему освобождения памяти, когда процесс выделения и освобождения памяти находится в разных функциях. Сейчас я продемонстрирую, как это работает на практике.
template <class TAccessor>
class CBulkRowset : public CRowset<TAccessor>
{
....
void SetRows(_In_ DBROWCOUNT nRows) throw()
{
if (nRows == 0)
nRows = 10;
if (nRows != m_nRows)
{
delete m_phRow;
m_phRow = NULL;
m_nRows = nRows;
}
}
HRESULT BindFinished() throw()
{
m_nCurrentRows = 0;
m_nCurrentRow = 0;
m_hr = S_OK;
if (m_phRow == NULL)
{
m_phRow = _ATL_NEW HROW[m_nRows];
if (m_phRow == NULL)
return E_OUTOFMEMORY;
}
return S_OK;
}
....
HROW* m_phRow;
....
}Предупреждение PVS-Studio: V611 The memory was allocated using 'new T[]' operator but was released using the 'delete' operator. Consider inspecting this code. It's probably better to use 'delete [] m_phRow;'. atldbcli.h 5689
Память выделяется в функции BindFinished с помощью оператора new []:
m_phRow = _ATL_NEW HROW[m_nRows];Память освобождается в функции SetRows с помощью оператора delete:
delete m_phRow;Результат: неопределённое поведение.
Теперь поговорим об одном очень подозрительном вызове функции memset. Однако, прежде чем смотреть некорректный код, взглянем на код, где всё хорошо.
Правильный код:
void CToolTipCtrl::FillInToolInfo(TOOLINFO& ti, ....) const
{
memset(&ti, 0, sizeof(AFX_OLDTOOLINFO));
ti.cbSize = sizeof(AFX_OLDTOOLINFO);
....
}Перед нами типовая ситуация. Все члены структуры очищаются (заполняются нулями) с помощью вызова функции memset. Затем в структуру записывается её размер. Это стандартная практика для WinAPI. Так многие функции узнают с какой версией (форматом) структуры они работают.
В приведённом выше коде всё логично. Вычисляется размер структуры AFX_OLDTOOLINFO. Далее этот размер структуры используется для вызова функции memset и этот же размер записывается внутрь структуры.
Теперь рассмотрим аномальный код:
BOOL CControlBar::PreTranslateMessage(MSG* pMsg)
{
....
TOOLINFO ti; memset(&ti, 0, sizeof(AFX_OLDTOOLINFO));
ti.cbSize = sizeof(TOOLINFO);
....
}Предупреждение PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer '& ti'. barcore.cpp 384
Структура имеет тип TOOLINFO. Именно размер структуры TOOLINFO и будет записан в неё: ti.cbSize = sizeof(TOOLINFO);.
Однако, обнуляется структура не целиком, а только частично, так как количество обнуляемых байт вычисляется с помощью выражения sizeof(AFX_OLDTOOLINFO).
В итоге, часть полей структуры останется неинициализированными.
Есть ещё один случай, когда memset заполняет не всю структуру.
GUID m_Id;
void zInternalStart()
{
....
// Zero the activity id in case we end up logging the stop.
ZeroMemory(&m_Id, sizeof(&m_Id));
....
}Предупреждение PVS-Studio: V512 A call of the 'memset' function will lead to underflow of the buffer '& m_Id'. traceloggingactivity.h 656
Классическая ошибка, когда вместо размера структуры вычисляется размер указателя. В результате, обнулятся только первые 4 или 8 байт, в зависимости от того, компилируется 32-битное или 64-битное приложение. Структура GUID же занимает 16 байт (128 бит).
Правильный вариант:
ZeroMemory(&m_Id, sizeof(m_Id));Не обошлось и без предупреждений V595. В этом нет ничего удивительного, так как эта диагностика выявляет одну из самых распространённых ошибок в программах на языке C и C++. Впрочем, в C# всё тоже не идеально.
Суть ошибки заключается в том, что разыменование указателя происходит до его проверки.
Рассмотрим фрагмент кода, выявленный с помощью этой диагностики.
HRESULT CBasePane::get_accHelp(VARIANT varChild, BSTR *pszHelp)
{
if ((varChild.vt == VT_I4) && (varChild.lVal == CHILDID_SELF))
{
*pszHelp = SysAllocString(L"ControlPane");
return S_OK;
}
if (((varChild.vt != VT_I4) && (varChild.lVal != CHILDID_SELF))
|| (NULL == pszHelp))
{
return E_INVALIDARG;
}
....
}Предупреждение PVS-Studio: V595 The 'pszHelp' pointer was utilized before it was verified against nullptr. Check lines: 1324, 1328. afxbasepane.cpp 1324
Если функцию вызвать вот так:
VARIANT foo;
foo.vt = VT_I4;
foo.lVal = CHILDID_SELF;
get_accHelp(foo, NULL);То функция должна была бы вернуть статус E_INVALIDARG, но вместо этого произойдёт разыменование нулевого указателя.
Анализатор рассуждает следующим образом. Указатель разыменовывается. Однако, ниже есть проверка на равенство этого указателя NULL. Раз есть такая проверка, то видимо указатель может быть нулевым. Если он будет нулевым, произойдет беда. Ага, надо предупредить!
Как я уже сказал, это весьма распространённая ошибка. Библиотеки Visual C++ не стали исключением. Вот ещё 17 мест, которые ждут рефакторинга: vs2017_V595.txt.
И рассмотрим завершающую ошибку, связанную с путаницей между константами FALSE и S_FALSE.
BOOL CMFCRibbonPanel::OnSetAccData (long lVal)
{
....
if (nIndex < 0 || nIndex >= arElements.GetSize())
{
return FALSE;
}
if (GetParentWnd()->GetSafeHwnd() == NULL)
{
return S_FALSE;
}
ASSERT_VALID(arElements[nIndex]);
return arElements[nIndex]->SetACCData(GetParentWnd(), m_AccData);
}Предупреждение PVS-Studio: V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. afxribbonpanel.cpp 4107.
Функция возвращает тип BOOL. Если не удалось получить HWND у родительского окна, программист хотел вернуть из функции значения FALSE. Но опечатался и написал S_FALSE, что всё радикально меняет.
Вот как объявлена константа S_FALSE:
#define S_FALSE ((HRESULT)1L)Я думаю, вы уже поняли, что происходит, но на всякий случай поясню.
Написать «return S_FALSE;» это тоже самое, что написать «return TRUE;». Epic fail.
Ошибка не одинока, у неё есть друзья:
- V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. afxribbonbar.cpp 5623
- V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. afxribbonbar.cpp 5627
- V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. ctlnownd.cpp 349
- V716 Suspicious type conversion in return statement: returned HRESULT, but function actually returns BOOL. olecli2.cpp 548
Примечание
Как уже говорилось в начале статьи, я проверил не все файлы. Вдобавок, я мог пропустить что-то и среди выданных анализатором предупреждений. Поэтому прошу не рассматривать разработчиков этот документ, как руководство по исправлению некоторых ошибок. Намного лучше, если разработчики сами полноценно проверят библиотеки и внимательно изучат предупреждения анализатора.
Заключение
Я в очередной раз смог продемонстрировать читателю, какую пользу могут приносить инструменты статического анализа кода.
Хочу предостеречь от одной ошибки. Время от времени я слышу, что некоторые программисты запускают статические анализаторы при подготовке к релизу. Если кто-то так делает и утверждает, что это нормально, то он глубоко ошибается, и прошу вас наставить его на путь истинный. Это самый неправильный способ использования статических анализаторов. Это то же самое, что включать перед релизом предупреждения компилятора, а все остальное время работать над проектом, полностью их отключив.
Приглашаю всех установить демонстрационную версию анализатора PVS-Studio и попробовать проверить свои проекты.
Страница продукта PVS-Studio: https://www.viva64.com/ru/pvs-studio/
Поддерживаемые языки и компиляторы:
- Windows. Visual Studio 2017 C, C++, C++/CLI, C++/CX (WinRT), C#
- Windows. Visual Studio 2015 C, C++, C++/CLI, C++/CX (WinRT), C#
- Windows. Visual Studio 2013 C, C++, C++/CLI, C++/CX (WinRT), C#
- Windows. Visual Studio 2012 C, C++, C++/CLI, C++/CX (WinRT), C#
- Windows. Visual Studio 2010 C, C++, C++/CLI, C#
- Windows. MinGW C, C++
- Windows/Linux. Clang C, C++
- Linux. GCC C, C++
Спасибо за внимание и подписывайтесь на твиттер @Code_Analysis.
Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Andrey Karpov. How to Improve Visual C++ 2017 Libraries Using PVS-Studio
Комментарии (74)
zanac
02.05.2017 23:19-1Лучше бы исходники симбиан проверили. Хотя бы библиотеку User(содержит реализацию много чего разного, используется — везде)
Jef239
03.05.2017 02:41-6Я в очередной раз смог продемонстрировать читателю, какую пользу могут приносить инструменты статического анализа кода.
Увы, вы очередной раз доказали, чтопиратскаябесплатная версия PVS-Studio полезна. Не вижу, что найденные вами ошибки оправдывают те килобаксы, которые стоит PVS-Studo.
А надо доказать не только то, что цена оправдывается, а что покупка PVS-Studio выгодней иных методов повышения качества кода.
Ну и последнее. Ваш ест показывает ошибку (мисфичу) в PVS-Studio, которой вы просто не заметили. Не так уж и редко бывают ситуации, когда один и тот же код должен компилироваться компиляторами, использующими разные версии стандарта. При этом основная масса кода использует старый стандарт, а в отдельных местах условной компиляцией вводятся альтернативные куски для старого и нового стандарта.
Судя по вашим примерам — тут ровно такая же ситуация. Но… судя по вашей статье PVS-Studio просто не позволяет и принимать фичи новых стандартов и считать правильным то, что должно было быть по старым стандартам.
Надо ли исправлять эту мисфичу — не знаю, это вопрос маркетинга. Но факт, что вы её не заметили.
Andrey2008
03.05.2017 09:26+4Это неправильный подход. Ценность статического анализа не в разовом запуске, а в его регулярном использовании. При регулярных запусках многие ошибки можно устранять на самых ранних этапах, а не когда о них сообщают пользователи. Например, вот человек в ответ на мой твит пишет:
I reported similar bug in MFC, they have leaked some tooltips or whatnot in function CMFCToolBar::UpdateTooltips. I found it by umdh tool.
И зачем так сложно, когда такую ошибку можно было бы найти самим разработчикам библиотек сразу после написания кода?
Отсюда следует, что фраза «не вижу, что найденные вами ошибки оправдывают те килобаксы, которые стоит PVS-Studio», логически неверная. Проведу аналогию с предупреждениями компилятора. Представим, что есть работающий проект, который разрабатывается с отключенными предупреждениями компилятора. Включаем их и смотрим. Есть ли смысл тратить и дальше время на их изучение, или лучше опять их выключить? Вряд ли предупреждения укажут на серьезную ошибку, проект ведь отлажен и работает. По предложенной Вами логике предупреждения надо отключить, так как они не показали свою ценность, не найдя фатальные ошибки.
Jef239
03.05.2017 10:01-7Вы что хотите? Чтобы взломали защиту и использовали PVS-Studio бесплатно? Или все-таки продавать?
Включение предупреждений компилятора — бесплатно (даже если сам компилятор платный). К расходам относится только время, потраченное на разбор предупреждений.
А PVS-Studio — это куча килобаксов. Вот и докажите экономическим расчетом, что эти килобаксы оправдываются. И не просто оправдываются, а ещё и эффективней иных методов.
Помножьте найденные вами ошибки на 10 или 100. Разве они оправдывают цену PVS-Studio? В обычной ситуации коммерческого кода — не оправдывают.
Вы бы для сравнения тот же код под gcc с -Wall -Wextra откомпилировали. Или под clang. Были бы хоть данные, насколько больше (или меньше) ошибок находит PVS-Studio.
Представим, что есть работающий проект, который разрабатывается с отключенными предупреждениями компилятора. Включаем их и смотрим. Есть ли смысл тратить и дальше время на их изучение, или лучше опять их выключить?
А я именно этот эксперимент сделал и описал. Включаем предупреждения — ничего. CppCheck — ерунда, жаль времени на анализ. Переводим под linux/gcc с -Wall -Wextra — и имеем вагон полезных замечаний. Вывод — даже бесплатный CppCheck не оправдывается. А вы предлагаете потратить килобаксы.
Так все-таки, вы хотите, чтобы PVS-Studio пиратили или покупали?MikailBag
03.05.2017 16:36PVS не продаст свой анализатор тем, кто захочет его спиратить.
Причем что-либо ломать тоже не нужно.
Нужно:
1) Проставить во всех файлах комментарии "this file is checked with pvs-..."
2) Запустить pvs studio
3) удалить комментарии
Причем это придумали в тот момент, когда появилась бесплатная версия лицензии на PVS.
Разработчики вроде говорили, что пираты им в любом случае не заплатили бы.Jef239
03.05.2017 21:02+1Проще скачать демо-версию под linux и использовать её. Только это все равно будет пиратское использование, ибо демо-версия предназначена лишь для оценки перед покупкой. По хорошему, найденные ей ошибки в коде исправлять до покупки анализатора нельзя, ибо тогда это уже будет использование, а не оценка.
Note that this mode is not intended to evaluate this software. Please use a demo version or request a temporary license key to try out the analyzer.
А пиратить программы российских коллег — ну как-то западло. Была бы западная — использовал бы и не парился. А российское — ну вот не привык я так. Слишком уж тесен этот мир.
firegurafiku
03.05.2017 21:24+1Нужно: 1) проставить во всех файлах комментарии "this file is checked with pvs-..."; 2) запустить pvs studio; 3) удалить комментарии.
Этот сценарий использования является пиратством, так как явно запрещён лицензией (секция «Update»). Правильней всего просто не использовать этот анализатор (и, тем более, не делать ему рекламы) если не согласны с лицензией.
MikailBag
03.05.2017 21:26Конечно.
Я просто хочу сказать, что ломать pvs в любом случае не имеет смысла.
Честные его купят.
Пираты его "сломают" крошечным скриптом.
balexa
03.05.2017 12:46+4А между прочим вполне правильный вопрос поднят.
Условно говоря у каждой ошибки есть цена и у каждого средства поиска этих ошибок она есть. Можно нанять кучу тестировщиков. Можно писать юнит-тесты, интеграционные тесты, тесты на тесты. Можно использовать открытые аналоги. Можно компилировать с treat warnings as errors. Можно делать две независимые имплементации. Можно наконец делать это все вместе.
Где на этой шкале находится ваш PVS студио лично мне, да и думаю многим другим непонятно, было бы неплохо если бы вы именно это прояснили — насколько стоимость поиска ошибки с PVS ниже чем поиск этой ошибки автоматически и ручным тестированием.
И да, если найти ошибку будет дороже чем она может принести убытка — то ее как ни странно проще вообще не искать.
Ценность статического анализа не в разовом запуске, а в его регулярном использовании
Это расходится с вашими поисками ошибок в открытом коде. Вместо того чтобы давать лицензии разработчикам опенсорсных проектов, как это делают все, вы отсылаете им иногда багрепорты (поправьте пожалуйста если я ошибаюсь).
Вообще лично у меня какое-то подсознательное неприятие вызывает сам способ продвижения. Он направлен не на то, чтобы PVS пользовались разработчики а потом убедили руководство купить лицензию, а на то, чтобы убедить высшее начальство пользоваться этим всех. Лично мне это напоминает какие-то корпоративные продукты от IBM, чье «качество» известно всем, кто ими пользовался. Это не в том смысле, что ваша система плохая, а в том что ассоциации не самые приятные.
SvyatoslavMC
03.05.2017 14:01+1А между прочим вполне правильный вопрос поднят.
Вопрос поднят неправильно, почему:
Условно говоря у каждой ошибки есть цена...
Что Jef239, и теперь Вы, говорите о какой-то «условной» величине, которую никто даже не потрудился назвать, а почему? Потому что её нет, это «внутренная кухня» компаний, в которых эксперты уже подсчитали все риски, приняли решение о преобретении инструмента или об отказе от него. И как вы уже поняли, перед нами никто не должен отчитываться в этом.
Это расходится с вашими поисками ошибок в открытом коде.
Давно не секрет, что на открытых проектах мы демонстрируем возможти анализатора, всегда уведомляя авторов. Давно есть возможность использовать PVS-Studio бесплатно.
поправьте пожалуйста если я ошибаюсь
Поправил.
firegurafiku
03.05.2017 16:09+4Давно есть возможность использовать PVS-Studio бесплатно.
Недавно думал посоветовать одному из своих студентов прикрутить PVS-Studio к дипломному проекту. Посмотрел на формулировку вида «Dear PVS-Studio, would you kindly check my stupid project, please.» и передумал советовать. Формально вы правы: вы даёте возможность пользоваться своим продуктом бесплатно, но «вы делаете это без уважения».
В особенности я не согласен с вашей политикой размещать этот комментарий именно на первых двух строках файла с кодом: если каждый условно-бесплатный инструмент начнёт требовать подношений таким образом, то больше одного такого инструмента подключить к проекту не получится.
Впрочем, вы, наверняка, все подобные аргументы уже слышали. Уверен, вы сознательно выбрали именно такие формулировки и ограничения, и это целиком ваше право. Право недовольных — предпочесть ваших конкурентов, несмотря на тот информационный шум, что вы устраиваете на Хабре.
SvyatoslavMC
03.05.2017 17:31-1Недавно думал посоветовать одному из своих студентов прикрутить PVS-Studio к дипломному проекту.
Лучше посоветуйте. Сам писал для диссертации большое приложение с MPI и в последние дни, когда делал срочные правки, смог поймать ошибки с индексами.
Насчёт комментариев всё просто. Большинству людей всё равно до парочки комментариев, особенно студентам. Это защита от коммерческих проектов с открытым исходным кодом.
Andrey2008
03.05.2017 17:35Это защита от коммерческих проектов с открытым исходным кодом.
И с закрытым тоже. :) Не каждый менеджер будет рад видеть в проекте компании такие комментарии.firegurafiku
03.05.2017 18:53+3Лучше посоветуйте.
Лично я нахожу условия неприемлемыми для любого публикуемого проекта, независимо от его лицензии. Как с таким убеждением можно кому-то рекомендовать принять эти условия?
(...) когда делал срочные правки, смог поймать ошибки с индексами.
У нас возможностей запутаться в индексах просто море, но мы возлагаем большие надежды на ассерты и valgrind.
Не каждый менеджер будет рад видеть в проекте компании такие комментарии.
Я не менеджер, но совершенно не рад видеть строчки с сюрным текстом первыми в файле. При этом, я бы вполне смирился с упоминание об анализаторе (со ссылкой на сайт разработчика), но в более нейтральной формулировке и сразу после информации о лицензии проекта.
Не могу представить подобного рода комментарии, например, в Boost или Qt. Выдвигая такие условия, вы достаточно ясно говорите своей аудитории: «На самом деле, мы не хотим, чтобы вы использовали наш анализатор бесплатно для чего-то серьёзного — поэтому или идите купите лицензию, или превратите свой проект в нашу рекламную вывеску.»
Andrey2008
03.05.2017 19:07+1Выдвигая такие условия, вы достаточно ясно говорите своей аудитории: «На самом деле, мы не хотим, чтобы вы использовали наш анализатор бесплатно для чего-то серьёзного — поэтому или идите купите лицензию, или превратите свой проект в нашу рекламную вывеску.»
Да, так и задумано. Не вижу ничего в этом плохого или нечестного.
У нас возможностей запутаться в индексах просто море, но мы возлагаем большие надежды на ассерты и valgrind.
Возлагать то можно, но лучше, когда разные инструменты дополняют друг друга. Тот-же valgrind не видит то, что замечает PVS-Studio. Ну-ка, что это у него такое вот тут в коде:
} else { goto no_match; //ATC cc = iselCondCode( env, guard ); }
Ай, это же мёртвый код. Фи, нехорошо… :) В пятницу будет статья про проверку valgrind.firegurafiku
03.05.2017 19:44+2«… или идите купите лицензию, или превратите свой проект в нашу рекламную вывеску.»
Да, так и задумано. Не вижу ничего в этом плохого или нечестного.Я тоже не вижу ничего нечестного. Но анализатор ваш не порекомендую использовать никому и никогда.
Ай, это же мёртвый код. Фи, нехорошо.
Для этого совершенно не обязательно использовать PVS-Studio. Опция
-Wunreachable-code, вовремя переданная вclang++(жаль, что из GCC её молча выпилили), позволит разработчику сэкономить немного денег/самоуважения.Andrey2008
03.05.2017 20:03Для этого совершенно не обязательно использовать PVS-Studio
Это всё лирика. :) Обязательно, не обязательно, а ошибка найдена именно с помощью PVS-Studio. Как и ещё 11037 ошибок. О чем это говорит?
- Анализатором PVS-Studio для поиска многих ошибок пользоваться удобнее и приятнее, чем компиляторами. У компиляторов много ложных срабатываний, неудобный способ фильтрации, плохая документация и т.д.
- Анализатор PVS-Studio мощнее. Более того, процесс работы над ядром анализатора только усиливается.
И за это мы получаем деньги.
И ещё мы очень скромные, да.
Andrey2008
04.05.2017 11:48-1
balexa
03.05.2017 16:23+3Ок, с бесплатной версией был неправ, прошу прощения.
Теперь все же к поднятому вопросу.
Вы в статье пишете
Я в очередной раз смог продемонстрировать читателю, какую пользу могут приносить инструменты статического анализа кода.
Возникает вполне конкретный вопрос — насколько эта польза измерима? Или это, цитирую «условная величина, которую никто даже не потрудился назвать „
Потому что её нет, это «внутренная кухня» компаний, в которых эксперты уже подсчитали все риски, приняли решение о преобретении инструмента или об отказе от него.
Ваш ответ весьма странный, с точки зрения блога пытающегося продать продукт. Я вот представляю, прихожу я в ту же мосигру, спрашиваю “а чем игра А отличается от игры B, что вы посоветуете купить?», в ответ получаю «наши покупатели уже купили игру и сами посчитали, они перед нами не отчитываются».
Вопрос — что дешевле, купить PVS или на эти деньги нанять пару лишних тестировщиков вполне легальный, и он неминуемо возникает при рассмотрении полезности софта. У вас нет статистики — ок, так и скажите, это тоже вполне нормальный ответ. Ответ на этот вопрос зависит от каких-то других факторов — ок, так и скажите.
Просто мне реально непонятно, в своих статьях вы пишете что внедрение PVS позволяет улучшить качество кода. Есть много других способов улучшить качество кода. Вот и возникает вопрос — чем ваше решение лучше и дешевле, при ограниченных финансах (а они всегда ограничены) и в каких случаях.
У меня возникает ощущение, что я этими вопросами вас как-то обидел, сразу после опубликования комментария вашего комментария получил минусы в карму. Такое чувство, что эти вопросы вам неудобны, надеюсь что это не так.Andrey2008
03.05.2017 17:09-2Вопрос — что дешевле, купить PVS или на эти деньги нанять пару лишних тестировщиков
Цена анализатора по сравнению стоимостью зарплаты команды программистов пренебрежимо мала. Так что не понятно, что тут собственно высчитывать. Анализ кода (т.е. контроль его качества) или нужен, или не нужен.
Я даже про тестировщиков не понимаю. Возьмём двух тестировщиков с зарплатой скажем в 40000р. В год на них будет потрачено 40*2*12=960 т.р. Это ещё без налогов. А если добавить траты на рабочие места, аренду, бух. сопровождение и т.д., то в итоге эти два тестировщика за год съедят около 1,5 млн. рублей. Эти траты уже в несколько раз больше, чем стоимость базовой версии PVS-Studio.
Поэтому я и говорю, Вам или нужен дополнительный контроль качества кода или не нужен. Статический анализ, это ответ на вопрос «что мы ещё можем сделать для улучшения качества проекта?». Естественно на него стоит отвечать только тогда, когда уже есть и квалифицированные программисты и тестировщики. А если их нет, тут уж не до статического анализа… :)Andrey2008
03.05.2017 17:33P.S. Не нравится в своём собственном ответе, что я о статическом анализе сказал как только о приятном дополнении в виде повышения качества кода. Но ведь очень важно, что выявление ошибок на ранних этапах, это ещё и большая экономия времени на отладку и тестирование. Т.е. ошибка может быть найдена сразу и исправлена. А если её будет находить дополнительно нанятый человек, то это целый процесс, где будет участвовать тестировщик, программист, багтрекер, отладчик и так далее. К сожалению, тут посчитать вообще очень сложно.
Jef239
04.05.2017 04:14-5Статический анализ, это ответ на вопрос «что мы ещё можем сделать для улучшения качества проекта?».
Вот именно. На сей раз вы правы на 100%. В этой ситуации можно ракеты освящать. Или офисы сбербанка. Можно рядом с каждым монитором персональный кактус поставить. А можно и станализатор прикупить. Особенно, если проект в стадии агонии, то есть каждое изменение в коде может давать неожиданные эффекты, которые никто не может предвидеть.
А обычно вопрос иной: а во что нам выгоднее вложить средства? В рекламу? В маркетинг? В поиск новых ниш на рынке? В развитие продукта? Или в улучшение качества? Что даст больший доход?
А если уж вкладывать в улучшение качества, то какие меры будут эффективней? Выдать каждлму разработчику по бубну? Ввести систему премий и штрафов? Сделать рефакторинг? Переписать продукт с нуля? Применить одну из модных технологий?
А какую модную технологию применить? TDD? Парное программирование? Купить станализатор?
И вот тут от вас ждут ответа почему покупка статанализатора выгодней, чем реклама. А ответа у вас, увы нет.SvyatoslavMC
04.05.2017 07:38-1И вот тут от вас ждут ответа почему покупка статанализатора выгодней, чем реклама. А ответа у вас, увы нет.
Вы опять что-то попутали. Мы утверждаем, что исправлять ошибки в коде сразу выгоднее, чем тащить их через тестеров и клиентов. Выгоднее, чем зарплата нескольких тестеровщиков…
Какая реклама, вы чего? Мы не участвуем в планировании действий компаний, не ходим на их собрания и не планируем их бюджет… Что за бред?Jef239
04.05.2017 18:25-2Вот только ваша компания не покупает Coverity, хотя качество кода он нам несомненно улучшит. Немного, на доли процента — но ведь улучшит?
Зато — вы активно рекламируете свой продукт. Настолько активно, что по словами «Статический анализ» находятся в основном статьи вашей компании.
Как видите, вашей компании реклама намного важнее качества кода. И это нормальная ситуация. Обычно важнее всего рынок сбыта, потом развитие продукта (для удержания и захвата рынка сбыта) и где-то близко к последнему месту — качество кода.
И лишь в двух ситуациях качество кода становится существенным:
- Когда оно настолько низко, что продукт вошел в стадию агонии, то есть «каждое изменение в коде может давать неожиданные эффекты, которые никто не может предвидеть.»
- Когда у нас специфичная область (авионика, атомная энергетика и так далее).
Мы утверждаем, что исправлять ошибки в коде сразу выгоднее, чем тащить их через тестеров и клиентов. Выгоднее, чем зарплата нескольких тестеровщиков…
Докажите это. Только нормальными расчетами экономики. И без сказок о том, PVS-studio находит ошибки на уровне тестера.
Собственно именно этого доказательства мы от вас и ждем.SvyatoslavMC
04.05.2017 19:24+3Докажите это. Только нормальными расчетами экономики.
У разработчика Вовы был хороший день, пока начальник не сообщил, что у клиентов, которые обновили софт на всех офисных компьютерах — появились проблемы с вводом данных.
Вова потратил несколько часов, изучая разные ревизии кода и нашёл следующую ошибку:
void SetActive(bool bActive) { if (bActive == bActive) return; m_bActive = bActive; OnResetState(); }
Некий контрол для ввода в некоторых ситуациях не становился активным.
Вопрос: сколько люлей получил Вова и его руководство за простой в бизнесе клиентов? Сколько можно было бы сэкономить, найдя ошибку статическим анализатором?
Jef239
04.05.2017 19:54-5В ответ на вашу вымышленную историю — реальная ситуация. В авионике станализаторы используются в обязательном порядке. Причем очень высокого класса. И что? Помогли они хоть капельку? Дичайший баг: в релизе просто не реализован алгоритм из ТЗ, а станализатор молчок.
А в вашей истории про Вову — как раз был смысл нанять тестера. Потому что если бы было написано if (bActive == bActive1) — стат анализатор бы ничего не заподозрил. А если бы тестер был — люлей дали бы ему, а не девелоперу. Ну или на gcc перейти — у него есть эта диагностика.
P.S. Частный пример того, что находят компиляторы — это не доказательство. Увы, это обычный маркетинговый обман.SvyatoslavMC
04.05.2017 20:27+3Дичайший баг: в релизе просто не реализован алгоритм из ТЗ, а станализатор молчок.
Если по утрам пользоваться статическим анализатором PVS-Studio, то можно своевременно узнать о простуде и принять меры. Но об этом тссс… Контр-фича войдёт только в следующий релиз. Тссс…Jef239
04.05.2017 20:31Зато бубен давно помогает находить очень много.
И не только находить. Только плясками с бубном вы заставите неработающий код — работать.
Jef239
04.05.2017 19:12-3Скажите а PVS-Studio делает:
- нагрузочное тестирование, то есть нахождение бутылочных горлышек в архитектуре?
- проверку соответствие документации коду?
- регрессионное тестирование?
- проверку на полноту реализации ТЗ?
- ищет грамматические ошибки в документации?
- находит неудобства у UI?
- и так далее, и так далеее
Нахождение тех кодовых ошибок, что может найти статанализатор — это 5% времени тестера, не более.
Ну вот и считаем. 5% от 1.5 миллиона — это 75 тысяч. В 4.5 раз меньше цены PVS-Studio
А теперь вспомним, что тестер находит баги, которые вылезут у большинства клиентов, а статанализатор — и то, что вылезет и то, что практически никогда не вылезет…
И в итоге получается совсем печальная для вас картина.
У меня есть предложение. Вы можете снабдить демо-версию нормальной лицензией? Интересуют два момента:
- Разрешение на использование демо-версии без цели покупки
- Разрешение на изменение кода по данным анализа, произведенным демо-версией.
Взамен можно проверять присутствие в файле (на любой строке) комментария «Checked by DEMO-version of PVS-Studio». И добавлять его, если такого комментария не нашлось.
Вполне возможно, что как раз DEMO-версия — продаваема. По бросовым ценам, но продаваема. В смысле консольная версия, которая не имеет «кликов», и связанных с ними ограничений.
SvyatoslavMC
03.05.2017 17:40Возникает вполне конкретный вопрос — насколько эта польза измерима? Или это, цитирую «условная величина, которую никто даже не потрудился назвать „
Давайте посадим дерево, выпьем минеральной воды, поспим 8 часов… насколько эта польза измерима?
Есть много других способов улучшить качество кода.
Есть много других способов улучшить качество кода. Не пойму, где конфликт. Обычно компании применяют комплексные меры для повышения качества кода. Есть ещё много других способов управления проектом, тоже помогает. Опять, нет конфликта ни программ, ни людей.Jef239
04.05.2017 04:15Конфликт в ограниченности средств. При безразмерном бюджете конфликта вправду нет.
Jef239
04.05.2017 03:58-4А причина минусов простая — PVS-Studio надо рассматривать наравне с бубном, кактусами у мониторов, освящением помещений и кучей других верований и суеверий.
При этом я не отрицаю, что с помощью плясок с бубном было исправлено множество ошибок. И сам могу продемонстрировать пользу бубна. Ибо любое плацебо — немного помогает.
Но секте поклонников PVS-studio ни вы, ни я ничего не докажем. Проще уж вегетарианца научить есть мясо.
Jef239
04.05.2017 03:33-2говорите о какой-то «условной» величине, которую никто даже не потрудился назвать, а почему?
Потому, что не вижу смысла объяснять то, что все проходили в школе на уроках обществоведения. Деньги — это мерило стоимости товара. Так что измеряться они будут в рублях.
Потому что её нет, это «внутренная кухня» компаний, в которых эксперты уже подсчитали все риски, приняли решение о преобретении инструмента или об отказе от него.
Ну вот уже и мифические эксперты появились. Гм, видимо придется объяснить, почему мифические…
«Анализ эффективности внедрения» — это абсолютно обычная вещь для любых систем. Вот вам для CRM, вот для СЭД, вот для ИС, вот для АСУ. Уровень — от курсовой до научной работы. Для некоторых областей — даже ГОСТ есть.
А вот по анализу эффективности внедрения статического анализа не находится ничего релевантного. Так что никакой науки тут нет. Ну и соответственно — нет экспертов. Единственные, кого можно назвать экспертами — это вы и сами и другие разработчики статнализаторов.
Давно не секрет, что на открытых проектах мы демонстрируем возможти анализатора
Вот в этом и ваша проблема, что вы демонстрируете возможности, а не экономическую эффективность. Знаете, у сверхзвукового истребителя тоже вагон возможностей, Но многие ли хотят его купить себе? Покупают-то больше ford focus, у которого и скорость поменьше, и кучи возможностей нет…
Потому что её нет, это «внутренная кухня» компаний,
Да нет, многие рассказывают о своей кухне. Но в одной из статей вы проговорились.
В больших проектах уже никто не знает, как всё работает. Из-за этого поиск и исправление ошибки обходятся на порядок дороже. Каждое изменение в коде может давать неожиданные эффекты, которые никто не может предвидеть.
Эта стадия проекта называется агония, потому что следующая стадия — это смерть. И вот такие агонизирующие проекты — и есть ваши клиенты. Правильное решение — переписать систему заново, с нуля. Но такое решение настолько дорого, что менеджмент хватает за разного рода соломинки. Освящает помещения, зовет шаманов, закупает статический анализатор…
Не удивлюсь, если большинство ваших клиентов — это именно агонизирующие системы. Ничего страшного в этом нет, просто нужно честно признать — PVS-Sdudio — это продукт для систем в стадии агонии.
А для обычных PVS-Studio просто не выгоден. Вот вам простейший экономический расчет на уровне начальной школы. Поскольку современная цена PVS-Studio неизвестна, возьмем цены 2013 года: €5250 на 9 разработчиков на год, то есть примерно 330 труб по курсу. На одного разработчика это будет 36.6 труб на год.
Не секрет, что работодатель на одного работника тратит примерно в полтора раза больше, чем зарплата, выдаваемая на руки. При зарплате в 50 труб в месяц в год будет 50*12*1.5 = 900 тысяч рублей. В году примерно 2 тысячи рабочих часов. Таким образом цена часа 450 рублей.
Получается, что для нулевой окупаемости PVS-Studio должен экономить 33600/450 — примерно 75 часов в год, то есть почти 10 рабочих дней.
Увы, весь ваш блог показывает, что речь идет лишь о копеечной экономии. Да, может быть для разработчика с оплатой 500 труб в месяц PVS-Studio будет выгодной. Но это не массовый сегмент.
Увы, при развитии PVS-studio было допущено много менеджерских ошибок. И ваше нынешнее непонимание экономики — это всего лишь очередная ошибка. Равно как и отсутствии образа типичного покупателя, отсутствие внятной лицензии на демо-версию и так далее…MikailBag
05.05.2017 20:42+1Среди их клиентов:
- Microsoft
- Epic Games
- Wargaming
- Kaspersky
Неужели вендекапец так близко?
Jef239
05.05.2017 22:05-2В микрософт — 120 тысяч работников. Ну скажем 50 тысяч программистов. Цена enterprise лицензии от от €9000, ну скажем $50 000. Получается — по доллару на програмиста в год.
ВАУ! «Дайте две!» При годовой стоимости в доллар — я сам куплю с удовольствием. И не буду смотреть, важное или неважное он нашел. Рано или поздно — найдет важное и окупиться. Тут главное, чтобы польза от найденного была больше, чем время на прочтение и анализ диагностик.
Иными словами — экономика фирмы с сотней тысяч работников — совсем другая.
Что касается «венлекапец», то линейка Windows 95/98 начала писаться с 1983 года и скончалась в 2000 с выпуском WinME. Судя по книгам с разбором кода — как раз полная агония там и была. Линейка Windows NT/XP/7/10 начала писаться с 1988ого года. За 19 лет — скорее всего никого из исходной команды не осталось. С другой стороны — там была классная команда архитекторов. Так что думаю, что агональная стадия там лишь местами. И лет 10 ещё винда проживет.
Jef239
04.05.2017 03:49-1Они лет 7 пытались убедить разработчиков. Не вышло. Теперь пытаются убедить менеджеров. Тоже вряд ли выйдет. Собственно они уже разок проговорились о своей модели клиента:
В больших проектах уже никто не знает, как всё работает. Из-за этого поиск и исправление ошибки обходятся на порядок дороже. Каждое изменение в коде может давать неожиданные эффекты, которые никто не может предвидеть. Поэтому, если инструмент помогает предотвратить хотя-бы 10% ошибок, это уже огромная экономия сил и в времени.
Была у них хорошая идея с SaaS. Но моя личная попытка узнать у них цены просто провалилась. В ответ очень веждиво и многословно цену назвать отказались.
DarkEld3r
06.05.2017 13:58+1Они лет 7 пытались убедить разработчиков. Не вышло.
Да ладно? Мне вот польза (подчёркиваю: польза, а не "панацея") статического анализатора очевидна. Аналогично с тестами, ревью, "континюес интегрейшн" и прочими валгриндами. Знакомые программисты мнение разделяют.
Другое дело, что решение как расходовать деньги, зачастую, принимают как раз не программисты, а менеджеры.
Jef239
06.05.2017 20:32+1Тогда, может быть, поделитесь своими знаниями? Беда в том, что кроме пользы, есть стоимость анализа. Она складывается из трех вещей: цена анализатора, время на разбор результатов, время на приведение кода в состояние, когда нет предупреждений.
Когда сотрудники простаивают, а анализатор бесплатен — польза есть от любого анализатора. А вот когда у сотрудников цейтнот, а анализатор стоит килобакс — вместо пользы легко получить проигрыш. Как по деньгам — так и по срокам завершения проекта.
Я уже много раз писал свою оценку. Компиляция с -Wall -Wextra оправдывается. Даже если для этого проект надо переводить с винды на linux. А вот cppcheck не оправдывается. Обычно после проверки им жалко времени, потраченного на анализ. Ну вот вам один из примеров.
Если вы считаете, что польза от PVS-Studio есть, то расскажите, что же оправдывает килобакс его цены? Насколько ускорилась сдача проекта? Насколько увеличились продажи?
SvyatoslavMC
06.05.2017 21:37-1Компиляция с -Wall -Wextra оправдывается. Даже если для этого проект надо переводить с винды на linux.
Не хочу никого обидеть, но такую оценку может дать только неопытный человек. Анализ | портирование — эти задачи даже рядом не стоят по требуемым затратам.Jef239
06.05.2017 23:08У нас основной проект работает под MS-DOS, Windows, freeBSD, QNX, Linux, FreeRTOS, МСВС. Соответственно есть своя библиотека, выполняющая в том числе функции слоя совместимости.
Так что перевод под liux — это в основном замена нескольких инклудов. Могу в личку патч кинуть. Ну и makefile сделать путем cut&paste. Работа простая и быстрая.
Чтение статей в вашем блоге показывает, что на понимание диагностик PVS-Studio и принятие решения о правки (или отключении предупреждения) требуется время от 5 минут до получаса. Много очень тонких ошибок, про которые сложно понять, баги или не баги.
С другой стороны, у меня много опыта в портировании, а у вас — в анализе предупреждений.
В целом оценки такие. На чуть более тысячи строк кода — полчаса на портирование под linux, 3 часа — на исправление 33 предупреждений, которые нарыл gcc. Но все это время — оправдывается.
А вот cppcheck — не оправдывается даже затраченное на него время. Тех ошибок, что видит gcc, он не замечает в упор (видимо by design он не дублирует gcc). Например — неполный перечень значений перечислимого типа в switch.
DarkEld3r
06.05.2017 23:18+2Насколько увеличились продажи?
Повторюсь: (к счастью) это не моя задача.
Со "сдачей проекта" уже не всё так просто. Скажем, почти всегда проще и быстрее воткнуть костыль. Если же планируется долгая жизнь проекта, то накопившаяся гора костылей может больно аукнуться.
Мой опыт такой, что если цейтнот постоянно, то что-то делается неправильно и страдать будет как качество, так и такая "эфемерная" штука "удовольствие от работы" (ну или "уровень стресса"). В итоге сотрудники будут перегорать и разбегаться. Ну это если аргумент про "некогда точить, пилить надо!" не принимается. А если периодически выделяется время на разгребание накопившихся проблем и прочий рефакторинг, то найдётся и на то чтобы прикрутить анализатор и задавить имеющиеся предупреждения. Что касается цены, так на прошлом месте работы выбирали между PVS и Coverity и надо сказать, что второй инструмент стоит на порядок дороже.
Впрочем, вы говорите о "завершении проекта". Возможно, у нас сильно разный опыт, так как, в основном, сталкивался с (потенциально) "бесконечными" проектами. Впрочем, это не значит отсутствие своих релизов/дедлайнов. Но опять же, в моей практике, с какого-то момента это всё окупается, тем более, что ресурсы на интеграцию придётся потратить один раз.
Аргумент про ошибки в ТЗ мне совершенно не понятен. В том плане, что серебряной пули не бывает и все проблемы не решит ни один инструмент, но из этого никак не следует бесполезность этих самых инструментов.
Jef239
07.05.2017 02:25-2Увы, ни одного аргумента я от вас не услышал, только голые заявления о полезности.
Хотя и ожидал услышать конкретные цифры вроде на анализ и правку найденного тратиться Х часов в неделю, из найденного K% оказалось критичным, что привело к экономии Yчасов в неделю. И вывод, что Y настолько больше Х, что стоит платить. Ну или не стоит.
В качестве развлечения — можете убедить авторов PVS-Studio разок проверить свое творение при помощи valgtind. Динамический анализ хорош тем, что ловит ошибки на «главному пути», то есть найденное им действительно нуждается в исправлении.
Вопросы управления техническим долгом — это отдельная тема для обсуждения. Что касается цейтнота, то я просто за 30 лет работы так и научился работать от сих до до сих. Или уж как в понедельнике — или менять работу.
Серебряной пули нет, но есть ошибки важные и неважные. Если при обновлении прошивки в датчике проект перестает получать от него данные — это важно. А если программа ломается при командной строке длиннее 4 килобайт — это неважно, потому что командную строку пишем мы сами. И если уж злоумышленник долез до командной строки — он может с тем же успехом и топором ударить.
При вагоне свободного времени у сотрудников и бесплатности инструментов — любой инструмент будет полезен. А как только рабочее время и инструмент стновятся платными — так сразу и возникает вопрос о полезности отдельных инструментов.
Вы осциллограф себе для отладки купили? А ведь он полезен! Вот только для вас он полезен абстрактно, а для нас — конкретно. Как простейший пример — вместо того, чтобы гадать — то ли провод сдох, то ли воткнули криво, то ли сбой в программе — можно просто посмотреть, идет ли сигнал.
Ну вот пока что полезность PVS-Studio для нас — на уровне полезности осциллографа для вас.DarkEld3r
07.05.2017 23:55+3Хотя и ожидал услышать конкретные цифры...
Как вы себе это представляете? Даже если бы кто-то и решился на такой эксперимент, то как обеспечить объективность? Ведь нельзя дважды написать одинаковый код одинаковыми людьми в одинаковых обстоятельствах, но в одном случае использовать статический анализатор.
потому что командную строку пишем мы сами...
С чего вы решили, что я собираюсь доказывать нужность инструмента конкретно вам? Или вообще кому-либо. Понятное дело, что ситуации бывают разные, я просто напомню контекст: в моём окружении люди понимают ценность статического анализатора (собственно сейчас вообще на расте пишу по аналогичным причинам). Всё. Разумеется, глобальных выводов из этого делать нельзя, точно так же, как и из вашей аргументации.
Вы осциллограф себе для отладки купили? А ведь он полезен! Вот только для вас он полезен абстрактно, а для нас — конкретно.
Передёргивание. Осциллограф полезен для ограниченного числа ситуаций/проектов, статический анализатор — практически для любого кода. Размер этой пользы (и "стоимость") можно обсуждать, но отрицать её целиком мне кажется странным.
Ну и уточню ещё один момент касательно "стоимости": если у нас на проекте (условно) два человека, то да, это всё дорого, сложно и некогда. А если под сотню, то анализатор стоит копейки относительно всех затрат.
Jef239
08.05.2017 01:35-3Легко представляю. Просто не надо стремиться к лишней точности. Вот вам анализ для -Wall -Wextra.
50% варнингов и 10% хинтов — это реальные ошибки. Процентов 80 из них — на главном ходу, то есть обязательно должны быть найдены и исправлены. Среднее время на исправление замечания gcc — 3 минуты. Среднее время на нахождение и исправление бага тестированием — 1 час.
Теперь считаем на 100 варнигов. В пассиве — 100 на 3 минуты — 5 часов на обработку. В активе 100 * 50% * 80% + 1 час = 40 часов. 40/5 — 8 раз. Вот в 8 раз -Wall лучше -w.
Считаем на 100 хинтовв. В пассиве — 100 на 3 минуты — 5 часов на обработку. В активе 100 * 10% * 80% + 1 час = 8 часов. 8/5 — 1.6 раза. Вот в 1.6 раза -Wextra лучше.
Разумеется, все это очень приблизительно. По варнингам может быть и не 8 раз, а 5. Или 15. По хинтам может быть и 0.9 и 3. Но примерно — расклад такой.
в моём окружении люди понимают ценность статического анализатора
В моем окружении понимание чего-либо начинается с возможности доказать свою точку зрения и подтвердить её расчетами. А все остальное — это вера. Ну или суеверие. С другой стороны интуиция опытного инженера — это вещь, которой лучше доверять, чем не доверять. Даже если она основана на вере.
Осциллограф полезен для ограниченного числа ситуаций/проектов, статический анализатор — практически для любого кода
А вы оглянитесь вокруг себя. Микроволновка, стиральная машина, лифт, кодовый замок… всюду вокруг вас — embeded. То есть устройства, к которым проще подключить осциллограф, чем организовать вывод на консоль. Фактически, как только вы уходи с linux на freeRTOS или голое железо — так сразу есть смысл начать просчитывать выгоду от осциллографа.
С другой стороны, есть несколько замечаний от авторов PVS-Studio:
- До 250 тысяч строк PVS-Studio не оправдывается.
- Пока у кода есть конкретные авторы, которые помнят код целиком — PVS-Studio не оправдывается.
- Там, где нет динамического выделения памяти (то есть embeded) PVS-Studio не оправдывается.
Если сильно нужно — найду ссылки, где об этом написано.
А если под сотню, то анализатор стоит копейки относительно всех затрат.
А вот тут согласен. При сотне девелоперов цена анализатора будет равна 2-4 дням работы девелоперов. А количество ошибок — прилично растет.DarkEld3r
08.05.2017 23:49Легко представляю. Просто не надо стремиться к лишней точности.
Ну да, так что-то посчитать можно, но у меня вызывает сомнение полезность таких грубых прикидок. В любом случае, временный ключ авторы выдают по запросу, если очень интересно, то можно и посмотреть на своём проекте. Или вы хотите чтобы это кто-то другой для вас сделал? (:
А вы оглянитесь вокруг себя. Микроволновка, стиральная машина, лифт, кодовый замок…
Дык, я не говорю, что осциллограф бесполезен всем. Хотя всё-таки кажется, что если считать "проекты" или, тем более, строки кода, то железячных проектов окажется меньше.
Если сильно нужно — найду ссылки, где об этом написано.
Тоже что-то такое припоминаю и возражений нет — звучит вполне логично. Если это ваш случай, ну что ж, не нужен инструмент и ладно.
Jef239
09.05.2017 00:41-1если очень интересно, то можно и посмотреть на своём проекте
Судя по лицензии, есть только один честный способ использования демо-версии — попробовать перед покупкой. А использование без цели покупки — это пиратство. Ну как-то не привык я пиратить программы российских разработчиков.
меня вызывает сомнение полезность таких грубых прикидок.
Почему же? Вы когда машину покупаете, что вначале делаете? Тест-драйв или в интернете отзывы ищете? А тут ещё и неизвестный гаджет. Так что отзывы прежде всего. Производитель — он всегда себя нахвалит. А на деле — бывает иначе…
В любом случае, для почти всех типов систем анализ эффективности внедрения — это курсовая или глава в дипломе. Есть методики, есть расчеты, и они делаются до покупки. И мы почти в каждом договоре такой раздел пишем. А вот на статанализаторы методики нет. И то, что авторы PVS-Studio её не придумали — намекает, что с эффективностью плохо…
если считать «проекты» или, тем более, строки кода, то железячных проектов окажется меньше.
Если помножить строки кода на количество экземпляров… Ну вообще-то в каждой флэшке — 1-2 процессора. Один для общения по USB, а второй — для равномерной нагрузки на сектора flash. В вашем мобильнике — процессоров десяток. Тот же GPS-приемник — это 1-2 процессора (DSP+ARM). Сейчас проще запихнуть процессор, чем делать аналоговым путем.
Но я соглашусь с тем, что это ненастоящее программирование… Когда причиной бага оказывается лишняя капля припоя на ножках процессора — это уже ближе к радиотехнике…
DarkEld3r
09.05.2017 16:45Судя по лицензии, есть только один честный способ использования демо-версии — попробовать перед покупкой.
Зачем нужна такая демо-версия, которая обязует к покупке? (:
Jef239
09.05.2017 21:08-2Покупка не обязательна. Но использовать, ну скажем, с целью сравнения возможностей — эта лицензия не позволяет. И не факт, что найденные демо-версией ошибки можно исправлять в коде.
Jef239
08.05.2017 03:35-4Собственно то, что я вижу в блоге PVS-Studio — это антиреклама. Берут большой проект, проверяют, находят какую-то мелочевку. Ни одного случая, чтобы нашли какой-то важный баг, который пользователи постоянно репортят, я не видел. Судя по их блогу PVS-Studio еле-еле оправдывает время, потраченное на проверку проекта и исправление кода. Ну или не оправдывает.
Конечно, если у нас управление атомным реактором или космическим кораблем — стоит применять все инструменты для поиска багов. Но это как раз редкость ещё большая, чем осциллограф при отладке.
Я надеялся, что вы опровергните это мнение. Но увы, видимо не получится.
Ещё раз повторю, что на мой взгляд 95% выгоды от статанализа дает компиляция программы разными компиляторами с максимальными предупреждениями. Более того, авторы PVS-Studio при доказательствах полезности регулярно ссылаются на те диагностики, что есть в компиляторах.
MikailBag
08.05.2017 16:44+3Эмм, если пользователи зарепортили какой-то баг, то его уже задебажили и исправили.
Собственно PVS об этом и говорят, что можно все баги исправлять с помощью дебага, но дешевле купить их анализатор.Jef239
08.05.2017 19:33-3Да ну? В открытых программах иногда баги больше 5 лет висят. Особенно, если баг мешает только русскоязычным пользователям. Да и в закрытых… Помнится был некий текст, после набора которого ворд вылетал…
Увы, есть баги, которые можно очень долго искать. Например: состояние гонки, редкая порча случайного места памяти… Динамический анализ в этих ситуациях несколько помогает, статический — не очень.
все баги исправлять с помощью дебага, но дешевле купить их анализатор.
Ну вот и докажите, что дешевле. Последняя известная цена анализатора — 600 тысяч рублей. Ну вот и докажите, что он нашел багов на миллион или больше в год.
Если у вас тысяча программистов — вполне верится, что анализатор дешевле. А когда четверо?
Пока что все попытки использовать бесплатный cppcheck показали, что он не оправдывает даже время, потраченное на анализ его логов и исправление кода.
DarkEld3r
08.05.2017 23:55+3Я надеялся, что вы опровергните это мнение. Но увы, видимо не получится.
Да, не получится.
Более того: если посмотреть на факты, то мы легко к соглашению придём. Вот только выводы делаем разные.
Ну и чтобы два раза не вставать: с какого-то момента, меня (тоже?) данная реклама стала слегка раздражать. Но надо отдать должное: продукт они делают востребованный (иначе разорились бы) и качественный. И как для рекламных статей, технических деталей даже много. (:
Jef239
09.05.2017 00:48-1Меня раздражает не реклама, а её расхождение с выводами её авторов. Сами статьи говорят о том, что продукты работают и успешно используются несмотря на кучу мелких недочетов. А авторы пытаются впарить, что без статанализатора — никак, что статанализатор способен найти действительно важные проблемы, а не эту кучу мелочей.
У меня такое впечатление, что скоро они к куче уже признанных ими менеджерских ошибок добавят ещё одну: отсутствие образа потребителя.
Более того: если посмотреть на факты, то мы легко к соглашению придём. Вот только выводы делаем разные.
Проверим? Был бы выгоден для вас PVS-Studio при наличии четырех разработчиков в команде?
Jef239
09.05.2017 14:34-1Ну вот вам пример, чего бы мне хотелось. Нашумевший баг Intel AMT
if(strncmp(computed_response, user_response, response_length))
exit(0x99);
computed_response вычитывается из ПЗУ, user_response — принимается по сети, длина берется от user_response — и получаем жирный баг.
Теоретически статический анализ может понять, где ввод пользователя, а где — записанный пароль. Практически — мне не верится, PVS-Studio это поймает. Но даже если оно скажет, что в качестве длины надо использовать max от длин строк — это уже полезно.
Вот что-то такого уровня важности у вас статанализ ловил? Увы, в блогах я настолько жирных примеров не видел. И вообще, не видел ни одного примера, когда публикация статьи откладывалась из-за нахождения реальной, крупной уязвимости.
Jef239
07.05.2017 18:57-2Чуть-чуть про осциллограф при отладке. Помимо абстрактной полезности есть условия, когда польза от осциллографа намного превышает его цену. Это ситуации:
- Когда используется кастомное железо (свое или заказное) и баг в железе (или в соединениях) столь же вероятен, как и баг в софте.
- Когда вывод в лог более трудоемок, чем вывод на GPIO. Например, чтобы посмотреть время обработки между приемом данных и выводом, можно задействовать высокоскоростной таймер, сделать усреднение, определение пиков… А можно установить 1 при приеме данных и 0 при окончании обработки. И сразу будет видно, нет ли задержек, всегда ли обработка успевает, как влияют нагрузка на другие нити...
- Когда программисты или в детстве были радиолюбителями или имеют радио-образование.
В итоге получается довольно узкая область, для которой осциллограф must have. Причем не только стационарный, но и переносной — для полевых испытаний.
Вот такую вот область для PVS-Studio я и хочу очертить.
Но пока что мое мнение — 95% пользы от статанализа дает проверка несколькими компиляторами с выключенными на максимум предупреждениями. Остальное — для нас не оправдывает цену.
Возможно, оправдаетсяпиратскоеиспользование бесплатной демо-версии PVS-Studio. Но тут уже вопрос лицензии и собственной совести. Ну как бы демо-версия — она только для оценки перед покупкой.SvyatoslavMC
07.05.2017 21:22Возможно, оправдается
сквозноепроветривание помещения, чтоб код не пах. Но тут уже вопрос веры и собственной адекватности. Ну как бы проветривание — оно только для улучшения условий труда.Jef239
07.05.2017 22:19-2Лучше сделайте проверку PVS-Studio при помощи valgrind. Скорее всего, довольно много интересного найдете.
Прежде чем отказываться от бесплатного продукта — имеет смысл его попробовать.MikailBag
08.05.2017 16:46Jef239
08.05.2017 19:12-2я в курсе, что пока не пробовали. Это-то и удивляет. С одной стороны они пытаются всех убедить, что качество кода — это самое главное. А с другой — не находят времени на разовое использование бесплатного инструмента.
Какое-то лицемерие, на мой взгляд.
SvyatoslavMC
07.05.2017 21:37Но пока что мое мнение — 95% пользы от статанализа дает проверка несколькими компиляторами с выключенными на максимум предупреждениями.
Вы забыли соответствующую опцию указать, я помогу :D
3.8 Options to Request or Suppress Warnings
-w
Inhibit all warning messages.
Jef239
07.05.2017 22:15-1На максимум — это -Wall -Wextra. -w — это на минимум.
Если не практикуете — реквестирую отчет, что нашел gcc в вашем собственном коде.
alexoron
03.05.2017 14:57+2Может пора требовать от Микрософта перед релизом Windows 11 анализаторами прочесывать?
Тогда гляди уязвимостей на порядок уменьшится, а скорость и стабильность увеличится.
И не надо будет патчи и сервис-паки каждый день клепать.
sumanai
03.05.2017 15:39+5Может пора требовать от Микрософта перед релизом Windows 11 анализаторами прочесывать?
Ага, притом что 11 версии не будет.
Я думаю, они и так пользуются каким-либо анализатором.
Dovgaluk
03.05.2017 19:25А странные сигнатуры operator++ и operator-- вы не видите смысла детектить?
Andrey2008
03.05.2017 20:03-1Прошу пояснить.
Dovgaluk
03.05.2017 20:39+1Во-первых префиксный и постфиксный операторы делают одно и то же, а во-вторых, префиксный возвращает константную ссылку.
Andrey2008
03.05.2017 20:41Да, мы подумаем про это.
Dovgaluk
03.05.2017 21:07Хотя про константную ссылку я не прав. Это ведь не добавляет ошибок в поведении.
fsmoke
99% это MFC/ATL код — мы все знаем как он писался и когда… честно говоря, открывая данную статью надеялся увидеть ошибки в стандартной библиотеке, что гораздо интереснее. Да и используется стд в наше время всё больше и больше нежели умирающие древности типа MFC/ATL.
iOrange
Поддерживаю. Открывал статью в надежде посмотреть как анализатор справится с развестисым спагетти шаблонов в std-библиотеке.
Andrey2008
В шаблонах особенно анализировать нечего. В том смысле, там куда не пойди везде неизвестные типы. В связи с этим анализ крайне ограничен в возможностях, потому что, когда нельзя вывести тип, лучше молчать.
Поясню на примере. Выход за границу массива:
Анализатор PVS-Studio здесь молчит. Ибо не понятно, что такое это T, какой тип массива и какой тип у индекса 'i'.
Но стоит добавить ниже:
И анализатор PVS-Studio радостно выдаст предупреждение:
Так что не стоит ожидать от анализатора чудес, если просто напихать в проект заголовочных файлов с шаблонными классами и функциями (что собственно я и сделал).
fsmoke
Дык и не надо ожидать — нужно просто взять более или менее сложный реальный проект(для чистоты эксперимента) с большим обилием стд и проверить его вместе со стандартной библиотекой, ну или на худой конец написать тесты хотя бы со стандартными типами — это не долго, хотя бы ради статьи. Все мы крестовики — и я не говорю про синтетические ситуации — типа перекрытого оператора неравенства != 11. Таких ситуации, которые выстреливают мимо бага одна на миллион. И на 90% этот анализ отразит состояние стандартной библиотеки того или иного компилятора — тем более Ваш PVS анализатор не плохо с этим справляется. А глазками уже можно будет понять баг это или фича или какая-то уникальная ситуация — и если это баг, отразить в статье — вот так как-то…
ПС
Вы создали великолепный продукт, но не нужно надеяться на то, что он за Вас будет и статьи на хабре писать, если Вы заявляете про анализ «Visual C++ 2017 Libraries» нужно ожидать, что люди ждут анализа именно самой главной — т.е. стандартной библиотеки — а что Вы хотели…
Andrey2008
С замечанием согласен.