Break and fallthrough operator

We bring to your attention a series of articles devoted to recommendations for writing high-quality code using the example of errors found in the Chromium project. This is the second part, which will be devoted to the switch operator, or rather, the problem of the forgotten break operator.



For many years I have been studying errors in programs and now I can say with confidence that in C, and after him in C ++, the switch statement was done incorrectly. I understand that the possibility not to write break , made to transfer control further, allows writing elegant algorithms. But still, a huge number of errors convinced me that the wrong approach was taken. It is clear that now it is too late. I just wanted to say that the right decision would be to write the word break or the reverse keyword, for example, fallthrough . How much effort, time and money was saved. Of course, this drawback can not be compared with Null References: The Billion Dollar Mistake , but still a big mistake.



Ok, philosophized and that's enough. C ++ language is what it is. However, this does not mean that you can relax and do nothing to improve the quality and reliability of the code. The problem of a “missed break” is a big problem and should not be underestimated. Even in the high-quality Chromium project, errors of this type are hidden.



Let's see what I noticed in the process of parsing the report issued by PVS-Studio. As I wrote in the introductory article , I looked at the report rather briefly, so there may be other errors that I did not notice. Nevertheless, the errors found will be enough for me to demonstrate that all these are not separate random blunders, but a steady pattern of errors. Readers should take this pattern seriously and try to use measures to prevent it.

')

The first 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 you need to automatically fill in a certain “Street Address” field or the “City” field, the GROUP_ADDRESS_CITY constant will be selected in any case. Those. somewhere instead of the street name will automatically be substituted the name of the city.



The reason is that the break statement is missing. As a result, after assignment:

 group = GROUP_STREET_ADDRESS; 

The group variable will immediately be assigned a new value:

 group = GROUP_ADDRESS_CITY; 

The 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 relates 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: .... } 

2 or 3 break statements are forgotten here. I do not know how exactly this code should work, so I will refrain from commenting on how to correct the error. The PVS-Studio analyzer generates two warnings for this code:

• 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

The third error from the 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 picture as before. 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 the Chromium code. This time, the V796 warning will help us, not the V519. Diagnostics V519 detects a missing break in an indirect way when it notices a reassignment. Diagnostics V796 is designed specifically to search for missing operators 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; } } 

After executing the “UI_COMPOSITOR_CONTEXT” branch, control is transferred to the “CONTEXT_TYPE_UNKNOWN” branch. Apparently, this leads somewhere to incorrect processing ... But I do not know what it will affect. But the break here is missed, apparently by chance, and not deliberately.



PVS-Studio Warning: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. command_buffer_metrics.cc 125



The fifth error in the Chromium code, due to which the middle mouse button suffers.

 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; } .... } 

Incorrectly handling a click on the middle mouse button. After correct action:

 modifier = ui::MODIFIER_MIDDLE_MOUSE_BUTTON; 

The transition to the error flag handler occurs, and the function completes its work ahead of schedule.



PVS-Studio Warning: V796 CWE-484 It is possible that 'break' statement is missing in switch statement. system_input_injector_mus.cc 78



Here the reader has the right to say: “Everything is clear, enough!”. However, I caught sight of a couple more such errors in the libraries used, so let's look at them as well. I want to convincingly show that this kind of error is ubiquitous.



The sixth error lives in the code of the Angle 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 error lives in the SwiftShader library code 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 beautiful number. On it and stop. Perhaps there are other errors, but I leave it to the authors of Chromium and the libraries. I was bored watching the V519 warnings carefully. Diagnostics V519 gives a lot of stupid false positives associated with inaccurate code writing or with macros. And setting up the analyzer for such a large project is already a job requiring payment (yes, it was a subtle hint for Google).



So, we’ve finished looking at examples, and it’s time to talk about how to defend against the error pattern in question.



Recommendation



As I wrote at the beginning, in my opinion, the reason for such errors is the unsuccessful implementation of the syntax of the language. And something is too late to change. However, compilers and analyzers gradually solve this problem. For a long time, there have been warnings that the break statement has been forgotten. And when it is really necessary to transfer control further, the compilers and analyzers are informed about this with the help of special magic spells, such as:

• [[gnu :: fallthrough]];
• [[clang :: fallthrough]];
• __attribute __ ((fallthrough));
• BOOST_FALLTHROUGH;
• and so on.

Unfortunately, all this was somehow not universal. Fortunately, I have good news for all C ++ programmers. In the C ++ standard, 17 finally approved the standard method to prompt the compiler that the programmer specifically plans to transfer control further. This is the [[fallthrough]] attribute. Analyzers, of course, will also use this hint. By the way, I recommend to look at our article " C ++ 17 " about what is new in this standard.



A little more about the attribute [[fallthrough]] .



This attribute indicates that the break statement inside the case block is missing intentionally (that is, control is passed to the next case block), and therefore the corresponding warning from the compiler or static code analyzer should not be issued.



Usage example:

 switch (i) { case 10: f1(); break; case 20: f2(); [[fallthrough]]; //    case 30: f3(); break; case 40: f4(); break; } 

If you have already moved to C ++ 17, there is no reason not to use [[fallthrough]] . Turn on warnings in your compiler so that it informs about a missed break . And where the break statement is not really needed, write [[fallthrough]] . I also recommend to describe all this in the coding standard used in your company.



The Clang and GCC compilers begin to warn about a missed break , if you specify the flag:

 -Wimplicit-fallthrough 

If you add [[fallthrough]] , then the warning disappears.



With MSVC harder. Starting with Visual C ++ 2017 RTM, it seems to be displaying the warning C4468 if the / W4 flag is specified. Details: Compiler Warnings by compiler version (see C4468). But something I have the latest version of Visual Studio with all the latest updates stubbornly silent. However, I have not experimented for a long time and may have done something wrong. In any case, if not now, then in the near future this mechanism will work in Visual C ++.



Thank you all for your attention. I wish you all a hopeless code. Yes, and do not forget to try checking your working projects with PVS-Studio .





If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey Karpov. break and fallthrough .