GDB is a tool that is hard to live without. Of course, as Brooks says: "The quality in software development is achieved through proper design, not by endless testing". However, proper design doesn't protect from logical errors, typos, null pointers, and so on. That's why various debugging tools, like GDB, come to help. My aim is to show that static code analyzers are also very useful tools which help to detect errors at the early stages of development. It's much better if the bug is fixed in the code before the testing and debugging stage. To demonstrate the benefits of static code analysis, let's delve inside GDB and look for errors, using PVS-Studio.
Having already written an article about checking GCC, I've decided to write an article about GDB as well. But this time it was much harder to do. Apparently, the size of the projects played its role. However, it's not that easy to compare the size of the code base. Both projects have files containing large tables of data. They contribute significantly to the size of the code, and the number of lines in it. For example, in the GDB project there is a file i386-tbl.h, 5Mb, having a table like this:
I think there is several times more real code in GCC than the size of GDB code. Checking GCC, I could easily get a decent amount of errors to write an article, just by skimming the code and not digging into the suspicious parts, but there was difficultly in understanding code fragments. In the case of GDB I had to look very carefully and I was still only able to find very few suspicious places.
I checked the GDB source code, version 7.11.1. The code was checked with a PVS-Studio version working under Linux.
A quick reference. PVS-Studio is a commercial static analyzer which detects errors in source code written in C, C++, and C#. It works in both the Linux and Windows environment.
To check GDB with the help of PVS-Studio static code analyzer, we need to follow several simple steps.
0) Read the documentation: How to run PVS-Studio on Linux. I have chosen a way that allows checking of the project without the analyzer integration to the build system.
1) Download the latest version of the source code from the official repository:
$ git clone git://sourceware.org/git/binutils-gdb.git2) Change the configuration file PVS-Studio.cfg, and namely, the parameters output-file and sourcetree-root. In my case:
exclude-path = /usr/include/
exclude-path = /usr/lib64/
lic-file = /home/andr/PVS-Studio.lic
output-file = /home/andr/gdb.log
sourcetree-root = /home/andr/binutils-gdb3) Go to the downloaded directory:
$ cd binutils-gdb4) Create Makefile:
$ ./configureStart gdb building and PVS-Studio analyzer:
$ pvs-studio-analyzer trace -- make -j36) Run the analysis (by specifying the path to the configuration file PVS-Studio.cfg)
$ pvs-studio-analyzer analyze --cfg /home/andr/PVS-Studio.cfgAfter successful completion of the analysis, there will appear in the home directory, a log-file gdb.log, which can be viewed in Windows with the help of the Standalone utility. I did it exactly this way, as it was very convenient for me.
If you want to view the report in Linux, the utility-converter (plog-converter) will help you; the source code is also included in the PVS-Studio distribution kit. The utility can convert *.plog files to different formats (see the documentation). Now you can customize the converter so that it meets your demands.
Important. Please, don't try opening the *.log in a text editor. It will be awful. This file contains a lot of unnecessary and duplicate information; that's why these files are so large. For example, if some warning is related to the h-file, you still see it as many times as this h-file is included in the cpp-files. When you use PVS-Studio Standalone or plog-converter, these tools will automatically remove such duplicates.
Let's say you like viewing the report in Qt Creator, converting the *.log file to the format Qt Task List File. Then we should use the plog-converter utility as follows:
$ plog-converter -t tasklist -o /home/andr/gdb.tasks
-r /home/andr/binutils-gdb/ -a GA:1,2,3 /home/andr/gdb.logAlthough, for a start, it would be better to use GA:1,2. It's not the best idea to start the acquaintance with the analyzer by turning on all three levels of warnings.
After you run this command, the report file gdb.tasks will appear in the home directory, which can be viewed with the help of Qt Creator:
Viewing the converter options:
$ plog-converter --helpAs I have already said, this time I was able to find just a few bugs, demonstrating the capabilities of PVS-Studio. The reason for this is the high quality of the source code of the GDB project, and the fact that it is very well tested by a large number of users who are programmers themselves, which means that they are more demanding and attentive than average program users.
Let's see which interesting errors I was able to find. Let's start with the error in the comparison function. I can call this a new error pattern. I come across such mistakes in a large number of projects, and soon plan to write a new article on this topic, which will resemble "Last line effect".
Incorrect comparison function
static int
psymbol_compare (const void *addr1, const void *addr2, int length)
{
struct partial_symbol *sym1 = (struct partial_symbol *) addr1;
struct partial_symbol *sym2 = (struct partial_symbol *) addr2;
return (memcmp (&sym1->ginfo.value, &sym1->ginfo.value,
sizeof (sym1->ginfo.value)) == 0
&& sym1->ginfo.language == sym2->ginfo.language
&& PSYMBOL_DOMAIN (sym1) == PSYMBOL_DOMAIN (sym2)
&& PSYMBOL_CLASS (sym1) == PSYMBOL_CLASS (sym2)
&& sym1->ginfo.name == sym2->ginfo.name);
}PVS-Studio warning: V549 The first argument of 'memcmp' function is equal to the second argument. psymtab.c 1580
The first and second arguments are the function memcmp(), and are the same. Apparently, the programmer wanted to write:
memcmp (&sym1->ginfo.value,
&sym2->ginfo.value,
sizeof (sym1->ginfo.value))Incorrect code that works correctly
Static code analyzers work with the source code of programs, and can find such fragments which are certainly a mistake from the point of view of a human. Interestingly, despite this error in the code, and thanks to a stroke of luck, the code can work completely correctly. Let's look at one of these interesting cases.
struct event_location *
string_to_explicit_location (const char **argp, ....)
{
....
/* It is assumed that input beginning with '-' and a non-digit
character is an explicit location. "-p" is reserved, though,
for probe locations. */
if (argp == NULL
|| *argp == '\0'
|| *argp[0] != '-'
|| !isalpha ((*argp)[1])
|| ((*argp)[0] == '-' && (*argp)[1] == 'p'))
return NULL;
....
}PVS-Studio warning: V528 It is odd that pointer to 'char' type is compared with the '\0' value. Probably meant: ** argp == '\0'. location.c 527
We are interested in the following fragment of the code:
.... const char **argp ....
if (argp == NULL
|| *argp == '\0'
|| *argp[0] != '-'Literal '\0' is a terminal null, which is used when it is neccesary to check if the string is empty or not. To do this, the programmer checks the first buffer element, containing the string, and if there is a terminal null, then the string is considered to be empty. This is exactly what the programmer wanted here. But it didn't take into account that the variable argp is not a pointer to characters, but a pointer to a pointer.
Therefore, the correct check should be like this:
*argp[0] == '\0'
Or like this:
**argp == '\0'
However, if we write code like this
if (argp == NULL
|| *argp[0] == '\0'
|| *argp[0] != '-'then it is dangerous. We need to add one more check to the null pointer:
if (argp == NULL
|| *argp == NULL
|| *argp[0] == '\0'
|| *argp[0] != '-'Now the code is correct. But be aware that it is redundant. If the first character is not a dash '-' then it doesn't matter what kind of symbol it is. There is no difference, if there is a terminal null, or any other symbol. This is why we can simplify the code in the following way:
if (argp == NULL
|| *argp == NULL
|| *argp[0] != '-'Pay attention to the fact that this correct code is equivalent to the original code:
if (argp == NULL
|| *argp == '\0'
|| *argp[0] != '-'The difference is in the way 0 is written. In the first case, it is NULL. In the second it is '\0'. In general this is just the same and the code behaves in the same way.
Quite funny. Despite the fact that the code was written incorrectly, it works absolutely correctly.
Incorrect evaluation of buffer size
extern void
read_memory (CORE_ADDR memaddr, gdb_byte *myaddr, ssize_t len);
void
java_value_print (....)
{
....
gdb_byte *buf;
buf = ((gdb_byte *)
alloca (gdbarch_ptr_bit (gdbarch) / HOST_CHAR_BIT));
....
read_memory (address, buf, sizeof (buf));
....
}PVS-Studio warning: V579 The read_memory function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the third argument. jv-valprint.c 111
This error most likely occurred during refactoring. I would venture to guess that at some point the code was something like this:
gdb_byte buf[gdbarch_ptr_bit (gdbarch) / HOST_CHAR_BIT)];
....
read_memory (address, buf, sizeof (buf));The sizeof() operator evaluated the buffer size correctly. Then, the programmer started allocating the memory for the buffer, with the help of the alloca() function. As a result, the sizeof(buf) operator evaluates not the buffer size, but the pointer size.
I think the correct code should be like this:
gdb_byte *buf;
const size_t size = gdbarch_ptr_bit (gdbarch) / HOST_CHAR_BIT;
buf = ((gdb_byte *) alloca (size));
....
read_memory (address, buf, size);But that's not the end of it, the funny part is still to come. I just decided to explain the essence of this error, and the way it could get there. Everything gets way more interesting if we have a look at several code lines:
read_memory (address, buf, sizeof (buf));
address += gdbarch_ptr_bit (gdbarch) / HOST_CHAR_BIT;
/* FIXME: cagney/2003-05-24: Bogus or what. It
pulls a host sized pointer out of the target and
then extracts that as an address (while assuming
that the address is unsigned)! */
element = extract_unsigned_integer (buf, sizeof (buf),
byte_order);As you can see, I am not the first one who noticed that something is wrong with this code. The error has lived in this code since at least 2003. It's really unclear why it hasn't been fixed yet.
As I understand, the comment is related to the string:
element = extract_unsigned_integer (buf, sizeof (buf),
byte_order);Upon the call of the function extract_unsigned_integer(), the same error was made as the one I had described above.
PVS-Studio issues a warning for this string: V579 The extract_unsigned_integer function receives the pointer and its size as arguments. It is possibly a mistake. Inspect the second argument. jv-valprint.c 117
The analyzer issues two more warnings for the code of the functions java_value_print():
Double assignment
FILE *
annotate_source (Source_File *sf, unsigned int max_width,
void (*annote) (char *, unsigned int, int, void *),
void *arg)
{
....
bfd_boolean new_line;
....
for (i = 0; i < nread; ++i)
{
if (new_line)
{
(*annote) (annotation, max_width, line_num, arg);
fputs (annotation, ofp);
++line_num;
new_line = FALSE;
}
new_line = (buf[i] == '\n');
fputc (buf[i], ofp);
}
....
}PVS-Studio warning: V519 The 'new_line' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 253, 256. source.c 256
New_line string = FALSE; It makes no sense. Right after it, the value of the variable new_line gets rewritten by another value. So, this code fragment is extremely suspicious:
new_line = FALSE;
}
new_line = (buf[i] == '\n');Apparently, we have a logical error here. Or the first assignment is just redundant, and we can delete it.
A typo
int
handle_tracepoint_bkpts (struct thread_info *tinfo, CORE_ADDR stop_pc)
{
int ipa_trace_buffer_is_full;
CORE_ADDR ipa_stopping_tracepoint;
int ipa_expr_eval_result;
CORE_ADDR ipa_error_tracepoint;
....
if (ipa_trace_buffer_is_full)
trace_debug ("lib stopped due to full buffer.");
if (ipa_stopping_tracepoint)
trace_debug ("lib stopped due to tpoint");
if (ipa_stopping_tracepoint)
trace_debug ("lib stopped due to error");
....
}PVS-Studio warning: V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 4535, 4537. tracepoint.c 4537
If the variable ipa_stopping_tracepoint is TRUE, then two debugging messages will be printed:
lib stopped due to tpoint
lib stopped due to errorI am not familiar with the principle of the code work, but it seems that in the last case in the condition, the variable ipa_error_tracepoint should be used, not ipa_stopping_tracepoint. Then the code will be like this:
if (ipa_trace_buffer_is_full)
trace_debug ("lib stopped due to full buffer.");
if (ipa_stopping_tracepoint)
trace_debug ("lib stopped due to tpoint");
if (ipa_error_tracepoint)
trace_debug ("lib stopped due to error");Forgotten break statement
A classic mistake. Break operator was forgotten inside a switch in one fragment.
static debug_type
stab_xcoff_builtin_type (void *dhandle, struct stab_handle *info,
int typenum)
{
....
switch (-typenum)
{
....
case 8:
name = "unsigned int";
rettype = debug_make_int_type (dhandle, 4, TRUE);
break;
case 9:
name = "unsigned";
rettype = debug_make_int_type (dhandle, 4, TRUE);
case 10:
name = "unsigned long";
rettype = debug_make_int_type (dhandle, 4, TRUE);
break;
....
}
....
}PVS-Studio warning: V519 The 'name' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 3433, 3436. stabs.c 3436
Regardless of the fact that we work with the "unsigned" or "unsigned long", we will assign the type the name "unsigned long".
Correct code:
case 9:
name = "unsigned";
rettype = debug_make_int_type (dhandle, 4, TRUE);
break;A complicated case
In the given code, the alt variable gets assigned a value twice, because of the missing break operator between two case. But according to the commentary, the programmer doesn't use break on purpose. Let's take a look at the code which seems confusing to me.
static int
putop (const char *in_template, int sizeflag)
{
int alt = 0;
....
switch (*p)
{
....
case '{':
alt = 0;
if (intel_syntax)
{
while (*++p != '|')
if (*p == '}' || *p == '\0')
abort ();
}
/* Fall through. */
case 'I':
alt = 1;
continue;
....
}
}PVS-Studio warning: V519 The 'alt' variable is assigned values twice successively. Perhaps this is a mistake. Check lines: 14098, 14107. i386-dis.c 14107
So, the comment /* Fall through. */ is saying that the break operator is not needed here at all. But then it is not clear why the variable alt is assigned with the value 0. In any case the value of the variable is replaced by a one. Between these two variable assignment alt is not used in any way. It's just not clear...
There is either a logical error here, or else, the first assignment should be removed.
Long live PVS-Studio for Linux! As you see, now I can not only show the benefits of static analysis for Windows open projects, but also help the Linux-community open programs. I reckon that soon our list of articles about checked projects, will include a large number of articles about programs from the Linux world.
I also invite you to follow me on Twitter, so that you don't miss something interesting @Code_Analysis.