Ідеальний шлях впровадження статичного аналізатора коду

Apple II emulator for Windows
Однією з основних труднощів при використанні інструментів статичного аналізу є робота з помилковими спрацьовуваннями. Існує безліч способів усунути помилкові спрацьовування, використовуючи налаштування аналізатора або змінюючи код. Я взяв маленький проект Apple II emulator for Windows і покажу, як можна на практиці працювати зі звітом статичного аналізатора PVS-Studio. Покажу на прикладах, як правити помилки і пригнічувати помилкові спрацьовування.

ВведенняЯ буду описувати ідеальний процес впровадження в проект методології статичного аналізу. Він полягає в тому, щоб усунути всі помилкові спрацьовування аналізатора і справжні помилки так, щоб аналізатор видавав 0 попереджень. Саме такий підхід ми застосовували, працюючи з проектом Unreal Engine 4.

На практиці ідеальний підхід рідко можливий. Тому у великому проекті розумно скористатися альтернативним підходом. Можна приховати всі наявні на даний момент попередження і бачити тільки повідомлення, що відносяться до нового чи зміненим кодом. Для цього в аналізаторі PVS-Studio існує спеціальний механізм, який зберігає інформацію в спеціальній базі. Подробиці можна дізнатися зі статті: Як впровадити статичний аналіз в проект, у якому більш 10 мегабайт вихідного коду?

Отже, приховавши всі повідомлення, Ви зможете уважно стежити за якістю нового коду. Знаходячи помилки в новому коді, ви швидко оціните всю міць і користь методології статичного аналізу. А коли у вас з'явиться вільний час, ви зможете повернутися до прихованих попереджень і поступово вносити необхідні поправки в код.

Однак повернемося до ідеального щасливого світу. Уявімо, що у нас є час, і ми можемо спокійно позайматися з попередженнями, які видає аналізатор PVS-Studio.

У цій статті я хочу показати, як працювати з попередженнями аналізатора. Причому ми пройдемо весь шлях — від першої перевірки, до вікна, в якому буде 0 попереджень.

Саме тому я вибрав маленький проект. Я міг би взяти і побільше, але тоді я втомлюся писати статтю, а ви — її читати. Втім, ви все одно втомитеся. Стаття навіть для маленького проекту вийде великий, але прошу приділити їй увагу. Вона може допомогти вам більш ефективно використовувати наш аналізатор коду.

Піддослідної миші став проект Apple II emulator for Windows. Вибір упав на нього абсолютно випадково. Тому не будемо на ньому зупинятися. Мені було все одно, який проект взяти, головне, щоб він виявився маленький, але при цьому в ньому щось знаходилося цікаве.

Характеристики проекту:
  • Розмір вихідного коду: 3 мегабайти.
  • Кількість рядків коду: 85700.
  • Час аналізу (8 процесорних ядер): 30 секунд.
Перший запускПісля першого запуску аналізатора ми маємо наступні попередження:

Малюнок 1. Попередження, видані при першому запуску аналізатора PVS-Studio для проекту Apple II emulator for Windows.
Малюнок 1. Попередження, видані при першому запуску аналізатора PVS-Studio для проекту Apple II emulator for Windows.

В статті я буду обговорювати тільки попередження 1 і 2 рівня, що відносяться до аналізу загального призначення (GA). Можна було б «перемогти» і 3 рівень, але тоді стаття затягнеться. На 3 рівень ми глянемо зовсім швидко, і я дам деякі пояснення, але виправляти нічого не буду.

Микрооптимизации (OP): зараз не цікаво.

Про 64-бітні діагностики: в проекті немає 64-бітної конфігурації. Не цікаво.

Перевіривши проект, я відсортував всі попередження за кодом. Це можна зробити, клацнувши мишкою на стовпець «Code» (див. малюнок N2).

Малюнок 2. Вікно з повідомленнями PVS-Studio. Повідомлення відсортовані за номером діагностики.
Малюнок 2. Вікно з повідомленнями PVS-Studio. Повідомлення відсортовані за номером діагностики.

Сортування по коду полегшує роботу з повідомленнями. Виникають групи однотипних повідомлень. Розібравшись у причині виникнення одного повідомлення, можна легко і швидко обробити інші.

Примітка У читача може виникнути питання, чому б відразу не робити таку сортування. Справа в тому, що хочеться дозволити користувачам дивитися на повідомлення ще в процесі аналізу. Якщо їх сортувати, то нові повідомлення в процесі аналізу будуть додаватися не кінець, а в різні місця списку. В результаті повідомлення буде «стрибати». Працювати з таким «дергающимся» списком буде неможливо.

Робота з повідомленнями аналізаторарішення (solution) є три проекти (вони видно у вікні Solution Explorer на малюнку N2). Два з них, zlib і zip_lib, перевіряти не цікаво. Тому їх потрібно виключити з аналізу. Насправді досить виключити zip_lib, так як zlib вже за замовчуванням додано в винятки. Це робиться у вікні налаштувань PVS-Studio (розділ don't Check Files):

Малюнок N3. Виключили zip_lib з перевірки.
Малюнок N3. Виключили zip_lib з перевірки.

Я виключив непотрібний проект заздалегідь. Однак це легко зробити вже після перевірки. Причому не обов'язково для цього вирушати в налаштування. У контекстному меню є пункт, що дозволяє зручно заховати всі повідомлення, що відносяться до файлу, або певного каталогу. Це дуже зручно. Рекомендую познайомитися зі статтею "PVS-Studio для Visual C++". У ній описана ця та інші можливості, які допоможуть ефективно використовувати інструмент.

Отже, все готово до початку роботи над повідомленнями. Почнемо з діагностик під номером V501 і підемо далі. Всього ми розглянемо 32+49 = 81 повідомлення. Це досить багато. Тому місцями я буду писати докладно, а місцями буду лаконічним.

Помилкове спрацьовування на макросах xxxxxREGПерші 6 повідомлень виникають із-за складних макросів ADDXXREG, ADCHLREG, SBCHLREG, SBCHLREG. При їх розкритті виникають надлишкові конструкції, і аналізатор видає, наприклад, таке повідомлення:

V501 There are identical sub-expressions to the left and to the right of the '^' operator: (tmp >> 8) ^ reg_ixh ^ reg_ixh z80.cpp 3444

Макрос ADDXXREG досить великий і складається з інших макросів. Тому не буду приводити його в статті.

Нам важливо, що операція XOR двічі виконується із змінною reg_ixh. Відповідно вираз можна спростити до (tmp >> 8). Однак ніякої помилки тут немає. Лише вийшла надмірне вираження при підстановці певних аргументів макросу:

ADDXXREG(reg_ixh, reg_ixl, reg_ixh, reg_ixl, 15, 2);

Це помилкові спрацьовування, і ми повинні їх усунути. Подавим всі попередження, пов'язані з ними. Для цього в заголовочном файлі, де оголошено ці макроси, я додав наступні коментарі:
  • //-V:ADDXXREG:501
  • //-V:ADCHLREG:501
  • //-V:SBCHLREG:501
  • //-V:SBCHLREG:501
Подробиці про цей механізм придушення попереджень можна прочитати в відповідному розділі документації.

В принципі, можна обійтися одним коментарем. В іменах всіх макросів є «REG». Тому можна написати один коментар: //-V:REG:501. Він придушить попередження V501 у тих рядках, де буде зустрічатися «REG». Але це погано, так як випадково можна заховати корисне повідомлення, що не має відношення до перерахованих вище макросів. Трохи поліпшити ситуацію можна, додавши для пошуку дужку: //-V:REG(:501. В даному випадку я вважаю, що не варто лінуватися і слід написати 4 коментаря.

Помилка у параметрах функції sprintf()
sprintf( sText, "%s %s = %s\n"
, g_aTokens[ TOKEN_COMMENT_EOL ].sToken
, g_aParameters[ PARAM_CATEGORY ].m_sName
, g_aParameters[ eCategory ]
);

Попередження: V510 The 'sprintf' function is not expected to receive class-type variable as fifth actual argument. debug.cpp 2300

І дійсно, п'ятим фактичним аргументом є структура, що має тип Command_t. Очевидно, в якості аргументу слід використовувати g_aParameters[eCategory].m_sName. Вніс відповідну правку в код.

Погано пахне ZeroMemory()Наступне повідомлення говорить нам про те, що один масив заповнюється не повністю: V512 A call of the 'memset' function will lead to underflow of the buffer 'pHDD->hd_buf'. harddisk.cpp 491
BYTE hd_buf[HD_BLOCK_SIZE+1]; // Why +1?
ZeroMemory(pHDD->hd_buf, HD_BLOCK_SIZE);

Останній байт не обнуляється. Не можу сказати, помилка це чи ні. Зверніть увагу на коментар. Можливо, навіть самі розробники не знають, якого розміру має бути масив і чи обнуляти його цілком.

Про такому коді кажуть, що він із запахом. Він не обов'язково є помилковим, але підозрілий і може стати причиною помилки надалі.

Я просто подавлю це попередження з допомогою коментаря. Можна самостійно внести зміни у файл, а можна скористатися пунктом контекстного меню «Mark selected messages as False Alarms»:

Малюнок 3. Додавання в код коментаря для придушення попередження.
Малюнок 3. Додавання в код коментаря для придушення попередження.

Вибравши цей пункт, аналізатор сам вставить в код коментар:
ZeroMemory(pHDD->hd_buf, HD_BLOCK_SIZE); //-V512

Помилкове спрацьовування на виклику функції memcpy()
unsigned char random[ 256 + 4 ];
memcpy( &memmain[ iByte ], random, 256 );

Функція memcpy() копіює тільки частина буфера 'random'. Аналізатор вважає це підозрілим і чесно повідомляє про це. В даному випадку він неправий, і помилки немає. Я придушив попередження, як і в попередньому випадку, з допомогою коментаря. Це не дуже гарно, але як краще вчинити в чужому коді я не знаю.

Зайві дії
nAddress_ = 0;
nAddress_ = (unsigned)*(LPBYTE)(mem + nStack);
nStack++;
nAddress_ += ((unsigned)*(LPBYTE)(mem + nStack)) << 8;

Попередження: V519 The 'nAddress_' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 568, 569. debugger_assembler.cpp 569

Аналізатор звернув увагу, що змінної nAddress_ присвоюють різні значення кілька разів поспіль. Помилки тут ніякої немає, просто зайвий код. Я видалив першу сходинку, де змінної присвоюється значення 0. Ще один варіант позбутися попередження, це замінити друге призначення на "+=".

Аналогічну ситуацію можна спостерігати ще в двох файлах:

Файл video.cpp (див. рядок 3310 і 3315). Я видалив зайву операцію «pSrc += nLen;».

Файл Debug.cpp (див. рядок 5867 і 5868). Замінив:
char *p = sLine;
p = strstr( sLine, ":" );
на
char *p = strstr( sLine, ":" );

Детальніше на цих фрагментах зупинятися не буду.

Помилка в операторі switchНаступна діагностика V519 вже вказує на справжню серйозну помилку. Хоча класична помилка, і всі про неї знають, ми знову і знову зустрічаємо її в різних програмах.
switch( c )
{
case '\\':
eThis = PS_ESCAPE;
case '%':
eThis = PS_TYPE;
break;
default:
sText[ nLen++ ] = c;
break;
}

Попередження: V519 The 'p' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 5867, 5868. debug.cpp 5868

Після «eThis = PS_ESCAPE;» немає оператора 'break'. З-за цього значення змінної 'eThis' тут же зміниться на PS_STYPE. Це явна помилка. Щоб її виправити, я додав оператор 'break'.

Помилка: завжди помилкове умова
static inline ULONG ConvertZ80TStatesTo6502Cycles(UINT uTStates)
{
return (uTStates < 0) ?
0 : (ULONG) ((double)uTStates / uZ80ClockMultiplier);
}

Попередження: V547 Expression 'uTStates < 0' is always false. Unsigned type value is never < 0. z80.cpp 5507

Програміст хотів захиститися від випадку, коли в функцію буде передано від'ємне значення. Однак, оскільки змінна 'uTStates' є беззнаковой, то захист не спрацює.

Я додав явне приведення до типу 'INT':
return ((INT)uTStates < 0) ?
0 : (ULONG) ((double)uTStates / uZ80ClockMultiplier);

Надмірна настороженість аналізатораУ наступній функції аналізатор турбується, що може відбутися вихід за кордон масиву.
void SetCurrentImageDir(const char* pszImageDir)
{
strcpy(g_sCurrentDir, pszImageDir);
int nLen = strlen( g_sCurrentDir );
if( g_sCurrentDir[ nLen- 1 ] != '\\' )
....
}

Попередження: V557 Array underrun is possible. The value of 'nLen — 1' index could reach -1. applewin.cpp 553

Якщо у функцію передати порожній рядок, то її довжина дорівнюватиме нулю. Тоді відбудеться вихід за кордон масиву: g_sCurrentDir[ 0 — 1 ].

Аналізатор не знає, можлива така ситуація чи ні. Тому на всякий випадок і попереджає.

Я теж не знаю, чи можлива така ситуація чи ні. Якщо можлива, то аналізатор знайшов помилку. Якщо ні — це помилкове спрацьовування.

Я вирішив вважати, що це помилкове спрацьовування. Однак буде більш корисним поліпшити код, а не додавати коментар для придушення попередження. Тож нехай у функції буде додана додаткова перевірка:
if (nLen == 0)
return;

Є ще одне місце, де теоретично може відбутися вихід за кордон масиву. Але треба намагатися не перетворити статтю в товстий довідник. Тому описувати це попередження не буду і просто подавлю його з допомогою коментаря. См. цей же файл (рядок 556).

замість Присвоювання порівняння
if ((bytenum == 3) && (byteval[1] = 0xAA))
{

Попередження: V560 A part of conditional expression is always true: (byteval[1] = 0xAA). diskimagehelper.cpp 439

Я впевнений, що хотіли виконати операцію '==', а не '='. Якщо потрібно присвоювання, набагато природніше було б написати так:
if (bytenum == 3)
{
byteval[1] = 0xAA;

Тому це помилка, і її варто виправити:
if ((bytenum == 3) && (byteval[1] == 0xAA))

Помилкове спрацьовування через макросів
if ((TRACKS_MAX>TRACKS_STANDARD) &&....)

Попередження: V560 A part of conditional expression is always true: ((35 + 5) > 35). diskimagehelper.cpp 548

Якщо розкрити макроси, то вийде вираз(35 + 5) > 35). Вираз завжди істинно, але це не помилка.

Цей той випадок, коли я навіть не знаю, як краще вчинити. Схалтурю і просто подавлю помилкове спрацьовування за допомогою коментаря: //-V560.

Зайва мінливаУ процесі рефакторінгу іноді залишаються «загублені» змінні. Вони використовуються в коді, але насправді більше не потрібні. Мабуть саме так сталося зі змінною bForeground:
BOOL bForeground;
....
bForeground = FALSE;
....
if( bForeground )
dwCoopFlags |= DISCL_FOREGROUND;
else
dwCoopFlags |= DISCL_BACKGROUND;
....
if( hr == DIERR_UNSUPPORTED && !bForeground && bExclusive )

І більше змінна 'bForeground' ніде не змінюється і не використовується. Це призводить до виникнення попередження: V560 A part of conditional expression is always true: !bForeground. mouseinterface.cpp 690

Це цікавий приклад для філософії. Помилкове це спрацьовування чи ні? Навіть людині складно дати відповідь. Аналізатор прав, виявивши аномалію. Але з точки зору людини цей фрагмент може бути недописаним кодом. І тоді все в порядку.

Будемо вважати, що це ще один приклад «коду з запахом». Я видалив змінну 'bForeground' коду.

Невизначено поведінка
*(mem+addr++) =
(opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;

Попередження: V567 Undefined behavior. The 'addr' variable is modified while being used twice between sequence points. cpu.cpp 564

Невідомо, як буде обчислюватись вираз:
  • Можливо, спочатку збільшиться змінна 'addr', а потім вона буде використана в правій частині виразу.
  • А можливо навпаки.
Правильним буде такий код:
*(mem+addr) =
(opcode >= BENCHOPCODES) ? 0x00 : ((addr >> 4)+1) << 4;
addr++;

Неправильні аргументи при виклику функції wsprintf() та аналогічних їйЄ кілька помилок, пов'язаних з передачею неправильного кількості фактичних аргументів функції форматованого виводу. Всього таких помилок було знайдено 10, але ми розглянемо тільки одну:
wsprintf( sText, TEXT("%s full speed Break on Opcode: None")
, sAction
, g_iDebugBreakOnOpcode
, g_aOpcodes65C02[ g_iDebugBreakOnOpcode ].sMnemonic
);

Попередження: V576 Incorrect format. A different number of actual arguments is expected while calling 'wsprintfA' function. Expected: 3. Present: 5. debug.cpp 939

При формуванні рядка два останніх параметра не враховуються. Мені, як сторонній людині, складно сказати, чи є ці параметри зайвими, або помилку допущено в рядку форматування.

Я вирішив, що параметри зайві, і видалив їх.

Аналогічні проблеми спостерігається у наступних ділянках коду:
  • Expected: 8. Present: 9. debug.cpp 7377
  • Expected: 3. Present: 4. debugger_help.cpp 1263
  • Expected: 3. Present: 4. debugger_help.cpp 1265
  • Expected: 3. Present: 4. debugger_help.cpp 1267
  • Expected: 3. Present: 4. debugger_help.cpp 1282
  • Expected: 3. Present: 4. debugger_help.cpp 1286
  • Expected: 3. Present: 4. debugger_help.cpp 1288
  • Expected: 5. Present: 4. debugger_help.cpp 1332
  • Expected: 3. Present: 4. frame.cpp 691
  • Expected: 3. Present: 4. frame.cpp 695
Ще є пара місць, де для роздруківки значень покажчика використовується "%08X". На 32-бітної системі це на практиці працює. Але в 64-бітній системі буде роздрукована тільки частина покажчика. Правильним рішенням є використання "%p". Відповідні ділянки коду:
  • To print the value of pointer the '%p' should be used. tfe.cpp 507
  • To print the value of pointer the '%p' should be used. tfe.cpp 507
Помилкові спрацьовування на подвійних порівнянняхХоча аналізатор не винен, він видав два помилкові спрацьовування на повторюваних умовах. Розглянемо один випадок:
if (nAddress <= _6502_STACK_END)
{
sprintf( sText,"%04X: ", nAddress );
PrintTextCursorX( sText, rect );
}

if (nAddress <= _6502_STACK_END)
{
DebuggerSetColorFG( DebuggerGetColor( FG_INFO_OPCODE ));
sprintf(sText, " %02X",(unsigned)*(LPBYTE)(mem+nAddress));
PrintTextCursorX( sText, rect );
}

Попередження: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 2929, 2935. debugger_display.cpp 2935

Помилки немає. Програміст просто розділив два набору дій. З точки зору аналізатора такий код підозрілий. Раптом умови повинні відрізнятися? Тим не менше з помилковим спрацьовуванням треба щось робити. Я вирішив об'єднати два умовних оператора в один:
if (nAddress <= _6502_STACK_END)
{
sprintf( sText,"%04X: ", nAddress );
PrintTextCursorX( sText, rect );

DebuggerSetColorFG( DebuggerGetColor( FG_INFO_OPCODE ));
sprintf(sText, " %02X",(unsigned)*(LPBYTE)(mem+nAddress));
PrintTextCursorX( sText, rect );
}

Думаю, читаність коду від цього не постраждала, але попутно ми позбавилися від помилкового спрацьовування.

Другий випадок аналогічний: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 2237, 2245. debugger_display.cpp 2245

<img src=«habrastorage.org/getpro/habr/post_images/ab2/611/800/ab26118001faa212e23f96ac5f7c5407.png» alt=«Picture » 12"/>
Малюнок 5. В середині довгої статті рекомендують вставляти картинку, щоб читач відпочив. Я не знаю, яку відноситься до статті картинку вставити. Тому ось вам котик.

Розіменування вказівника до перевіркиВсього аналізатор видав 3 попередження на цю тему. На жаль, текст програми в цих місцях досить заплутаний. Тому для спрощення я наведу не справжній код, а псевдокод. Для перших 2 попереджень код виглядає приблизно так:
int ZEXPORT unzGetGlobalComment(char *szComment)
{
....
if (A)
{
*szComment='\0';
return UNZ_ERRNO;
}
....
if ((szComment != NULL) && X)
....
}

Попередження: V595 The 'szComment' pointer was utilized before it was verified against nullptr. Check lines: 1553, 1558. unzip.c 1553

Як бачите, переданий покажчик 'szComment' може бути дорівнює NULL. Про це свідчить перевірка (szComment != NULL).

Однак є ділянка коду, в якому покажчик сміливо разыменовывается без виконання перевірки. Це небезпечно. Можливо, що на практиці ніколи не виникає ситуацій, коли 'szComment' дорівнює 0. Але код небезпечний, і його слід виправити.

Аналогічно: V595 The 'pToken_' pointer was utilized before it was verified against nullptr. Check lines: 811, 823. debugger_parser.cpp 811

А ось з останнім, третім випадком, все складніше. Я вже втомився доводити, що подібний код невірний і його варто виправити. Функція коротка, тому я наведу її повністю
bool ArgsGetValue ( Arg_t *pArg,
WORD * pAddressValue_, const int nBase )
{
TCHAR *pSrc = & (pArg->sArg[ 0 ]);
TCHAR *pEnd = NULL;

if (pArg && pAddressValue_)
{
*pAddressValue_ =
(WORD)(_tcstoul( pSrc, &pEnd, nBase) & _6502_MEM_END);
return true;
}
return false;
}

Попередження: V595 The 'pArg' pointer was utilized before it was verified against nullptr. Check lines: 204, 207. debugger_parser.cpp 204

Покажчик 'pArg' може дорівнювати нулю, про що свідчить умова «if (pArg && pAddressValue_)». Але до того, як вказівник буде перевірений, він використовується у вираженні:
TCHAR *pSrc = & (pArg->sArg[ 0 ]);

Це вираз призводить до невизначеної поведінки програми. Не можна розв'язати нульові покажчики.

Багато заперечують, що в такому коді не відбувається звертання до пам'яті, а просто обчислюється якийсь адресу. Отже, ніякої проблеми немає. Проте це занадто вузьке тлумачення невизначеного поведінки. Взагалі не треба ворожити, як буде вести себе компілятор і як код буде працювати. Так не можна писати, і немає сенсу обговорювати, чому саме.

Невизначений поведінку в такому коді — це не тільки звернення до нульового адресою пам'яті (якого може і не бути). Наприклад, компілятор цілком має право скоротити умова перевірки до «if (pAddressValue_)». Раз є вираз «pArg->xxx», значить покажчик точно не нульовий і його не треба перевіряти.

Більш детально обговорювати це питання немає сенсу. Я пропоную познайомитися зі спеціальною статтею: Разыменовывание нульового покажчика призводить до невизначеної поведінки.

Виправити код просто. Досить перенести оголошення змінної всередину 'if'.

Лякаюче виразАналізатор збентежило наступне вираз:
if ((cx > 4) & (cx <= 13))

Попередження: V602 Consider inspecting the '(cx > 4)' expression. '>' possibly should be replaced with '>>'. debug.cpp 8933

Аналізатор бачить, що використовується оператор '&', операндами якого є застосовуваний типу 'bool'. Це дивно. Зазвичай в таких випадках прийнято використовувати логічний оператор '&&'.

Оператор '&' прийнято використовувати для бітових операцій. Тому аналізатор припустив можливість, що і тут хотіли працювати з бітами:
if ((cx >> 4) & (cx <= 13))

Аналізатор тут перемудрував і неправий. Однак вина програміста тут теж є. Цей код з запахом. Набагато природніше буде написати:
if (cx > 4 && cx <= 13)

Неуточняемое поведінку і жах в макросахНезрозуміло, який результат вийде при зсуві вправо від'ємних значень. Краще так не робити, так як на іншому компіляторі поведінка коду може змінитися.
const short SPKR_DATA_INIT = (short)0x8000;
if (g_nSpeakerData == (SPKR_DATA_INIT >> 2))

Попередження: V610 Unspecified behavior. Check the shift operator '>>'. The left operand 'SPKR_DATA_INIT' is negative. speaker.cpp 450

Вихід: можна оголосити константу SPKR_DATA_INIT беззанковой. Правда тоді знадобиться зробити ще кілька дрібних виправлень, щоб не з'явилися попередження компілятора і аналізатора про порівняння знакових/беззнакових чисел.

Аналізатор знайшов ще 3 таких небезпечних місця:
  • The left operand 'SPKR_DATA_INIT' is negative. speaker.cpp 453
  • The left operand '~0x180' is negative. tfe.cpp 869
  • The left operand '~0x100' is negative. tfe.cpp 987
До речі, коли я почав правити два останніх попередження, то знайшов ще 2 помилки. Можна сказати, що аналізатор допоміг знайти їх непрямим шляхом.

Ось як макрос використовується:
SET_PP_16(TFE_PP_ADDR_SE_BUSST, busst & ~0x180);

Він розкривається в довгу рядок. Тому я покажу тільки її частину:
..... = (busst & ~0x180 >> 8) & 0xFF;.....

Пріоритет оператора зсуву >> більше, ніж пріоритет оператора &. См. таблицю: пріоритети операцій.

Програміст очікував, що код буде працювати так:
..... = ((busst & ~0x180) >> 8) & 0xFF;.....

А на саме справі він працює так:
..... = (busst & (~0x180 >> 8)) & 0xFF;.....

Ось тому аналізатор PVS-Studio і попереджає: «the left operand '~0x180' is negative».

Ось як небезпечно використовувати макроси!

Дірки в безопаснотиУ проекті дуже небезпечним чином використовуються функції sprintf(), wsprintf() і так далі. Якщо зовсім коротко, то функції використовуються наступним чином:
sprintf(buf, STR);

Якщо рядок STR буде містити керуючі символи, такі як "%s", то наслідки будуть непередбачувані.

Зазвичай такий код розглядається як діра в безпеці (див. подробиці).

Однак для емулятора, думаю, це не критично. Ніхто не буде його атакувати. Однак такий код небезпечний сам по собі. Він може легко призвести до падіння програми або до неправильної її роботі.

Правильно робити так: sprintf(buf, "%s", STR);

Аналізатор знайшов досить багато небезпечних викликів функцій. Всього він видав 21 попередження.

Протилежні умови
// TO DO: Need way of determining if DirectX init failed
if (soundtype != SOUND_WAVE)
{
if (soundtype == SOUND_WAVE)
soundtype = SOUND_SMART;

Попередження: V637 Two opposite conditions were encountered. The second condition is always false. Check lines: 270, 272. speaker.cpp 270

Судячи з коментаря код не дописаний. Складно сказати, як поступати з таким кодом. Я вирішив закоментувати другий безглуздий 'if':
if (soundtype != SOUND_WAVE)
{
//if (soundtype == SOUND_WAVE)
// soundtype = SOUND_SMART;

Невдале вирівнювання кодуКод виглядає так, як ніби обидва дії відносяться до оператора 'if':
{
if ((Slot4 == CT_MockingboardC) || (Slot4 == CT_Phasor))
m_PropertySheetHelper.GetConfigNew().m_Slot[4] = CT_Empty;
m_PropertySheetHelper.GetConfigNew().m_Slot[5] = CT_SAM;
}

Попередження: V640 The code's operational logic does not correspond with its formatting. The statement is indented to the right, but it is always executed. It is possible that curly brackets are missing. pagesound.cpp 229

Як я зрозумів, помилки в коді немає. Однак це помилкове спрацьовування. Аналізатор абсолютно прав, попереджаючи про такому коді. Слід обов'язково поправити вирівнювання:
{
if ((Slot4 == CT_MockingboardC) || (Slot4 == CT_Phasor))
m_PropertySheetHelper.GetConfigNew().m_Slot[4] = CT_Empty;
m_PropertySheetHelper.GetConfigNew().m_Slot[5] = CT_SAM;
}

Неправильна робота з функцією strncat()
strncat( sText, CHC_DEFAULT, CONSOLE_WIDTH );
strncat( sText, pHelp , CONSOLE_WIDTH );

Попередження: V645 The 'strncat' function call could lead to the 'sText' buffer overflow. The bounds should not contain the size of the buffer, but a number of characters it can hold. debugger_help.cpp 753

Третій аргумент функції — кіль символів, які ще можна додати до рядку. Тому правильно і безпечно зробити так:
strncat( sText, CHC_DEFAULT, sizeof(sText) - strlen(sText) - 1);
strncat( sText, pHelp , sizeof(sText) - strlen(sText) - 1);

Подробиці можна дізнатися, ознайомившись з описом діагностики V645.

Зайві перевіркиВже дуже давно оператор 'new' генерує виняток std::bad_alloc, якщо не може виділити пам'ять. Однак у програмах можна зустріти непотрібні перевірки:
BYTE* pNewImageBuffer = new BYTE [uNewImageSize];
_ASSERT(pNewImageBuffer);
if (!pNewImageBuffer)
return false;

Попередження: V668 There is no sense testing in the 'pNewImageBuffer' pointer against null, as the memory was allocated using the 'new' operator. The exception will be generated in the case of memory allocation error. diskimagehelper.cpp 197

_ASSERT і перевірку можна і потрібно прибрати. Тут у них немає ніякого сенсу.

Аналогічна ситуація:
  • mouseinterface.cpp 175
  • serialcomms.cpp 839
  • savestate.cpp 108
  • savestate.cpp 218
  • speech.cpp 40
Самостійне оголошення системних типівУ проекті заможнє визначаються деякі типи даних:
typedef unsigned long ULONG;
typedef void *LPVOID;
typedef unsigned int UINT;

Явної помилки тут немає. Будемо вважати що це «код за запахом» і подавим попередження з допомогою коментаря //-V677.

Порушений «Закон Великої Двійки»Наприклад, у класі CConfigNeedingRestart оголошено operator =, але немає конструктора копіювання. Це порушує "Закон Великої Двійки".

Клас досить великий, тому не буду тут наводити фрагменти коду. Повірте на слово.

В цьому класі всі поля є простими типами, тому свій власний operator = взагалі не потрібен. Клас буде успішно копіюватися автоматично.

З класом Disk_t ситуація аналогічна. І там і там можна видалити operator =.

Попередження аналізатора:
  • V690 The 'CConfigNeedingRestart' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. config.h 7
  • V690 The 'Disk_t' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. disk.cpp 74
Помилка
int nHeight=nHeight=g_aFontConfig[ FONT_CONSOLE ]._nFontHeight; 

Попередження: V700 Consider inspecting the 'foo = foo = ...' expression. It is odd that variable is initialized through itself. debugger_display.cpp 1226

Просто помилка. Замінив на:
int nHeight = g_aFontConfig[ FONT_CONSOLE ]._nFontHeight;

Зайве занепокоєння аналізатора з приводу перерахуваньУ перерахуванні 'AppMode_e' є такі іменовані константи: MODE_LOGO, MODE_PAUSED, MODE_RUNNING, MODE_DEBUG, MODE_STEPPING.

Аналізатор переживає, що не всі вони використовуються у цьому switch():
switch (g_nAppMode)
{
case MODE_PAUSED : _tcscat (.....); break;
case MODE_STEPPING: _tcscat (.....); break;
}

Попередження: V719 The switch statement does not cover all values of the 'AppMode_e' enum: MODE_DEBUG, MODE_LOGO, MODE_RUNNING. frame.cpp 217

У цьому прикладі мені за аналізатор навіть трохи соромно. Емпіричні алгоритми підвели. Помилкове спрацьовування. Є кілька способів його усунути. Наприклад, можна додати гілку «default».
switch (g_nAppMode)
{
case MODE_PAUSED : _tcscat (.....); break;
case MODE_STEPPING: _tcscat (.....); break;
default: break;
}

Ще одне аналогічне помилкове спрацьовування: V719 The switch statement does not cover all values of the 'AppMode_e' enum: MODE_DEBUG, MODE_LOGO. frame.cpp 1210

Я обіцяв побіжно поглянути на попередження третього рівняМи не рекомендуємо (принаймні на перших етапах) взагалі заглядати на 3 рівень. Там багато помилкових, нецікавих або специфічних повідомлень. Зараз саме така ситуація.

Наприклад, тут досить багато попереджень V601.
inline int IsDebugBreakpointHit()
{
if ( !g_bDebugNormalSpeedBreakpoints )
return false;
return _IsDebugBreakpointHit();
}

Попередження: V601 The 'false' value is implicitly cast to the integer type. debug.h 210

Функція повертає тип 'int'. Але при цьому написано «return false».

Аналізатор прав, що прискіпується до цього коду, але на практиці за цим дуже рідко ховається справжня помилка. Тому це попередження і потрапило на 3 рівень.

Тепер приклад специфічної діагностики:
double g_fClksPerSpkrSample;
....
if ((double)g_nRemainderBufferSize != g_fClksPerSpkrSample)

Попередження: V550 An odd precise comparison. It's probably better to use a comparison with defined precision: fabs(A — B) > Epsilon. speaker.cpp 197

Правильний цей код чи ні — дуже сильно залежить від програми і від того, які значення поміщаються в змінні типу 'double'.

Ряд користувачів в захваті від цієї діагностики. Інші кажуть, що double вони кладуть цілі числа і знають, що роблять, коли їх порівнюють. Всім не догодиш.

Запуск після правок помилокТепер, поправивши все повідомлення (1 і 2 рівень), ми можемо перезапустити аналізатор. Результат очікуємо — всі попередження зникли (див. малюнок 6).

Малюнок 6. Більше немає попереджень 1 і 2 рівня.
Малюнок 6. Більше немає попереджень 1 і 2 рівня.

Цей ідеальний варіант, застосовний тільки до маленьких проектів. Тим не менш, я сподіваюся, що мені вдалося показати, що немає нічого складного в роботі з повідомленнями аналізатора. Хоча частина повідомлень були помилковими, з ними не виникло труднощів, і всі їх вдалося усунути.

Підведемо підсумкиНас часто запитують, яка кількість помилкових спрацьовувань видає наш аналізатор. У нас немає відповіді. Зібрати таку статистику дуже складно, і вона мало що дає. Кількість помилкових спрацьовувань дуже сильно змінюється від проекту до проекту.

Ще є проблема з интерпретированием даних. Наприклад, з-за невдалого макросу, який активно використовується в усьому проекті, кількість помилкових спрацьовувань може бути в 20 разів більше, ніж корисних повідомлень. Але це не біда. Досить придушити попередження в цьому макросі, і кількість помилкових повідомлень може відразу зменшитися на 90%.

Наступна складність дати відповідь полягає в тому, що програмісти не враховують, що деякі попередження складно класифікувати. Ці повідомлення не виявляють помилки, але виявляють «код за запахом». Такий код варто поправити, так як хоча зараз він працює коректно, надалі він може стати причиною дефекту. У статті було показано кілька прикладів таких діагностик.

Але програмісти тяжіють до двійкової логіки. І наполягають отримати відповідь: «Це помилкове повідомлення? Так/Ні?». Якщо ви уважно прочитали статтю, я сподіваюся, так суворо питання ви ставити не будіть.

Як бачите, говорити узагальнено про кількість помилкових спрацьовувань складно. Але якщо взяти конкретний маленький проект, то дати відповідь саме для нього можна.

Підсумки по діагностичних повідомлень, виданими аналізатором PVS-Studio для проекту Apple II emulator for Windows:
  • Всього видано повідомлень (General Analysis, перший і другий рівень): 81
  • Знайдено помилок: 57
  • Знайдено фрагментів «коду з запахом», які слід виправити: 9
  • Помилкові спрацьовування: 15
Підсумуємо у відсотках:
  • Повідомлення, що вказують на помилки: 70 %
  • Повідомлення, що вказують на «код за запахом»: — 11 %
  • Помилкові спрацьовування: 19 %
ВисновокСпробуйте статичний аналізатор PVS-Studio на своєму проекті. Ви можете завантажити демонстраційну версію за адресою: http://www.viva64.com/ru/pvs-studio-download/

І пропоную порекомендувати спробувати наш статичний аналізатор своїм колегам і друзям. Прошу написати повідомлення в twitter або будь-яку іншу стрічку. Спасибі.

P. S. Ви можете слідкувати за нашими новими статтями і взагалі дізнаватися новини з світу Сі/Сі++, якщо підпишіться на мій twitter: https://twitter.com/Code_Analysis

Спасибі всім за увагу.


Якщо хочете поділитися цією статтею з англомовної аудиторією, то прошу використовувати посилання на переклад: Andrey karpov. An Ideal Way to Integrate a Static Code Analyzer into a Project.

Прочитали статтю і є питання?Часто до наших статей задають одні і ті ж питання. Відповіді на них ми зібрали тут: Відповіді на питання читачів статей про PVS-Studio, версія 2015. Будь ласка, ознайомтеся зі списком.


Джерело: Хабрахабр

0 коментарів

Тільки зареєстровані та авторизовані користувачі можуть залишати коментарі.