Андрей Карпов

27 Янв 2018

Теги:

#Cpp #Knowledge

break и fallthrough

27 Янв 2018

Автор: Андрей Карпов

Предлагаем вашему вниманию цикл статей, посвященных рекомендациям по написанию качественного кода на примере ошибок, найденных в проекте Chromium. Это вторая часть, которая будет посвящена оператору switch, а, вернее, проблеме забытого оператора break.

Многие годы я изучал ошибки в программах и сейчас могу с уверенностью заявить, что в C, а вслед за ним и в C++, оператор switch сделан неправильно. Я понимаю, что возможность не писать break, сделанная, чтобы передать управление дальше, позволяет писать изящные алгоритмы. Но всё равно огромное количество ошибок убедило меня, что был выбран неправильный подход. Понятно, что теперь уже поздно. Просто хотелось сказать, что правильным решением было бы обязательно писать слово break или обратное ключевое слово, например, fallthrough. Сколько бы сил, времени и денег было сэкономлено. Конечно, этот недостаток не сравнится с Null References: The Billion Dollar Mistake, но всё равно большой ляп.

Ладно, пофилософствовали и хватит. Язык C++ такой, какой есть. Однако, это не значит, что можно расслабиться и ничего не предпринимать для повышения качества и надёжности кода. Проблема "пропущенного break" - это большая проблема, и её нельзя недооценивать. Даже в высококачественном проекте Chromium прячутся ошибки этого типа.

Давайте посмотрим, что я заметил в процессе разбора отчета, выданного PVS-Studio. Как я писал в вводной статье, отчёт я смотрел достаточно бегло, поэтому могут быть и другие, незамеченные мной ошибки. Тем не менее, найденных ошибок мне будет достаточно, чтобы продемонстрировать, что всё это не отдельные случайные ляпы, а устойчивый паттерн ошибки. Читателям следует отнестись к этому паттерну серьезно и постараться использовать меры по его предотвращению.

Первая ошибка взята непосредственно из кода проекта Chromium.

int GetFieldTypeGroupMetric(....) {
  ....
  switch (AutofillType(field_type).group()) {
    ....
    case ADDRESS_HOME_LINE3:
      group = GROUP_ADDRESS_LINE_3;
      break;
    case ADDRESS_HOME_STREET_ADDRESS:
      group = GROUP_STREET_ADDRESS;
    case ADDRESS_HOME_CITY:
      group = GROUP_ADDRESS_CITY;
      break;
    case ADDRESS_HOME_STATE:
      group = GROUP_ADDRESS_STATE;
      break;
    ....
}

Независимо от того, нужно ли автоматически заполнить некое поле "Street Address", или поле "Сity", в любом случае будет выбрана константа GROUP_ADDRESS_CITY. Т.е. где-то вместо названия улицы автоматически будет подставляться название города.

Причина в том, что пропущен оператор break. В результате, после присваивания:

group = GROUP_STREET_ADDRESS;

Переменной group тут же будет присвоено новое значение:

group = GROUP_ADDRESS_CITY;

Анализатор PVS-Studio замечает это двойное присваивание и выдаёт предупреждение: V519 The 'group' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 145, 147. autofill_metrics.cc 147

Вторая ошибка также относится к коду Chromium и выглядит схожим образом.

void GLES2Util::GetColorFormatComponentSizes(...., int* a) {
  ....
  // Sized formats.
  switch (internal_format) {
    case GL_ALPHA8_EXT:
      *a = 8;
    case GL_ALPHA16F_EXT:
      *a = 16;
    case GL_ALPHA32F_EXT:
      *a = 32;
    case GL_RGB8_OES:
    case GL_SRGB8:
    case GL_RGB8_SNORM:
    case GL_RGB8UI:
    case GL_RGB8I:
      *r = 8;
      *g = 8;
      *b = 8;
      break;
    case GL_RGB565:
  ....
}

Здесь забыто 2 или 3 оператора break. Я не знаю, как точно должен работать этот код, поэтому воздержусь от комментариев по поводу того, как следует исправить ошибку. Анализатор PVS-Studio выдаёт для этого кода два предупреждения:

V519 CWE-563 The '* a' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1385, 1387. gles2_cmd_utils.cc 1387
V519 CWE-563 The '* a' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 1387, 1389. gles2_cmd_utils.cc 1389

Третья ошибка из кода Chromium.

gfx::ColorSpace VideoColorSpace::ToGfxColorSpace() const {
  ....
  switch (primaries) {
  ....
  case PrimaryID::SMPTEST431_2:
    primary_id = gfx::ColorSpace::PrimaryID::SMPTEST431_2;
    break;
  case PrimaryID::SMPTEST432_1:
    primary_id = gfx::ColorSpace::PrimaryID::SMPTEST432_1;
  case PrimaryID::EBU_3213_E:
    primary_id = gfx::ColorSpace::PrimaryID::INVALID;
    break;
  }
  ....
}

Точно та же картина, что и раньше. Предупреждение PVS-Studio: V519 CWE-563 The 'primary_id' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 106, 109. video_color_space.cc 109

Четвёртая ошибка из кода Chromium. В этот раз нам уже поможет предупреждение V796, а не V519. Диагностика V519 выявляет пропущенный break косвенным образом, когда замечает повторное присваивание. Диагностика V796 разработана специально для поиска пропущенных операторов break.

void RecordContextLost(ContextType type,
                       CommandBufferContextLostReason reason) {
  switch (type) {
    ....
    case MEDIA_CONTEXT:
      UMA_HISTOGRAM_ENUMERATION("GPU.ContextLost.Media",
        reason, CONTEXT_LOST_REASON_MAX_ENUM);
      break;
    case MUS_CLIENT_CONTEXT:
      UMA_HISTOGRAM_ENUMERATION("GPU.ContextLost.MusClient",
        reason, CONTEXT_LOST_REASON_MAX_ENUM);
      break;
    case UI_COMPOSITOR_CONTEXT:
      UMA_HISTOGRAM_ENUMERATION("GPU.ContextLost.UICompositor",
        reason, CONTEXT_LOST_REASON_MAX_ENUM);
    case CONTEXT_TYPE_UNKNOWN:
      UMA_HISTOGRAM_ENUMERATION("GPU.ContextLost.Unknown",
        reason, CONTEXT_LOST_REASON_MAX_ENUM);
      break;
  }
}

После выполнения ветки "UI_COMPOSITOR_CONTEXT" управление передаётся в ветку "CONTEXT_TYPE_UNKNOWN". По всей видимости, это приводит где-то к неверной обработке... А вот не знаю я, на что это повлияет. Но break здесь пропущен, по всей видимости, случайно, а не намеренно.

Предупреждение PVS-Studio: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. command_buffer_metrics.cc 125

Пятая ошибка в коде Chromium, из-за которой страдает средняя клавиша мыши.

void SystemInputInjectorMus::InjectMouseButton(
  ui::EventFlags button, bool down)
{
  ....
  int modifier = ui::MODIFIER_NONE;
  switch (button) {
    case ui::EF_LEFT_MOUSE_BUTTON:
      modifier = ui::MODIFIER_LEFT_MOUSE_BUTTON;
      break;
    case ui::EF_RIGHT_MOUSE_BUTTON:
      modifier = ui::MODIFIER_RIGHT_MOUSE_BUTTON;
      break;
    case ui::EF_MIDDLE_MOUSE_BUTTON:
      modifier = ui::MODIFIER_MIDDLE_MOUSE_BUTTON;
    default:
      LOG(WARNING) << "Invalid flag: " << button
                   << " for the button parameter";
      return;
  }
  ....
}

Неправильно обрабатывается нажатие на среднюю клавишу мыши. После корректного действия:

modifier = ui::MODIFIER_MIDDLE_MOUSE_BUTTON;

Происходит переход к обработчику ошибочных флагов, и функция досрочно завершает свою работу.

Предупреждение PVS-Studio: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. system_input_injector_mus.cc 78

Здесь читатель вправе сказать: "Всё понятно, достаточно!". Однако, мне попалась на глаза ещё парочка таких ошибок в используемых библиотеках, так что давайте посмотрим и на них. Я хочу убедительно показать, что этот вид ошибок повсеместен.

Шестая ошибка живёт в коде библиотеки Angle, используемой в Chromium.

void State::getIntegerv(const Context *context,
                        GLenum pname, GLint *params)
{
  ....
  switch (pname)
  {
    ....
    case GL_DEBUG_GROUP_STACK_DEPTH:
      *params = static_cast<GLint>(mDebug.getGroupStackDepth());
       break;
    case GL_MULTISAMPLE_EXT:
      *params = static_cast<GLint>(mMultiSampling);
       break;
    case GL_SAMPLE_ALPHA_TO_ONE_EXT:
      *params = static_cast<GLint>(mSampleAlphaToOne);      // <=
    case GL_COVERAGE_MODULATION_CHROMIUM:
      *params = static_cast<GLint>(mCoverageModulation);
       break;
    case GL_ATOMIC_COUNTER_BUFFER_BINDING:
    ....
}

Предупреждение PVS-Studio: V519 CWE-563 The '* params' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2044, 2046. state.cpp 2046

Седьмая ошибка живёт в коде библиотеки SwiftShader, используемой в Chromium.

GL_APICALL void GL_APIENTRY glInvalidateSubFramebuffer(....)
{
  ....
  switch(target)
  {
  case GL_DRAW_FRAMEBUFFER:
  case GL_FRAMEBUFFER:
    framebuffer = context->getDrawFramebuffer();
  case GL_READ_FRAMEBUFFER:
    framebuffer = context->getReadFramebuffer();
    break;
  default:
    return error(GL_INVALID_ENUM);
  }
  ....
}

Предупреждение PVS-Studio: V519 CWE-563 The 'framebuffer' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3879, 3881. libglesv3.cpp 3881

Семь - это красивое число. На нём и остановимся. Возможно, есть другие ошибки, но их поиск я оставляю авторам Chromium и библиотек. Мне было скучно внимательно смотреть предупреждения V519. Диагностика V519 даёт немало бестолковых ложных срабатываний, связанных с неаккуратным написанием кода или с макросами. А настраивать анализатор для такого большого проекта – это уже работа, требующая оплаты (да, это был тонкий намёк для Google).

Итак, мы закончили рассматривать примеры, и пришло время поговорить о том, как защититься от обсуждаемого паттерна ошибки.

Рекомендация

Как я написал в начале, по моему мнению, причина подобных ошибок в неудачной реализации синтаксиса языка. И что-то менять уже поздно. Однако, постепенно эту проблему решают компиляторы и анализаторы. Уже давно существуют предупреждения, сообщающие о том, что забыт оператор break. А когда действительно надо передать управление дальше, то компиляторам и анализаторам сообщают об этом с помощью специальных магических заклинаний, таких как:

[[gnu::fallthrough]];
[[clang::fallthrough]];
__attribute__((fallthrough));
BOOST_FALLTHROUGH;
и так далее.

К сожалению, всё это было как-то не универсально. К счастью, у меня для всех C++ программистов есть хорошая новость. В стандарте C++17 наконец утвердили стандартный метод подсказать компилятору, что программист специально планирует передать управление дальше. Это атрибут [[fallthrough]]. Анализаторы, естественно, тоже будут пользоваться этой подсказкой. Кстати, рекомендую посмотреть нашу статью "C++17" про то, что нового появилось в этом стандарте.

Чуть подробнее про атрибут [[fallthrough]].

Этот атрибут показывает, что оператор break внутри блока case отсутствует намеренно (т.е. управление передается в следующий блок case), и поэтому соответствующее предупреждение компилятора или статического анализатора кода выдаваться не должно.

Пример использования:

switch (i)
{
case 10:
  f1();
  break;
case 20:
  f2();
  [[fallthrough]]; // Предупреждение будет подавлено
case 30:
  f3();
  break;
case 40:
  f4();
  break;
}

Если Вы уже перешли на C++17, нет причин не использовать [[fallthrough]]. Включите предупреждения в своём компиляторе, чтобы он информировал о пропущенном break. А там, где оператор break в действительности не нужен, пишите [[fallthrough]]. Также рекомендую описать всё это в используемом в вашей компании стандарте кодирования.

Компиляторы Clang и GCC начинают предупреждать о пропущенном break, если указать им флаг:

-Wimplicit-fallthrough

Если добавить [[fallthrough]], то предупреждение пропадает.

С MSVC сложнее. Начиная с Visual C++ 2017 RTM, он, вроде, должен выводить предупреждение C4468, если указан флаг /W4. Подробнее: Compiler Warnings by compiler version (см. C4468). Но что-то у меня последняя версия Visual Studio со всеми последними обновлениями упорно молчит. Впрочем, я долго не экспериментировал и, возможно, сделал что-то неправильно. В любом случае, если не сейчас, то в ближайшее время этот механизм будет работать и в Visual C++.

Спасибо всем за внимание. Желаю всем безбажного кода. Да, и не забудьте попробовать проверить с помощью PVS-Studio ваши рабочие проекты.

#Cpp #Knowledge