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

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

Gcc

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

The GCC compiler has good built-in diagnostics to help identify many errors at the compilation stage. Naturally, GCC is built using GCC and, accordingly, can detect errors in its own code. Additionally, GCC source code is verifiedusing the Coverity analyzer. Anyway, I think GCC was tested by enthusiasts using many analyzers and other tools. This makes the GCC bug search a big test for the PVS-Studio code analyzer.

For analysis, the trunk version was taken from the git repository : (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 identify 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 quote from Wikipedia in Russian:

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

I consider such statements unfounded. Yes, perhaps the GCC code contains many macros that make it difficult to read. But I just can not agree with the statement about his buggy. If the GCC were buggy, nothing would work anywhere at all. You just remember how many programs it compiles and works successfully. The creators of GCC do a huge, complex 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 cooler, let me remind you: PVS-Studio also found errors in it: 1 , 2 .

PVS-Studio

I checked the GCC code with the Alpha version of the PVS-Studio for Linux analyzer. We plan to start issuing the Beta version of the analyzer to interested programmers in mid-September 2016. You will find instructions on how to become one of the first to try the Beta version of PVS-Studio for Linux on your project in the article " PVS-Studio Declares 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/en/pvs-studio/

Validation Results

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

Unfortunately, I cannot give the compiler developers a full report. There is still too much garbage (false positives) connected with the fact that the analyzer is not completely ready to meet the Linux world. We need to do work to reduce the number of false warnings on the typical designs used. I’ll try to explain with one simple example. Many diagnostics do not have to swear on 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, the macro assert is definedin very different ways, and you need to train the analyzer in all typical options.

Therefore, I ask GCC developers to wait for the release of at least the Beta version of the analyzer. I do not want to spoil the impression with the report generated by the unfinished version.

Classics (Copy-Paste)

We will start with the most classic and common mistake that is detected using the V501 diagnostic . As a rule, such errors appear due to inattention during Copy-Paste or simply are typos allowed 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 to the right of the '&&' operator. dwarf2out.c 1428

Quickly see errors is problematic and you should carefully look. That is why the error was not detected during code reviews and refactoring.

The strcmp function compares the same lines twice. It seems to me that the second time it was necessary to compare not members of the class lbl1 , 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 in the article is slightly formatted so that it takes up little space on the X axis. Actually, the code looks like this:

Mistakes, perhaps, could have been avoided if we used “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 N13: Align the same type of code with a “table”). I recommend everyone who cares about the quality of their code to get acquainted with the link provided here.

Let's look at another error that I am sure came up 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 Various values ​​are written to the has_avx512vl

variable twice in a row. It does not make sense. I studied the code and discovered the has_avx512ifma variable . Most likely, it should be initialized with the expression ebx & bit_AVX512IFMA . Then the correct code should be like this:

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

Typo

I will continue the test of your mindfulness. Look at the code and, without peeping below, try to find an 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 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 expression xloc.file == '\ 0' . As a result, the pointer is simply compared to 0, i.e. with NULL . This has no effect, since previously such a check has already been performed: xloc.file == NULL .

It is good that the programmer wrote down 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' to denote terminal zero).

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 bit more. I recommend formatting the expression like this:

if (   xloc.file == NULL
    || strncmp (xloc.file, "\1", 2) == 0
    || xloc.file[0] == '\0'
    || xloc.file[0] == '\xff'
    || xloc.file[1] == '\xff')
  return false;

Please note: now, if you 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 Dereferencing

This section could also be called "the one hundred thousandth example, why macros are bad." I really dislike macros and always encourage less use of them. Macros make it difficult to read code, cause errors, complicate the work of static analyzers. It seemed to me from a brief conversation with the GCC code that its authors are very fond of macros. I was tormented to study what this or that macro is revealed in, and maybe that's why I missed a lot of interesting errors. I admit, I am sometimes lazy. But I’ll still 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 utilized before it was verified against nullptr. Check lines: 2135, 2139. ipa-devirt.c 2135

See an error here? I think not, and the analyzer’s message 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 we expand the macro and remove everything that is irrelevant, we get the following code:

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

At the beginning, the pointer is dereferenced, and then checked. It will be difficult to say whether this will lead to trouble in practice or not. It all depends on whether a situation may arise when the pointer is really nullptr . If this situation is impossible, then you should 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 utilized 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 mistake. We just get a pointer to a member of the class. There is no dereferencing of the null pointer here. Yes, maybe the code is not accurate, but there is no error here! ”

It is not that simple. Undefined behavior arises here. And the fact that such code can work in practice is just luck. In fact, you can’t write like that. For example, the optimizing compiler, having seen list-> first, can remove the if (list) check . Since we executed the -> operator , it means that the pointer is not nullptr . If so, then you do not need to check the pointer.

I wrote an entire article on this subject: "The dereferencing of a null pointer leads to undefined behavior . "There is a similar case there. Before arguing, I ask you to carefully read this article.

However, the situation considered is really complicated and not obvious. I assume that I can still be wrong and there is no mistake However, so far no one has been able to prove this to me. It will be interesting to hear the comments of the GCC developers if they pay attention to this article. They really need to know how the compiler works and whether it should be interpreted. To write such code 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 the scope of this array. Such a pointer will become invalid. hsa-dump.c 704

A 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 variable name , this buffer itself will be destroyed.

You cannot use a pointer to a destroyed buffer. Formally, we are dealing with indefinite behavior. In practice, this code can work quite successfully. The correct operation of the program is one of the manifestations of undefined behavior.

In any case, this code contains an error and needs to be fixed. The code may work for the reason that the compiler may consider it unnecessary to use a temporary buffer for 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 do its job correctly. That's just such luck at any moment that can end and the code that worked for 10 years, suddenly, when switching to a new version of the compiler, starts to behave in an amazing way.

To fix the error, just declare the buf array 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 condition

The analyzer detected a section of code that I cannot unambiguously identify as an error. However, it is extremely suspicious to perform a check, and then, regardless of its result, perform the same actions. Of course, perhaps this has touched the future and so far everything is correct, but it’s clearly worth checking this section of the 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, have no idea how to fix it. This is the case when you need to be familiar with the project to make an amendment.

Excessive 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 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 venture to assume that the operator ! = Should be replaced by == . Then the code will take a more meaningful look:

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

Zeroing the wrong pointer

Consider the function of freeing 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;

The bb_copy pointer is accidentally nullified twice . The correct option:

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

Assert that will not check anything

An incorrect condition, which is an argument to the gcc_assert macro, will not affect the correct operation of the program, but will complicate the search for an error, if any. 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 priority than the '<=' operator. dwarf2out.c 2053 The

priority of the ternary operator ?: is 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 mean true, so the expression can be simplified to:

(die_offset > 0)

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

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

Operator ?: Is very insidious and it is better not to use it in complex expressions. It's 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 book already mentioned earlier (see chapter N4: Fear the operator?: And enclose it in parentheses).

They seem to have forgotten about "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 that he needs. To do this, he needs to compare the structure fields. However, there seems to be 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 to the right of the '&&' operator. expmed.c 2573

Two times in a row is checked mode , but no test cost . Perhaps one of the comparisons just needs to be removed, or maybe you need to compare cost . It's hard for me to judge, but the code is clearly worth fixing.

Assignment Duplicates

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

Case N1

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 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 last case is stranger 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 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 GCC has excellent diagnostics. This compiler, when properly configured, does reveal a lot of problems in the code. But PVS-Studio is a specialized and rapidly developing tool, which means it will always be better to detect 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, for those using Twitter, follow me @Code_Analysis. I regularly publish links to interesting articles on programming in C and C ++, and also talk 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 .