We would like to suggest reading the series of articles dedicated to the recommendations on writing code of high quality using the examples of errors found in the Chromium project. This is the second part, which will be devoted to the switch operator and, more precisely, to the problem of a forgotten break operator.
For many years I have been studying errors in programs and now I can say for sure that in C and C++ the switch operator is implemented incorrectly. I understand that the possibility not to write break, made to pass control further, allows writing elegant algorithms. But still a great number of errors convinced me that the wrong approach was chosen. Sure, it is too late now. I just wanted to say that the right decision would be to necessarily write the word break or a reverse keyword, for example, fallthrough. It would have saved so much effort, time and money. Of course, this shortcoming can not be compared with Null References: The Billion Dollar Mistake, but is still a big blooper.
Well, enough of philosophy. C++ language is the way it is. However, it does not mean that you can relax and do nothing to improve the quality and reliability of your code. The problem of "missing break" is a big problem, and it should not be underestimated. Even in a high-quality Chromium project errors of this type are hidden.
Let's see what I noticed during studying the report issued by PVS-Studio. As I wrote in the introductory article, I looked through the report quite fluently, so there may be other, unnoticed errors. However, all these found bugs not enough for me to demonstrate, that they are not just separate random bloopers, but sustainable patterns of errors. Readers should take this pattern seriously and try to use measures to prevent it.
The first example of an error is taken directly from the Chromium project code.
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;
....
}Regardless of whether it is needed to automatically fill a field "Street Address", or a field "City", in any case, a constant GROUP_ADDRESS_CITY will be chosen. I.e. somewhere instead of a street name, a city name will be filled automatically.
The reason is the missing break operator. As a result, after the assignment:
group = GROUP_STREET_ADDRESS;The variable group will be immediately assigned a new value:
group = GROUP_ADDRESS_CITY;PVS-Studio analyzer notices this double assignment and issues a warning: V519 The 'group' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 145, 147. autofill_metrics.cc 147
The second error also refers to the Chromium code and looks the same way.
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:
....
}Here 2 or 3 break operators have been forgotten. I don't know how exactly this code should work, so I shall refrain from commenting about how to fix the error. PVS-Studio analyzer generates two warnings for this code:
The third error from Chromium code.
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;
}
....
}Exactly the same situation as earlier. PVS-Studio warning: 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
The fourth error from Chromium code. This time the V796 warning, not the V519 will help us. V519 diagnostic identifies a missed break indirectly when it notices a repeated assignment. V796 diagnostic was designed specifically to search for missed break operators.
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;
}
}After performing a branch "UI_COMPOSITOR_CONTEXT", the control is passed to a branch of "CONTEXT_TYPE_UNKNOWN". Apparently, this leads to incorrect handling... And here I don't know what impact it will have. Apparently, break is skipped here accidentally, not intentionally.
PVS-Studio warning: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. command_buffer_metrics.cc 125
The fifth bug in Chromium, because of which the work with the middle mouse button is incorrect.
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;
}
....
}Pressing the middle mouse button is handled incorrectly. After the correct action:
modifier = ui::MODIFIER_MIDDLE_MOUSE_BUTTON;A transition occurs to the handler of erroneous flags, and the function exits prematurely.
PVS-Studio warning: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. system_input_injector_mus.cc 78
Here a reader might say: "Enough, it is clear!". However, I noticed another couple of such errors in the used libraries, so let's see them. I would like to convincingly show that this kind of errors is widespread.
The sixth bug lives in the Angle code library, used in 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 warning: V519 CWE-563 The '* params' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 2044, 2046. state.cpp 2046
The seventh bug lives in the SwiftShader code library used in 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 warning: V519 CWE-563 The 'framebuffer' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3879, 3881. libglesv3.cpp 3881
Seven is a nice number. So, let's stop at this point. Perhaps, there are other errors, but I'll leave their finding up to authors of Chromium, and libraries. I was bored to carefully review the V519 warnings. V519 diagnostic gives a lot of stupid false positives related to sloppy code or macros writing. To configure the analyzer for such a large project - it's the work, requiring payment (Yes, it was a subtle hint for Google).
So, we finished to deal with examples, and it's time to talk about how to protect ourselves from the error pattern under discussion.
Recommendation
As I wrote at the beginning, in my opinion, the reason of such errors is the incorrect implementation of the language syntax. And it's too late to change something. However, compilers and analyzers are gradually solving the problem. Warnings, notifying that the break operator was forgotten, have been existing for a long time. When the control has to be passed further, compilers and analyzers are reported about this by using special magic spells, such as:
Unfortunately, all this was not universally. Luckily, I have good news for all C++ programmers. In the C++17, the standard method was finally introduced that can inform the compiler that a programmer plans to transfer control further. This is the [[fallthrough]] attribute. Analyzers, surely, will also use this hint. By the way, I recommend checking out our article "C++17" about what's new in this standard.
Few word about the attribute [[fallthrough]].
This attribute indicates that the break operator inside a case block is missing intentionally (i.e., control is passed to the next case block), and, therefore, an appropriate compiler or static code analyzer warning should not be issued.
It appears in a switch statement on a line of its own (technically as an attribute of a null statement), immediately before a case label.
Example of usage:
switch (i)
{
case 10:
f1();
break;
case 20:
f2();
[[fallthrough]]; // The warning will be suppressed
case 30:
f3();
break;
case 40:
f4();
break;
}If you have already moved to C++17, there is no reason not to use [[fall-through]]. Enable warnings in your compiler to inform about the skipped break. In cases when the break operator is not actually needed, write [[fallthrough]]. Also I recommend describing all this in the coding standard used in your company.
Clang and GCC compilers are starting to warn about a missed break, if you specify a flag to them:
-Wimplicit-fallthroughIf you add [[fallthrough]], the warning disappears.
It is harder with MSVC. Starting with Visual C++ 2017 RTM, it has to generate the C4468 warning, if the /W4 flag is specified. Read more: Compiler Warnings by compiler version (see. C4468). But my last Visual Studio version with the latest updates is keeping silent. However, I haven't experimented for a long time and may be I did something wrong. In any case, if not now, then in the near future this mechanism will work in Visual C++ as well.
Thank you for attention. I wish you bugless coding! Don't forget to try checking your work projects using PVS-Studio .