# Bugs found in GCC with the help of PVS\-Studio I regularly check various open\-source projects to demonstrate the abilities of the PVS\-Studio static code analyzer \(C, C\+\+, C\#\)\. Now it is time for the GCC compiler to get checked\. Unquestionably, GCC is a very qualitative and well\-tested project, that's why it's already a great achievement for a tool to find any errors in it\. Fortunately, PVS\-Studio coped with this task\. No one is immune to typos or carelessness\. This is why the PVS\-Studio can become an additional line of defense for you, on the front of the endless war against bugs\. ![0425_GCC/image1.png](https://import.viva64.com/docx/blog/0425_GCC/image1.png) ## GCC [GNU Compiler Collection](https://gcc.gnu.org/) \(usually shortened to GCC\) \- is a set of compilers for different programming languages developed in the scope of the GNU project\. GCC is free software, distributed by the free software foundation under the terms of the GNU GPL and GNU LGPL and is a key component of the GNU toolchain\. The project is written in C and C\+\+\. The GCC compiler has great built\-in diagnostics, which help in the detection of many issues at the compilation stage\. Of course, GCC is built with GCC and, thus, is able to find errors in its own code\. Additionally, the GCC source code is [checked](https://scan.coverity.com/projects/gcc) by the Coverity analyzer\. In general, I think that a lot of enthusiasts have also checked it with other analyzers and other tools\. This makes it a hard task for PVS\-Studio to find errors in GCC\. We used the trunk version from the git\-repository: git://gcc\.gnu\.org/git/gcc\.git \(git\) commit: 00a7fcca6a4657b6cf203824beda1e89f751354b svn\+ssh://gcc\.gnu\.org/svn/gcc/trunk@238976 Note\. The article publication is slightly late, and perhaps some bugs are already fixed\. However, it's not a big deal: new errors constantly get in the code and the old ones disappear\. The main thing is that the article shows that static analysis can help programmers to detect errors after they get into the code\. ## Foreseeing a discussion As I said in the beginning, I consider GCC project to be of high quality\. I am sure a lot of people will want to argue with that\. As an example, I'll give a quote from Wikipedia in Russian \(translated\): _Some OpenBSD_ _developers,_ _Theo_ _de_ _Raadt_ _and_ _Otto_ _Moerbeek_ _criticiz_ _GCC,_ _saying_ _that_ _"gcc_ _gets_ _about_ _5\-6%_ _slower_ _every_ _release,_ _has_ _new_ _bugs,_ _generates_ _crappy_ _code, and_ _drives_ _us_ _nuts"\._ To my mind, these statements are unjustified\. Yes, perhaps, GCC code has too many macros which make reading it a bit difficult\. But I can't agree with the statement about it being buggy\. If GCC were buggy, nothing would work at all\. Just think about the amount of programs that get successfully compiled by it and thus, work well\. The creators of GCC do a great, complicated job, with professionalism\. We should really thank them\. I'm glad I can test the work of PVS\-Studio on such a high\-quality project\. For those who say that the code of Clang is still much better, I can remind you: PVS\-Studio also found bugs in it: [1](https://pvs-studio.com/en/blog/posts/0108/), [2](https://pvs-studio.com/en/blog/posts/cpp/0155/)\. ## PVS\-Studio I have checked the GCC code with the help of the alpha\-version of PVS\-Studio for Linux\. We are planning to give the beta\-version of the analyzer in the middle of September 2016 to those programmers who will find it useful\. You may find the instruction of how to become the first person to try the Beta\-version of PVS\-Studio for Linux on your project in the article "[PVS\-Studio confesses its love for Linux](https://pvs-studio.com/en/blog/posts/0415/)" If you are reading this article much later than September 2016, and want to try PVS\-Studio for Linux, then I suggest visiting the [product page](https://pvs-studio.com/en/pvs-studio/)\. ## Analysis results We have got to the most interesting part of the article, which our regular readers are looking forward to\. Let's have a look at those code fragments where the analyzer found bugs or really suspicious code\. Unfortunately, I can't give the developers the full analysis report\. There is too much garbage \(false alarms\) at this point, because of the fact that the analyzer isn't ready to face the Linux world yet\. We still have a lot of work to do regarding the reduction of the number of false positives for typical constructions\. I'll try to explain using a simple example\. Many diagnostics should not issue warnings for the expressions related to the _assert_ macros\. These macros are sometimes written very creatively, so we should teach the analyzer not to pay attention to them\. The thing is that the _assert_ macro can be defined in many various ways, thus we should teach PVS\-Studio all the typical variants\. That's why I am asking GCC developers to wait until the Beta\-version is released\. I wouldn't like to spoil the impression by a report, generated by a half\-finished version\. ### Classics \(Copy\-Paste\) We'll start with the most common and classical error that is detected by [V501](https://pvs-studio.com/en/docs/warnings/v501/) diagnostic\. Typically, these errors appear because of carelessness when Copy\-Pasting the code, or are just typos during the creation of new code\. ```cpp static bool dw_val_equal_p (dw_val_node *a, dw_val_node *b) { .... case dw_val_class_vms_delta: return (!strcmp (a->v.val_vms_delta.lbl1, b->v.val_vms_delta.lbl1) && !strcmp (a->v.val_vms_delta.lbl1, b->v.val_vms_delta.lbl1)); .... } ``` **PVS\-Studio warning:** [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions '\!strcmp\(a\-\>v\.val\_vms\_delta\.lbl1, b\-\>v\.val\_vms\_delta\.lbl1\)' to the left and to the right of the '&&' operator\. dwarf2out\.c 1428 It's hard to see the errors right away, we should take a closer look here\. This is why the error was not detected during code reviews and refactoring\. The function _strcmp_ compares the same strings twice\. It seems to me that we should have compared not the members of _lbl1 _c\.lass, but of _lbl2\._ Then the correct code could look like this: ```cpp return (!strcmp (a->v.val_vms_delta.lbl1, b->v.val_vms_delta.lbl1) && !strcmp (a->v.val_vms_delta.lbl2, b->v.val_vms_delta.lbl2)); ``` It should be noted that the code, provided in the article, is slightly aligned, so that it doesn't take too much space on the x\-axis\. In fact, the code looks like this: ![0425_GCC/image2.png](https://import.viva64.com/docx/blog/0425_GCC/image2.png) This error could be avoided by using "table" code alignment\. For example, an error would be easier to notice if you format the code like this: ![0425_GCC/image3.png](https://import.viva64.com/docx/blog/0425_GCC/image3.png) I have spoken about this approach in details in the e\-book "[The Ultimate Question of Programming, Refactoring, and Everything](https://pvs-studio.com/en/blog/posts/cpp/0391/)" \(see chapter N13: Table\-style formatting"\)\. I recommend that everyone who cares about code quality have a look at this book\. Let's look at one more mistake, which I'm sure, appeared because of Copy\-Paste: ```cpp const char *host_detect_local_cpu (int argc, const char **argv) { unsigned int has_avx512vl = 0; unsigned int has_avx512ifma = 0; .... has_avx512dq = ebx & bit_AVX512DQ; has_avx512bw = ebx & bit_AVX512BW; has_avx512vl = ebx & bit_AVX512VL; // <= has_avx512vl = ebx & bit_AVX512IFMA; // <= .... } ``` **PVS\-Studio warning**: [V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'has\_avx512vl' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 500, 501\. driver\-i386\.c 501 Different values are written to the variable _has\_avx512vl_ twice in a row\. It does not make sense\. I have reviewed the code and found a variable _has\_avx512ifma_\. Most likely, it should be initialized by the expression _ebx & bit\_AVX512IFMA_\. Then the correct code should be as follows: ```cpp has_avx512vl = ebx & bit_AVX512VL; has_avx512ifma = ebx & bit_AVX512IFMA; ``` ### A typo I'll continue testing your attentiveness\. Look at the code and try to find the error, without looking at the warning below\. ```cpp static bool ubsan_use_new_style_p (location_t loc) { if (loc == UNKNOWN_LOCATION) return false; expanded_location xloc = expand_location (loc); if (xloc.file == NULL || strncmp (xloc.file, "\1", 2) == 0 || xloc.file == '\0' || xloc.file[0] == '\xff' || xloc.file[1] == '\xff') return false; return true; } ``` **PVS\-Studio warning:** [V528](https://pvs-studio.com/en/docs/warnings/v528/) It is odd that pointer to 'char' type is compared with the '\\0' value\. Probably meant: \*xloc\.file \=\= '\\0'\. ubsan\.c 1472 The programmer accidentally forgot to dereference the pointer in the expression _xloc\.file \=\= '\\0'_\. As a result the pointer is just compared with 0, i\.e\. with _NULL_\. It doesn't have any effect, because such a check was already done earlier: _xloc\.file \=\= NULL_\. The good thing is that the programmer wrote the terminal null as '\\0'\. This helps us to understand quicker, that there is a bug here and how it should be fixed\. I have also written about this in the [book](https://pvs-studio.com/en/blog/posts/cpp/0391/) \(see chapter N9: Use the '\\0' literal for the terminal null character\)\. Correct variant of the code: ```cpp if (xloc.file == NULL || strncmp (xloc.file, "\1", 2) == 0 || xloc.file[0] == '\0' || xloc.file[0] == '\xff' || xloc.file[1] == '\xff') return false; ``` Although, let's improve the code even more\. I recommend formatting the expression like this: ```cpp if ( xloc.file == NULL || strncmp (xloc.file, "\1", 2) == 0 || xloc.file[0] == '\0' || xloc.file[0] == '\xff' || xloc.file[1] == '\xff') return false; ``` Pay attention: now if you make the same mistake, the chance of noticing it is slightly higher: ```cpp if ( xloc.file == NULL || strncmp (xloc.file, "\1", 2) == 0 || xloc.file == '\0' || xloc.file[0] == '\xff' || xloc.file[1] == '\xff') return false; ``` ### Potential null pointer dereference This part could also be called "Example number one thousand, why macros are bad"\. I really don't like macros and always urge people to avoid using them if possible\. Macros make it difficult to read the code, provoke errors, and make the work of static analyzers harder\. As best I can tell, from a brief interaction with the GCC code, the authors are big fans of macros\. I was really tired looking at what the macros are expanded to, and perhaps missed quite a number of interesting errors\. I should confess that I was lazy at times\. But still, I will demonstrate a couple of errors, connected with macros\. ```cpp odr_type get_odr_type (tree type, bool insert) { .... odr_types[val->id] = 0; gcc_assert (val->derived_types.length() == 0); if (odr_types_ptr) val->id = odr_types.length (); .... } ``` **PVS\-Studio warning**: [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'odr\_types\_ptr' pointer was utilized before it was verified against nullptr\. Check lines: 2135, 2139\. ipa\-devirt\.c 2135 Do you see an error here? I guess not, and the analyzer warning isn't of much help\. The matter of the fact is that _odr\_types_ isn't a name of a variable, but a macro that is declared in the following way: ```cpp #define odr_types (*odr_types_ptr) ``` If we expand the macro and remove everything that isn't really related to the code, we'll get the following: ```cpp (*odr_types_ptr)[val->id] = 0; if (odr_types_ptr) ``` First, the pointer is dereferenced and then checked\. It's hard to say whether this will lead to trouble or not\. It all depends on the situation, whether the pointer will really be equal to _nullptr_\. If this situation is impossible, then this redundant check should be removed, otherwise it will mislead people supporting the code, and the code analyzer as well\. If a pointer can be null, then it is a serious mistake that requires even more attention and should be fixed\. Let's consider one more similar case: ```cpp static inline bool sd_iterator_cond (sd_iterator_def *it_ptr, dep_t *dep_ptr) { .... it_ptr->linkp = &DEPS_LIST_FIRST (list); if (list) continue; .... } ``` **PVS\-Studio warning:** [V595](https://pvs-studio.com/en/docs/warnings/v595/) The 'list' pointer was utilized before it was verified against nullptr\. Check lines: 1627, 1629\. sched\-int\.h 1627 We should show the macro again to see the error: ```cpp #define DEPS_LIST_FIRST(L) ((L)->first) ``` Let's expand the macro and get: ```cpp it_ptr->linkp = &((list)->first); if (list) continue; ``` Some of you may say: "Hey, wait\! There is no error here\. We just get a pointer to the class member\. There is no null pointer dereference\. Yes, perhaps the code isn't really accurate, but there is no error\!" Yet, it's not as simple as it may seem\. We have undefined behavior here\. It's just pure luck that such code works\. Actually, we cannot write like this\. For example, the optimizing compiler can remove the check _if \(list\), _if it sees_ list\-\>first\. _If we execute the _\-\> _operator, then it is assumed that the pointer isn't equal to _nullptr_\. If it so, then we shouldn't check the pointer\. I have written a whole article on this topic: "[Null Pointer Dereferencing Causes Undefined Behavior](https://pvs-studio.com/en/blog/posts/cpp/0306/)" I am examining a similar case there\. Before starting any arguments, please carefully read this article\. However, this situation is really complicated, and is not really obvious\. I can assume that I may be wrong and there is no error here\. However, until now no one could prove it to me\. It will be interesting to see the comments of GCC developers, if they read this article\. They should know how the compiler works, and if this code should be interpreted as erroneous or not\. ### Using a destroyed array ```cpp static void dump_hsa_symbol (FILE *f, hsa_symbol *symbol) { const char *name; if (symbol->m_name) name = symbol->m_name; else { char buf[64]; sprintf (buf, "__%s_%i", hsa_seg_name (symbol->m_segment), symbol->m_name_number); name = buf; } fprintf (f, "align(%u) %s_%s %s", hsa_byte_alignment (symbol->m_align), hsa_seg_name(symbol->m_segment), hsa_type_name(symbol->m_type & ~BRIG_TYPE_ARRAY_MASK), name); .... } ``` **PVS\-Studio warning:** [V507](https://pvs-studio.com/en/docs/warnings/v507/) Pointer to local array 'buf' is stored outside the scope of this array\. Such a pointer will become invalid\. hsa\-dump\.c 704 The string is formed in the temporary buffer _buf_\. The address of this temporary buffer is stored in the variable _name_, and is used further on in the body of the function\. The error is that after the buffer is written to the variable _name,_ the buffer itself will be destroyed\. We cannot use a pointer to a destroyed buffer\. Formally, we are dealing with undefined behavior\. In practice, this code may work quite successfully\. Correct working of the program is one of the ways in which undefined behavior shows itself\. In any case, this code has an error and it must be fixed\. The code can work due to the fact that the compiler may think it is unnecessary to use a temporary buffer for storing other variables and arrays further on\. Then, although the array, created on the stack is considered to be destroyed, it will not be used, and the function will work correctly\. But this luck can end any time, and the code that used to work for 10 years may suddenly start acting weirdly when upgrading to a new version of the compiler\. To fix this error, we should declare the _buf _array in the same scope as the _name_ pointer: ```cpp static void dump_hsa_symbol (FILE *f, hsa_symbol *symbol) { const char *name; char buf[64]; .... } ``` ### Execution of similar actions regardless of the condition The analyzer detected a code fragment that I cannot call erroneous with 100% certainty\. However, it is really suspicious to do the check and then, regardless of the result, perform the same actions\. Of course, it may work correctly, but this code fragment is worth revising for sure\. ```cpp bool thread_through_all_blocks (bool may_peel_loop_headers) { .... /* Case 1, threading from outside to inside the loop after we'd already threaded through the header. */ if ((*path)[0]->e->dest->loop_father != path->last ()->e->src->loop_father) { delete_jump_thread_path (path); e->aux = NULL; ei_next (&ei); } else { delete_jump_thread_path (path); e->aux = NULL; ei_next (&ei); } .... } ``` **PVS\-Studio warning**: [V523](https://pvs-studio.com/en/docs/warnings/v523/) The 'then' statement is equivalent to the 'else' statement\. tree\-ssa\-threadupdate\.c 2596 If this code has a bug, it's hard to say how to fix it\. This is a case where it is necessary to be familiar with the project, in order to fix it\. ### Redundant expression of the \(A \=\= 1 \|\| A \!\= 2\) kind ```cpp static const char * alter_output_for_subst_insn (rtx insn, int alt) { const char *insn_out, *sp ; char *old_out, *new_out, *cp; int i, j, new_len; insn_out = XTMPL (insn, 3); if (alt < 2 || *insn_out == '*' || *insn_out != '@') return insn_out; .... } ``` **PVS\-Studio warning:** [V590](https://pvs-studio.com/en/docs/warnings/v590/) Consider inspecting this expression\. The expression is excessive or contains a misprint\. gensupport\.c 1640 We are interested in the condition: \(alt < 2 \|\| \*insn\_out \=\= '\*' \|\| \*insn\_out \!\= '@'\)\. It can be shortened to: \(alt < 2 \|\| \*insn\_out \!\= '@'\)\. I would venture to guess that the operator _\!\=_ should be replaced with_ \=\=_\. Then the code will make more sense: ```cpp if (alt < 2 || *insn_out == '*' || *insn_out == '@') ``` ### Zeroing a wrong pointer Let us consider a function that frees the resources: ```cpp void free_original_copy_tables (void) { gcc_assert (original_copy_bb_pool); delete bb_copy; bb_copy = NULL; delete bb_original; bb_copy = NULL; delete loop_copy; loop_copy = NULL; delete original_copy_bb_pool; original_copy_bb_pool = NULL; } ``` **PVS\-Studio warning**: [V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'bb\_copy' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 1076, 1078\. cfg\.c 1078 Take a look at these 4 lines of code: ```cpp delete bb_copy; bb_copy = NULL; delete bb_original; bb_copy = NULL; ``` Accidentally, the pointer _bb\_copy_ is zeroed twice\. Here is the correct version: ```cpp delete bb_copy; bb_copy = NULL; delete bb_original; bb_original = NULL; ``` ### Assert that does not check anything Invalid condition, being an argument of the macro _gcc\_assert_, won't affect how correctly the program works, but will make the bug search more complicated, if there is such\. Let us consider the code: ```cpp static void output_loc_operands (dw_loc_descr_ref loc, int for_eh_or_skip) { unsigned long die_offset = get_ref_die_offset (val1->v.val_die_ref.die); .... gcc_assert (die_offset > 0 && die_offset <= (loc->dw_loc_opc == DW_OP_call2) ? 0xffff : 0xffffffff); .... } ``` **PVS\-Studio warning:** [V502](https://pvs-studio.com/en/docs/warnings/v502/) Perhaps the '?:' operator works in a different way than it was expected\. The '?:' operator has a lower priority than the '<\=' operator\. dwarf2out\.c 2053 The priority of the ternary operator _?: _is lower than of the _<\=_ operator and _&&_ operator_\._ This means that we are dealing with a condition like this: ```cpp (die_offset > 0 && die_offset <= (loc->dw_loc_opc == DW_OP_call2)) ? 0xffff : 0xffffffff; ``` Both of values \(_0xffff_ and _0xffffffff_\) are true, so this expression can be simplified to: ```cpp (true) ``` This is clearly not what the programmer intended to get\. To fix this, you should add a pair of parentheses: ```cpp gcc_assert (die_offset > 0 && die_offset <= ((loc->dw_loc_opc == DW_OP_call2) ? 0xffff : 0xffffffff)); ``` The ?: operator is very treacherous, and it's better not to use it in complex expressions\. It is very easy to make a mistake\. We have [collected](https://pvs-studio.com/en/blog/examples/v502/) a large number of examples of such errors, which were found by PVS\-Studio in various open source projects\. I have also written in details about the _?:_ operator in the [book](https://pvs-studio.com/en/blog/posts/cpp/0391/) that I've mentioned earlier \(see chapter N4: Beware of the ?: operator and enclose it in parentheses\)\. ### Forgotten "cost" The structure _alg\_hash\_entry_ is declared in the following way: ```cpp struct alg_hash_entry { unsigned HOST_WIDE_INT t; machine_mode mode; enum alg_code alg; struct mult_cost cost; bool speed; }; ``` The programmer decided to check if in the _synth\_mult_ function there is an object that is needed\. To do this he needed to compare the structure fields\. However, it seems like there is an error here: ```cpp static void synth_mult (....) { .... struct alg_hash_entry *entry_ptr; .... if (entry_ptr->t == t && entry_ptr->mode == mode && entry_ptr->mode == mode && entry_ptr->speed == speed && entry_ptr->alg != alg_unknown) { .... } ``` **PVS\-Studio warning:** [V501](https://pvs-studio.com/en/docs/warnings/v501/) There are identical sub\-expressions 'entry\_ptr\-\>mode \=\= mode' to the left and to the right of the '&&' operator\. expmed\.c 2573 _Mode _is checked twice, but _cost _isn't checked in any way\. Perhaps, one of these comparisons should be removed, but there is a chance, that we should compare _cost_\. It's difficult for me to say, but the code should be fixed\. ### Duplicated assignments To my mind, the following code fragments don't pose any hazard to the life of the program, and it seems that the duplicated assignment can just be removed\. **Fragment** **N1** ```cpp type_p find_structure (const char *name, enum typekind kind) { .... structures = s; // <= s->kind = kind; s->u.s.tag = name; structures = s; // <= return s; } ``` **PVS\-Studio warning:** [V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'structures' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 842, 845\. gengtype\.c 845 **Fragment** **N2** ```cpp static rtx ix86_expand_sse_pcmpistr (....) { unsigned int i, nargs; .... case V8DI_FTYPE_V8DI_V8DI_V8DI_INT_UQI: case V16SI_FTYPE_V16SI_V16SI_V16SI_INT_UHI: case V2DF_FTYPE_V2DF_V2DF_V2DI_INT_UQI: case V4SF_FTYPE_V4SF_V4SF_V4SI_INT_UQI: case V8SF_FTYPE_V8SF_V8SF_V8SI_INT_UQI: case V8SI_FTYPE_V8SI_V8SI_V8SI_INT_UQI: case V4DF_FTYPE_V4DF_V4DF_V4DI_INT_UQI: case V4DI_FTYPE_V4DI_V4DI_V4DI_INT_UQI: case V4SI_FTYPE_V4SI_V4SI_V4SI_INT_UQI: case V2DI_FTYPE_V2DI_V2DI_V2DI_INT_UQI: nargs = 5; // <= nargs = 5; // <= mask_pos = 1; nargs_constant = 1; break; .... } ``` **PVS\-Studio warning:** [V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'nargs' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 39951, 39952\. i386\.c 39952 **Fragment** **N3** The latter fragment seems to be stranger than the other ones\. Perhaps there is some mistake here\. The variable _steptype_ is assigned with a value 2 or 3 times\. It is very suspicious\. ```cpp static void cand_value_at (....) { aff_tree step, delta, nit; struct iv *iv = cand->iv; tree type = TREE_TYPE (iv->base); tree steptype = type; // <= if (POINTER_TYPE_P (type)) steptype = sizetype; // <= steptype = unsigned_type_for (type); // <= .... } ``` **PVS\-Studio** **warning:** [V519](https://pvs-studio.com/en/docs/warnings/v519/) The 'steptype' variable is assigned values twice successively\. Perhaps this is a mistake\. Check lines: 5173, 5174\. tree\-ssa\-loop\-ivopts\.c 5174 ## Conclusion I'm glad I could write this article\. Now I will have something to say in response to the comments like "PVS\-Studio isn't necessary, because GCC issues the same warnings"\. As you can see, PVS\-Studio is a very powerful tool and excels GCC in the diagnostic capabilities\. I do not deny that the GCC has excellent diagnostics\. This compiler, if properly set up, really brings out a lot of problems in the code\. But PVS\-Studio is a specialized and rapidly developing tool, which means that it will also be better at detecting errors in the code than the compilers\. I suggest taking a look at the analysis of other open\-source projects, and visiting this [section](https://pvs-studio.com/en/blog/inspections/) of our web\-site\. Also, those who use Twitter can follow me [@Code\_Analysis](https://twitter.com/Code_Analysis)\. I regularly post links to interesting articles about programming on C and C\+\+, and also talk about the achievements of our analyzer\.