Один день из поддержки пользователей PVS-Studio

    Picture 1

    Мы рады любым тематическим общениям на тему качества кода. Нам пишут клиенты, студенты и любые другие пользователи со всех уголков интернета. Независимо от страны, часового пояса или языка. Языка разговорного, конечно. Среди языков программирования нам всё же интересен ограниченный набор. Сейчас это C, C++, C# и Java. Выгоды от общений очень много. Некоторые предложения пользователей мы реализуем сразу, т.к. они действительно полезные. Часто мы просто выручаем чей-то проект, объясняя предупреждения анализатора, которые в итоге оказываются ошибкой. Эта заметка об одном таком случае.

    Об анализаторе


    PVS-Studio — это инструмент для выявления ошибок и потенциальных уязвимостей в исходном коде программ, написанных на языках С, C++, C# и Java. Работает в среде Windows, Linux и macOS.

    Для связи существует 3 формы обратной связи:

    1. Обратная связь
    2. Запрос триала
    3. Запрос цен

    Вечер четверга


    Один активный пользователь, который попробовал анализатор на своём коде, активно начал присылать ложные предупреждения. Прежде чем я успел ответить, накопилось 3 письма. Это был конец рабочего дня и сил уже оставалось мало (к вопросу о надёжности ручного ревью кода). Наша команда активно готовилась к большому релизу, до которого оставалось несколько дней.

    Я решил отложить ответ на пятницу или даже на следующую неделю:

    Здравствуйте, Константин.

    Разбираем предупреждения. На следующей неделе прокомментирую подозрительные места :-)

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

    С разрешения пользователя расскажу о состоявшейся переписке:

    Письмо 1

    Ложно-позитивные срабатывания V712:

    uint32_t StartUpCounter = 0, HSEStatus = 0;
    RCC->CR |= ((uint32_t)RCC_CR_HSEON);
    /* Wait till HSE is ready and if Time out is reached exit */
    {
      HSEStatus = RCC->CR & RCC_CR_HSERDY;
      StartUpCounter++;
    } while((HSEStatus == 0) && (StartUpCounter != HSE_STARTUP_TIMEOUT)); // V712...

    Письмо 2

    Там же ложное срабатывание V715:

    { // V715 ... lpmode.cpp 356
      HSEStatus = RCC->CR & RCC_CR_HSERDY;
      StartUpCounter++;
    } while((HSEStatus == 0) && (StartUpCounter != HSE_STARTUP_TIMEOUT));

    Письмо 3

    Ёлки-палки, заколдованное место! Всё там же, ругается (см. код от предыдущих писем):

    V560 A part of conditional expression is always true: (StartUpCounter != ((uint16_t) 0x5000)). lpmode.cpp 356

    V776 Potentially infinite loop. The variable in the loop exit condition 'HSEStatus == 0' does not change its value between iterations. lpmode.cpp 356

    Может я чего-то не понимаю? Но на практике всё работает, и если кварц не запускается, то мы выходим из этого участка по тайм-ауту ;-)

    Письмо 4 (ответ)

    Здравствуйте, Константин.

    Разбираем предупреждения. На следующей неделе прокомментирую подозрительные места :-)

    Письмо 5

    Чёрт! Я только после вашего письма код увидел боковым зрением — оператор do пропущен… Всё встало на свои места! Совсем глаз замылился %)

    do {...} while (...);

    Заключение


    Как вы могли заметить, было 4 предупреждения анализатора на одно и то же место, но всё равно понадобилось время, чтобы убедить пользователя в наличии ошибки. В такой ситуации у ручного ревью даже шансов не было бы.

    Похожая история со счастливым концом: "Как PVS-Studio оказался внимательнее, чем три с половиной программиста".

    Используйте статические анализаторы в своём проекте. Они не являются заменой ревью кода с коллегой, а являются полезным дополнением к контролю качества кода.



    Если хотите поделиться этой статьей с англоязычной аудиторией, то прошу использовать ссылку на перевод: Svyatoslav Razmyslov. One Day from PVS-Studio User Support.
    PVS-Studio
    412,65
    Static Code Analysis for C, C++, C# and Java
    Поделиться публикацией

    Комментарии 15

      +4
      Чертов C++, позволяет дичь творить! :)
      Вот ввели бы более строгие ограничения в язык и такого не было бы.
        +3

        Согласен, операторы цикла без тела цикла. Экономия на {}, зачем? Так же про if. Хотя по хорошему такие места должны ещё на этапе линтера отваливаться по стилю кода типа clang-format, а не вот это condition never reached и другие навороты pvs.

          0
          Многие команды почему-то стремятся избегать лишних {}. Например, когда я переписывал во FreeBSD mount() через их новый ядерный интерфейс, меня ревьюеры постоянно шпыняли за привычку оборачивать в {} одиночные строчки после if, while и так далее — там почему-то такое не принято.
          0
          Платим за гибкость
            +1
            Ну и какая в данном случае гибкость?
            Сэкономить несколько строчек вынеся чего-то в Condition?

            Или вот никогда не понимал, зачем мне присвоение в if? Опять какая-то экономия на спичках во вред читаемости.
          +2
          ну, этот пример немного не то показывает.

          Что do нет там сразу видно. И ревьюер бы спросил «а где do?».
          А анализатор выдал три разные ошибки, которые по сути свелись к «что-то здесь не так».
          И на понимание ошибки ушло, как там, пять писем и два дня?

          Понятно, что всё равно лучше с анализатором, чем без. И что мысли он читать не может. Но пример неудачный.
            +1

            Насколько я понимаю, это иллюстрация того, что вероятность ошибки в коде гораздо выше вероятности ложного срабатывания анализатора.
            И первым делом надо внимательно код почитать, в препроцессорный вывод посмотреть, а уже потом писать в техподдержку о ложном срабатывании.

              0
              Очень удачный пример того, что ручное вревью кода уступает автоматическому.

              Примеры ошибок в нашем блоге создают ложное ощущение, что «там сразу видно», но это не так.
                +3
                Я по себе сужу, сразу заметил, что do нет. Было раньше много примеров, где совсем непонятно без PVS-Studio.

                А этот пример показывает, что анализатор выдал неправильное предупреждение. Формально правильное, «вы находитесь на воздушном шаре», но не очень полезное.
              +4

              Честно говоря, ожидал увидеть триллер на десяток мониторов вниз :)

                +2
                Но на практике всё работает, и если кварц не запускается, то мы выходим из этого участка по тайм-ауту ;-)
                Настоящий триллер мог бы быть потом, когда надо было бы отлаживать баг. А реальное место ошибки было изначально исключено из рассмотрения)
                0
                У меня несколько необычный вопрос.
                Есть ли какие то перспективы и возможности «прицепить» PVS-Studio
                к проверке кода сделанного на Tcl/Tk?
                  0
                  Не получится. Но я знаю умельцев, которые сделали транслятор PHP в C++ для выявления рада ошибок, и мы смогли проверить такой код и найти реальные ошибки в PHP. Можете пойит по такому же пути, если качество кода критично, а инструментов нет.
                    0
                    Вы говорите о ВКонтакте с их kPHP?

              Только полноправные пользователи могут оставлять комментарии. Войдите, пожалуйста.

              Самое читаемое