Finding errors in the GCC compiler code using the PVS-Studio analyzer

I regularly check various open projects to demonstrate the capabilities of the PVS-Studio static code analyzer (C, C ++, C #). It's time for the GCC compiler. Undoubtedly, GCC is a very high-quality and tested project, so finding at least a few mistakes in it is already a great achievement for any tool. To my delight, PVS-Studio has coped with this task. No one is immune from typos and inattention. That is why PVS-Studio can become your additional line of defense on the front of the endless war with bugs.

Gcc

The GNU Compiler Collection (commonly abbreviated as GCC) is a set of compilers for various programming languages ​​developed by the GNU project. GCC is free software, distributed by the free software foundation under the GNU GPL and the GNU LGPL, and is a key component of the GNU toolchain. The project is written in C and C ++.

The GCC compiler has good in-built diagnostics that help identify many errors at the compilation stage. Naturally, GCC is collected using GCC and, accordingly, it can detect errors in its own code. Additionally, the GCC source code is checked using the Coverity analyzer. And in general, I think GCC was tested by enthusiasts with the help of many analyzers and other tools. This makes finding errors in GCC a big challenge for the PVS-Studio code analyzer.

The trunk version from the git repository was taken for analysis: (git) commit 00a7fcca6a4657b6cf203824beda1e89f751354b svn + ssh: //gcc.gnu.org/svn/gcc/trunk@238976
')
Note. The article was delayed with the release, and perhaps some errors have already been fixed. But it does not matter: new errors constantly appear, old ones disappear. The main thing - the article shows that static analysis can help programmers to detect errors after they appear.

Anticipating the discussion

As I said in the introduction, I consider GCC a project with high quality code. I am sure many will want to argue. As an example, I will quote from Wikipedia in Russian:

Some OpenBSD developers, such as Theo de Raadt and Otto Moerbeek, have criticized GCC, calling it "cumbersome, buggy, slow, and generating bad code."

I consider such statements unfounded. Yes, it is possible that the GCC code contains many macros that make it difficult to read. But I can’t agree with the statement about its buggy. If GCC was buggy, nothing would work anywhere. Just remember how many programs they compile and work successfully. The creators of GCC do a tremendous, challenging job with great professionalism. Thanks to them. I am glad that I can test the work of PVS-Studio on such a high-quality project.

For those who say that the Clang compiler code is still better, let me remind you: PVS-Studio also found errors in it: 1 , 2 .

PVS-Studio

I checked the GCC code using the Alpha-version of the PVS-Studio for Linux analyzer. We plan to start issuing the Beta-version of the analyzer to mid-September 2016 to interested programmers. You will find instructions on how to be one of the first to try the Beta-version of PVS-Studio for Linux on your project in the article " PVS-Studio admits love for Linux ".

If you read this article much later than September 2016 and want to try PVS-Studio for Linux, then I invite you to the product page: http://www.viva64.com/ru/pvs-studio/

Test results

We got to the most interesting section, which, I think, our regular readers are looking forward to. Consider the code sections where the analyzer found errors or extremely suspicious moments.

Unfortunately, I can not give the compiler a complete report. It still has too much garbage (false positives) due to the fact that the analyzer is not fully ready to meet with the Linux world. It is necessary to do the work to reduce the number of false warnings on the standard constructions used. I will try to explain with one simple example. Many diagnostics should not swear expressions related to assert macros. These macros are very creative and you need to teach the analyzer not to pay attention to them. But the fact is that the assert macro is defined in very different ways, and the analyzer must be trained in all typical variants.

Therefore, developers of GCC, please wait for at least Beta-version of the analyzer. I don't want to spoil the impression with a report generated by the unfinished version.

Classic (Copy-Paste)

We will start with the most classic and common error that is detected using the V501 diagnostic. As a rule, such errors appear due to carelessness during Copy-Paste or are simply misprints made when typing a new code.

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 analyzer warning: V501 There are identical sub-expressions '! Strcmp (a-> v.val_vms_delta.lbl1, b-> v.val_vms_delta.lbl1)' to the left and the right of the '&&' operator. dwarf2out.c 1428

Quickly seeing errors is problematic and you should take a close look. That is why the error was not detected during code reviews and refactoring.

The strcmp function compares the same strings twice. It seems to me that the second time it was necessary to compare not members of the lbl1 class, but lbl2 . Then the correct code should look like this:

 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)); 

I want to note that the code given in the article is slightly formatted so that it takes up little space on the X axis. In fact, the code looks like this:

Errors, perhaps, could have been avoided if we use the “table” code alignment. For example, an error would be easier to notice if you format the code like this:

I considered this approach in more detail in the e-book “ The Main Question of Programming, Refactoring, and All That ” (see Chapter 13: Align the same type code with a “table”). I recommend everyone who cares about the quality of their code to get acquainted with the link given here.

Let's consider another mistake that, I am sure, appeared due to Copy-Paste:

 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 analyzer warning: V519 The 'has_avx512vl' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 500, 501. driver-i386.c 501

The variable has_avx512vl twice in a row recorded various values. This makes no sense. I studied the code and found the variable has_avx512ifma . Most likely, it should be initialized by the expression ebx & bit_AVX512IFMA . Then the correct code should be:

 has_avx512vl = ebx & bit_AVX512VL; has_avx512ifma = ebx & bit_AVX512IFMA; 

Typo

I will continue the test of your attentiveness. Look at the code and, without looking below, try to find the error.

 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 analyzer warning: V528 It is odd that the pointer to 'char' type is compared with the '\ 0' value. Probably meant: * xloc.file == '\ 0'. ubsan.c 1472

Here, the programmer accidentally forgot to dereference the pointer in the xloc.file == '\ 0' expression. As a result, the pointer is simply compared with 0, i.e. with null . This has no effect, since previously such a check has already been performed: xloc.file == NULL .

It’s good that the programmer wrote the terminal zero as '\ 0'. This helps to quickly understand that the code is wrong and how to fix it. I also wrote about this in the book (see chapter N9: Use the literal '\ 0' for the terminal zero).

The correct code is:

 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 a little more. I recommend formatting the expression as follows:

 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 we make the same mistake, the chance to notice it will be slightly higher:

 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 section could also be called "the one hundred thousandth example of why macros are bad." I really do not like macros and always urge to use them less. Macros make it difficult to read the code, provoke errors, complicate the work of static analyzers. As it seemed to me from a short conversation with the GCC code, its authors are very fond of macros. I was tortured to learn what a particular macro is revealed in and perhaps that is why I missed a lot of interesting errors. I admit, I am sometimes lazy. But I will demonstrate a couple of errors related to macros.

 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 analyzer warning: V595 The 'odr_types_ptr' pointer was used against nullptr. Check lines: 2135, 2139. ipa-devirt.c 2135

See an error here? I think not, and the message of the analyzer does not bring clarity. The thing is that odr_types is not a variable name, but a macro, declared as follows:

 #define odr_types (*odr_types_ptr) 

If you expand the macro and remove all irrelevant, we get the following code:

 (*odr_types_ptr)[val->id] = 0; if (odr_types_ptr) 

In the beginning, the pointer is dereferenced, and then checked. It is difficult to say whether this will lead to trouble in practice or not. It all depends on whether there can be a situation when the pointer will really be equal to nullptr . If such a situation is impossible, then remove the extra check, which will mislead people who support the code and the code analyzer. If the pointer can be null, then this is a serious mistake that requires even more attention and correction.

Consider another similar case:

 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 analyzer warning: V595 The 'list' pointer was used before it was verified against nullptr. Check lines: 1627, 1629. sched-int.h 1627

To see the error, we again need to show the macro device:

 #define DEPS_LIST_FIRST(L) ((L)->first) 

We open the macro and get:

 it_ptr->linkp = &((list)->first); if (list) continue; 

And now many will exclaim: “Stop, stop! There is no error. We after all simply receive the pointer on a member of a class. No null pointer dereference is here. Yes, maybe the code is not accurate, but there is no error here! ”.

It is not that simple. This is where undefined behavior arises. And the fact that such code can work in practice is just luck. In fact, it is impossible to write like that. For example, an optimizing compiler, upon seeing list-> first, can remove an if (list) check. Since we executed the -> operator, it means that the pointer is not equal to nullptr . If so, then you do not need to check the pointer.

I wrote a whole article on this topic: " Dereferencing a null pointer leads to undefined behavior ." There is just considered a similar case. Before you argue, I ask you to carefully read this article.

However, the considered situation is really complicated and not obvious. I admit that I may still be wrong and there is no error here. However, so far nobody has been able to prove it to me. It will be interesting to hear the comments of the GCC developers if they pay attention to this article. They must know exactly how the compiler works and whether such code should be interpreted as erroneous or not.

Using a destroyed array

 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 analyzer warning: V507 Pointer to local array 'buf' is stored outside. 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 later in the function body. The error is that after writing the buffer to the name variable, this buffer will be destroyed.

You cannot use a pointer to a destroyed buffer. Formally, we are dealing with uncertain behavior. In practice, this code can work quite well. The correct work of the program is one of the options for the manifestation of uncertain behavior.

In any case, this code contains an error and needs to be fixed. The code can work for the reason that the compiler may find it unnecessary to use a temporary buffer for the subsequent storage of other variables or arrays. And then, although the array created on the stack is considered to be destroyed, in fact no one can touch it, and the function will correctly perform its work. That's just such luck at any time may end and the code that worked for 10 years, suddenly, when switching to a new version of the compiler, begins to behave in an amazing way.

To correct the error, it is enough to declare an array buf in the same scope as the name pointer:

 static void dump_hsa_symbol (FILE *f, hsa_symbol *symbol) { const char *name; char buf[64]; .... } 

Performing the same actions, regardless of the condition

The analyzer has detected a section of code that I definitely cannot identify as erroneous. However, it is extremely suspicious to perform a check, and then, regardless of its result, perform the same actions. Of course, perhaps this is a reserve for the future and so far everything is correct, but it is clearly worth checking out this piece of code.

 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 analyzer warning: V523 The 'then' statement is equivalent to the 'else' statement. tree-ssa-threadupdate.c 2596

If this code is erroneous, I, unfortunately, can’t guess how to fix it. This is the case when you need to be familiar with the project to make edits.

Redundant expression of the form (A == 1 || A! = 2)

 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 analyzer warning: V590 Consider inspecting this expression. The expression is misprint. gensupport.c 1640

We are interested in the condition: (alt <2 || * insn_out == '*' || * insn_out! = '@')

It can be abbreviated to: (alt <2 || * insn_out! = '@')

I would venture to suggest that the ! = Operator should be replaced with == . Then the code will take a more meaningful view:

 if (alt < 2 || *insn_out == '*' || *insn_out == '@') 

Zeroing the wrong pointer

Consider a function that frees up resources:

 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 analyzer warning: V519 The 'bb_copy' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 1076, 1078. cfg.c 1078

Pay attention to these 4 lines of code:

 delete bb_copy; bb_copy = NULL; delete bb_original; bb_copy = NULL; 

Randomly double pointer bb_copy . The correct option is:

 delete bb_copy; bb_copy = NULL; delete bb_original; bb_original = NULL; 

Assert that won't check anything

An incorrect condition that is an argument to the macro gcc_assert will not affect the correctness of the program, but will complicate the search for an error, if one occurs. Consider the code:

 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 analyzer warning: V502 Perhaps the '?:' Operator works in a different way than it was expected. The '?:' Operator has a lower limit than the '<=' operator. dwarf2out.c 2053

The priority of the ternary operator ?: Lower than that of the comparison operator <= . This means that we are dealing with a condition of the form:

 die_offset > 0 && ((die_offset <= (loc->dw_loc_opc == DW_OP_call2)) ? 0xffff : 0xffffffff); 

Thus, the second operand of the && operator can take the value 0xffff or 0xffffffff . Both of these values ​​indicate true, so the expression can be simplified to:

 (die_offset > 0) 

This is clearly not what the programmer intended. To remedy this, add a pair of parentheses:

 gcc_assert (die_offset > 0 && die_offset <= ((loc->dw_loc_opc == DW_OP_call2) ? 0xffff : 0xffffffff)); 

Operator ?: Very cunning and it is better not to use it in complex expressions. It is very easy to make a mistake. We have collected a large number of examples of such errors found by the PVS-Studio analyzer in various open source projects. More information about the operator ?: I wrote in the already mentioned book (see Chapter N4: Fear the operator?: And enclose it in parentheses).

It seems to have forgotten about the "cost"

The alg_hash_entry structure is declared as follows:

 struct alg_hash_entry { unsigned HOST_WIDE_INT t; machine_mode mode; enum alg_code alg; struct mult_cost cost; bool speed; }; 

In the synth_mult function , the programmer decided to check if this is the object he needs. For this, it needs to compare the structure fields. However, it seems there is a mistake in this place:

 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 analyzer warning: V501 There are identical sub-expressions' entry_ptr-> mode == mode 'to the left and' operator. expmed.c 2573

The mode is checked twice in a row, but there is no cost check. Perhaps one of the comparisons you just need to remove, and perhaps you need to compare the cost . It's hard for me to judge, but the code is clearly worth fixing.

Duplicate assignments

The following parts of the code, in my opinion, are not dangerous and it seems that duplicate assignment can be simply deleted.

Case N1

 type_p find_structure (const char *name, enum typekind kind) { .... structures = s; // <= s->kind = kind; s->ustag = name; structures = s; // <= return s; } 

PVS-Studio analyzer warning: V519 The 'structures' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 842, 845. gengtype.c 845

Case N2

 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 analyzer warning: V519 The 'nargs' variable is assigned values ​​twice successively. Perhaps this is a mistake. Check lines: 39951, 39952. i386.c 39952

Case N3

The latter case is more strange than the rest. Perhaps there is some kind of mistake. Variable steptype value is assigned 2 or 3 times. This is suspicious.

 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 analyzer warning: 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 wrote this article. Now I have something to respond to comments like “PVS-Studio is not needed, since GCC gives all the same warnings”. As you can see, PVS-Studio is a very powerful tool and is superior in diagnostic capabilities to GCC. I do not deny that excellent diagnostics are implemented in GCC. This compiler, if properly configured, does reveal many problems in the code. But PVS-Studio is a specialized and rapidly developing tool, which means it will always be better to identify errors in the code than compilers do.

I invite you to get acquainted with the checks of other well-known open source projects by visiting this section of our site. And also, those who use Twitter, follow me @Code_Analysis . I regularly publish links to interesting articles on programming in C and C ++, as well as talking about new achievements of our analyzer.


If you want to share this article with an English-speaking audience, then please use the link to the translation: Andrey karpov. Bugs found in GCC with the help of PVS-Studio .