Check with static code analyser #3342

Closed
kostyfisik opened this Issue Nov 2, 2016 · 43 comments

Projects

None yet

5 participants

@kostyfisik
Contributor
kostyfisik commented Nov 2, 2016 edited

I have a one week trial of PVS-studio, there are quite a number places it the code that looks at least strange and some of them are for sure should be considered as a bug and probably should be inspected with some experienced developer. A short list:

/home/tig/dealii/dealii-git/include/deal.II/base/exceptions.h	42	warn	V690 The 'ExceptionBase' class implements a copy constructor, but lacks the '=' operator. It is dangerous to use such a class. 
/home/tig/dealii/dealii-git/include/deal.II/base/thread_management.h	2944	warn	V678 An object is used as an argument to its own method. Consider checking the first actual argument of the 'destroy' function. 
/home/tig/dealii/dealii-git/include/deal.II/algorithms/newton.templates.h	131	err	V672 There is probably no need in creating the new 'out' variable here. One of the function's arguments possesses the same name and this argument is a reference. Check lines: 96, 131. 
/home/tig/dealii/dealii-git/include/deal.II/lac/block_vector_base.h	214	warn	V690 The 'Iterator' class implements the '=' operator, but lacks a copy constructor. It is dangerous to use such a class. 
/home/tig/dealii/dealii-git/include/deal.II/lac/block_vector_base.h	2052	err	V568 It's odd that the argument of sizeof() operator is the 'this->n_blocks()' expression. 
/home/tig/dealii/dealii-git/include/deal.II/base/thread_management.h	2918	warn	V730 Not all members of a class are initialized inside the constructor. Consider inspecting: task_is_done. 
/home/tig/dealii/dealii-git/bundled/boost-1.62.0/include/boost/signals2/last_value.hpp	70	err	V607 Ownerless expression '* first'. 
/opt/centos/devtoolset-1.1/root/usr/include/c++/4.7.2/array	130	err	V557 Instantiate std_cxx11::array < char, 30 >: Array overrun is possible. The '_Nm' index is pointing beyond array bound./home/tig/dealii/dealii-git/bundled/boost-1.62.0/include/boost/archive/iterators/mb_from_wchar.hpp	125	err	V730 Not all members of a class are initialized inside the constructor. Consider inspecting: m_mbs. 
/home/tig/dealii/dealii-git/source/grid/cell_id.cc	25	err	V730 Not all members of a class are initialized inside the constructor. Consider inspecting: child_indices. 
/home/tig/dealii/dealii-git/source/multigrid/mg_level_global_transfer.cc	451	err	V758 The 'global_partitioner' reference becomes invalid when smart pointer returned by a function is destroyed. 
/home/tig/dealii/dealii-git/source/grid/grid_generator.cc	1740	warn	V560 A part of conditional expression is always true: dim > 1. 
/home/tig/dealii/dealii-git/include/deal.II/matrix_free/mapping_info.templates.h	393	warn	V581 The conditional expressions of the 'if' operators situated alongside each other are identical. Check lines: 391, 393. 
/home/tig/dealii/dealii-git/source/fe/fe_abf.cc	331	warn	V688 The 'cached_values' local variable possesses the same name as one of the class members, which can result in a confusion. 
/home/tig/dealii/dealii-git/include/deal.II/matrix_free/fe_evaluation.h	2665	err	V621 Consider inspecting the 'for' operator. It's possible that the loop will be executed incorrectly or won't be executed at all. 
/home/tig/dealii/dealii-git/source/grid/grid_in.cc	296	err	V663 Infinite loop is possible. The 'cin.eof()' condition is insufficient to break from the loop. Consider adding the 'cin.fail()' function call to the conditional expression. 
/home/tig/dealii/dealii-git/source/grid/grid_out.cc	1475	warn	V656 Variables 'x_min', 'x_max' are initialized through the call to the same function. It's probably an error or un-optimized code. Consider inspecting the 'tria.begin()->vertex(0)[0]' expression. Check lines: 1474, 1475. 
/home/tig/dealii/dealii-git/source/grid/grid_out.cc	3220	err	V595 The 'q_projector' pointer was utilized before it was verified against nullptr. Check lines: 3220, 3250. 
/home/tig/dealii/dealii-git/bundled/boost-1.62.0/include/boost/math/policies/policy.hpp	1026	err	V562 It's odd to compare a bool type value with a value of 3: t1::value != user_error. 
/home/tig/dealii/dealii-git/bundled/boost-1.62.0/include/boost/math/special_functions/detail/fp_traits.hpp	304	err	V512 A call of the 'memcpy' function will lead to underflow of the buffer '& x'. 
/home/tig/dealii/dealii-git/bundled/boost-1.62.0/include/boost/math/constants/calculate_constants.hpp	693	warn	V760 Two identical blocks of text were found. The second block begins from line 701. 
/home/tig/dealii/dealii-git/bundled/boost-1.62.0/include/boost/math/special_functions/factorials.hpp	169	err	V547 Expression 'n >= 0' is always true. Unsigned type value is always >= 0. 
/home/tig/dealii/dealii-git/include/deal.II/lac/vector_operations_internal.h	254	err	V523 The 'then' statement is equivalent to the 'else' statement. 
/home/tig/dealii/dealii-git/include/deal.II/lac/la_parallel_vector.templates.h	59	warn	V728 An excessive check can be simplified. The '||' operator is surrounded by opposite expressions 'val == 0' and 'val != 0'.  
/home/tig/dealii/dealii-git/bundled/boost-1.62.0/include/boost/multi_index_container.hpp	843	err	V667 The 'throw' operator does not possess any arguments and is not situated within the 'catch' block. 
/home/tig/dealii/dealii-git/include/deal.II/fe/fe_dgp_nonparametric.h	602	warn	V703 It is odd that the 'degree' field in derived class 'FE_DGPNonparametric' overwrites field in base class 'FiniteElementData'. Check lines: fe_dgp_nonparametric.h:602, fe_base.h:296. 
/home/tig/dealii/dealii-git/bundled/boost-1.62.0/include/boost/unordered/detail/equivalent.hpp	53	warn	V690 The '=' operator is declared as private in the 'grouped_ptr_node' class, but the default copy constructor will still be generated by compiler. It is dangerous to use such a class. 
/home/tig/dealii/dealii-git/bundled/boost-1.62.0/include/boost/graph/graph_concepts.hpp	549	err	V530 The return value of function 'infinity' is required to be utilized. 
/home/tig/dealii/dealii-git/source/base/quadrature.cc	676	warn	V636 The 'subface_no / 2' expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;. 
/home/tig/dealii/dealii-git/source/grid/manifold_lib.cc	486	warn	V592 The expression was enclosed by parentheses twice: ((expression)). One pair of parentheses is unnecessary or misprint is present. 
/home/tig/dealii/dealii-git/source/base/tensor_product_polynomials.cc	316	err	V596 The object was created but it is not being used. The 'throw' keyword could be missing: throw ExcNotImplemented(FOO); 
/home/tig/dealii/dealii-git/source/grid/tria.cc	6378	warn	V506 Pointer to local variable 'lines_x' is stored outside the scope of this variable. Such a pointer will become invalid. 
/home/tig/dealii/dealii-git/source/lac/sparsity_pattern.cc	234	err	V501 There are identical sub-expressions 'cols == 0' to the left and to the right of the '&&' operator. 
/home/tig/dealii/dealii-git/source/fe/fe_values.cc	2383	err	V523 The 'then' statement is equivalent to the 'else' statement. 
/home/tig/dealii/dealii-git/source/numerics/time_dependent.cc	866	warn	V636 The expression was implicitly cast from 'int' type to 'double' type. Consider utilizing an explicit type cast to avoid the loss of a fractional part. An example: double A = (double)(X) / Y;.

Full log file is attached. Checked after commit
" 0e395a1 2016-11-01 | Merge pull request #3339 from Rombur/exception"
pvs.txt

@drwells
Member
drwells commented Nov 2, 2016

I have seen PVS studio before but I never bothered to configure it to do this. Thanks!

I'll post again when I have read this more carefully.

@bangerth
Member
bangerth commented Nov 2, 2016

Ah, that is very interesting. I bet that almost all of them are not really problems, but we should try to address them anyway or at least go through them.

There are reports about 161 files (cat Downloads/pvs.txt | perl -p -e 's/\t[0-9]+[ \t]*(err|warn).*//g;' | sort | uniq | wc -l) of which 36 are in source/ and 31 are in include/. The rest are in bundled/ or in system files that we will likely not want to change.

I think it would be quite interesting to create individual github issues for each of these 36+31 files. This would make it possible to track which ones have been addressed and which still need someone to look at it. @kostyfisik -- would you be interested in doing this? I bet you could use the hub program to script this (https://github.com/github/hub). If you use a title such as "Static analysis: source/grid/tria.cc" then they would be easy to search for. If you do that, also put a text such as "In reference to #3342" into the body so that it links back to here.

If you don't feel confident opening so many issues by a script, let me know and I can help out.

@bangerth
Member
bangerth commented Nov 2, 2016

Start of a script:

FILES=`cat Downloads/pvs.txt | perl -p -e 's/\t[0-9]+[ \t]*(err|warn).*//g;' | sort | uniq | egrep '/home/tig/dealii/dealii-git/(include|source)'`
for file in $FILES ; do 
   echo "========= $file" ; 
   grep $file Downloads/pvs.txt ; 
done

The body of the loop would need to take the output of grep and pass it to hub.

@kostyfisik
Contributor

Hub looks to be hard for a "do\forget" cycle. I found python script https://gist.github.com/JeffPaine/3145490 to create issues, however, I am overloaded at the moment (and will be for next 3 weeks), so probably someone else can script it in a proper way. I have done the scan just because I was reading news and found that due to release of Linux version of PVS they will provide a trial to anyone, you just need to e-mail them. And it was very easy to do the scan, just few commands. http://www.viva64.com/en/m/0036/

@kostyfisik
Contributor

BTW, https://scan.coverity.com/github can be used for automated static code analysis with github integration as a Travis job...

This was referenced Nov 2, 2016
@bangerth
Member
bangerth commented Nov 2, 2016

OK, enough spam -- github blocked me after these first few issues with a message that says "You have triggered an abuse detection mechanism and have been temporarily blocked from content creation. Please retry your request again later."

Anyway, here's the script if someone wants to take over:

for file in $FILES ; do echo "========= $file" ; ./ghi open -m "Static analysis: $file

\`\`\`
`grep $file ~/Downloads/pvs.txt`
\`\`\`

We should address these warnings and errors from the static analysis tool PVS. In response to #3342." -L"Low priority" -L"Starter project" ; done
@bangerth
Member
bangerth commented Nov 2, 2016

And the ghi script is from here:

curl -sL https://raw.githubusercontent.com/stephencelis/ghi/master/ghi > ghi
@bangerth
Member
bangerth commented Nov 2, 2016

The following entries of $FILES were not processed because of the error:

dealii-git/source/base/function_parser.cc
dealii-git/source/base/logstream.cc
dealii-git/source/base/quadrature.cc
dealii-git/source/base/quadrature_lib.cc
dealii-git/source/base/tensor_product_polynomials.cc
dealii-git/source/base/timer.cc
dealii-git/source/dofs/dof_tools_constraints.cc
dealii-git/source/fe/fe_abf.cc
dealii-git/source/fe/fe_nedelec.cc
dealii-git/source/fe/fe_q_base.cc
dealii-git/source/fe/fe_q_hierarchical.cc
dealii-git/source/fe/fe_raviart_thomas.cc
dealii-git/source/fe/fe_values.cc
dealii-git/source/fe/mapping_cartesian.cc
dealii-git/source/fe/mapping_fe_field.cc
dealii-git/source/fe/mapping_manifold.cc
dealii-git/source/fe/mapping_q_generic.cc
dealii-git/source/grid/cell_id.cc
dealii-git/source/grid/grid_generator.cc
dealii-git/source/grid/grid_in.cc
dealii-git/source/grid/grid_out.cc
dealii-git/source/grid/manifold_lib.cc
dealii-git/source/grid/tria.cc
dealii-git/source/lac/solver_control.cc
dealii-git/source/lac/sparse_direct.cc
dealii-git/source/lac/sparsity_pattern.cc
dealii-git/source/lac/sparsity_tools.cc
dealii-git/source/multigrid/mg_level_global_transfer.cc
dealii-git/source/multigrid/multigrid.cc
dealii-git/source/numerics/data_out.cc
dealii-git/source/numerics/data_out_rotation.cc
dealii-git/source/numerics/time_dependent.cc
@kostyfisik
Contributor

Add a random delay between submissions 1h+rand(20min)? According to existing history 30 issues a day are a valid number

@Rombur
Member
Rombur commented Nov 2, 2016

The limit is 60 requests per hour https://developer.github.com/v3/#rate-limiting

@kostyfisik
Contributor

This way two min delay should be for sure safe :)

As soon as @bangerth actively started to remove "smell" from detected places - I am going to run the analyzer once more, probably on 7th November, just before my trial period expires.

It would be nice if you can put some label on issues, where the analyzer was mistaken like here #3378 I will forward this cases to PVS devs, so the next time someone will try to run PVS against deal.ii this will not give a false positive.

@bangerth
Member
bangerth commented Nov 3, 2016

I can try to mention here ones that were mistaken, but I think the majority are actually valid points.

This was referenced Nov 3, 2016
@bangerth
Member
bangerth commented Nov 3, 2016

We should also see how far we can get within a week in addressing these issues. Any help appreciated!

@kostyfisik
Contributor

The second mentioning does not work. Mentioning #3378 again. I believe this is a good thing to provide back a short report to PVS on this usage. For sure they would prefer someone buying a license, however, some knowledge on possible weak sides of PVS should also be valuable.

@bangerth
Member
bangerth commented Nov 3, 2016

#3410, #3411, #3412 are other corner cases where we cannot do anything (although the warning was not wrong).

@kostyfisik
Contributor

The updated check attached. The diff is really impressive! It looks like that I have 2 days of trial left, so I will try not to forget to run the check about 40 hrs later...

pvs2.txt

revision checked: fd6ab4 2016-11-07 | Merge pull request #3485 from bangerth/remove-duplicate-variable (HEAD, origin/master, origin/HEAD, master) [Denis Davydov]

Issues with some disscusion about why not to fix warnings:
#3360 #3363 #3367 #3370 #3378 #3402 #3408 #3411 #3423

@drwells
Member
drwells commented Nov 7, 2016

It looks like PVS is still complaining about #3356. Any idea?

Nice work everyone 🎉

@bangerth
Member
bangerth commented Nov 7, 2016

Yes, we've made good progress on closing these -- nice teamwork :-)

@kostyfisik -- is it complicated to set up running PVS on a project? Would you be interested on running it also on ASPECT? (http://aspect.dealii.org, https://github.com/geodynamics/aspect) I will fully understand if you don't want to do it, though!

@kostyfisik
Contributor

@bangerth The PVS setup on Linux is very simple - install PVS binary (yum install pvs-studio-VERSION.rpm) and (using my path):

  cd dealii-git/
  git pull
  cd ..
  cd dealii-build/
  rm -rf *
  cmake -DCMAKE_INSTALL_PREFIX=/home/tig/dealii/dealii /home/tig/dealii/dealii-git
  cp ~/jade/PVS-Studio.lic ./
  pvs-studio-analyzer trace -- make -j12 install
  pvs-studio-analyzer analyze -l PVS-Studio.lic -o pvs.log -j12
  plog-converter -a GA:1,2 -t tasklist -o pvs2.txt pvs.log

The problem is that I was spent 10 minutes to cmake Trilinos without any success, as soon as I need to be able to compile Aspect beforehand. I had downloa cmake -DCMAKE_INSTALL_PATH=/home/tig/trilinos /home/tig/trilinos-rel and make leads to nothing.

Actually it should not be a problem to get a Linux trial now, PVS devs looks to be quite friendly. Follow "contact" link in the annoncement http://www.viva64.com/en/b/0441/

@bangerth
Member
bangerth commented Nov 7, 2016

OK, thanks for the detailed instructions. I may try this sometime over the winter break :-)

@kostyfisik
Contributor

@bangerth I have alreade mentioned, here is the link to Coverty static analyzer https://scan.coverity.com/github It can be run as Travis-CI job to do analysis as you commit... And it is free for open-source projects.

@kostyfisik
Contributor

BTW, just a bit of statistics, Initial pvs.txt has 173 messages, related to dealii, with 1.55 mln LoC which gives us estimated bug density of 0.11 per 1000 LoC - this is far less than 0.75 average (for open source with more than 1 mln LoC) and beats Linux kernels 0.59! Deal.ii rocks! :)
https://www.helpnetsecurity.com/2013/05/07/analyzing-450-million-lines-of-software-code/

pvs3.txt has 49 messages, so now it formally even better now! (keep in mind, that most of these 49 seem to be false positive, this means outstanding quality!).

1728ec0 2016-11-07 | Merge pull request #3499 from kronbichler/remove_memory_consumption_test

@bangerth
Member
bangerth commented Nov 8, 2016

Ha, these are interesting statistics! I think I always knew that we had fairly good code quality, but it's nice to see some actual statistics about this!

@bangerth
Member
bangerth commented Nov 8, 2016

It looks like we're down to one issue, #3353. @Rombur -- what's your suggestion how we should proceed with that one?

@kostyfisik
Contributor

@bangerth I posted a short report to PVS and requested on the trial in winter. Feel free to contact support@viva64.com as soon as you are ready to put your hands on PVS testing, they will issue you a trial license in a short period of time.

@davydden
Member

I can confirm that it's indeed easy to get a week trial licence for PVS by writing an email. Playing around with it now...

@davydden davydden referenced this issue in trilinos/Trilinos Nov 11, 2016
Open

a few issues from static code analyzer PVS #818

@kostyfisik
Contributor

@davydden Probably you can do the test of ASPECT if you have a trilinos installed.

I`ve got (sorry, it happened after the trial was over) an idea that it can be also interesting to run tests of the deal.ii competitors - just to better understand if it is valid to claim that deal.ii has the best codebase?

Two fast tests:

  1. mfem - seems to be extremely lightened but powerful from LLNL http://mfem.org/
    and very easy to build and check with PVS (I tested first two commands) - due to the origin - zero messages?
make config
pvs-studio-analyzer trace -- make all -j12
pvs-studio-analyzer analyze -l PVS-Studio.lic -o pvs.log -j12
plog-converter -a GA:1,2 -t tasklist -o pvs.txt pvs.log
  1. http://www.freefem.org/ - I think that this FEM package has the most academic impact around open sourced FEM (as I was looking references to deal.ii, FreeFem++, and many others in scopus database). While it seems to be not so big project (about 130 KLoC) I would expect many errors (during compilation I so many warnings).
    to check I had to sudo apt-get install bison flex first, after that
./configure
pvs-studio-analyzer trace -- make -j6

Two long runs:
3) libMesh - the only one comparable in code base https://github.com/libMesh/libmesh
check should be easy (no dependences, so just configure and pvs trace make).
4) https://fenicsproject.org/ - like deal.ii it has a Wilkinson Prize (in 2015), Python interface and can scale up to 24k core (confirmed scaling). However, putting it via PVS can be hard (the custom build script is hard to understand).

@davydden
Member

I am trying to run analyzis on deal.II

 pvs-studio-analyzer trace -- make all -j12

with license file in the build folder, but have error

No compilation units found
Analysis finished in 0::0::00.00

@kostyfisik any ideas?

@davydden
Member

trying with JSON now...

@davydden
Member

@bangerth @drwells @Rombur btw, i think we need to run the whole test suite through PVS. Because (from http://www.viva64.com/en/m/0036/)

It is important to understand that all files to be analyzed should be compiled. If your project actively uses code generation, then this project should be built before analysis, otherwise there may be errors during preprocessing.

So anything that is header only, won't get checked as far as I understand.

@bangerth
Member

Valid point. Though I suspect that we instantiate most everything that's in one header or another through one of the .cc files we have. Maybe not for all possible template arguments, but for some at least, and that should uncover most errors I think.

@davydden
Member

Agreed, there are very few classes which are header-only, for example I picked up this warning for Parpack when compiling my library:

/home/davydden/spack/opt/spack/linux-ubuntu16-x86_64/gcc-5.4.0/dealii-develop-s3q44glws4wtxtqcyscogri3blotqm5j/include/deal.II/lac/parpack_solver.h 526 err V730 Not all members of a class are initialized inside the constructor. Consider inspecting: lworkl, nloc, ncv, ldv, ldz, lworkev.
@kostyfisik
Contributor

@davydden lic file does not matter for trace build... Are you doing the build from the empty directory? Another point is that there are no valid all target for deal.ii, the correct command was pvs-studio-analyzer trace -- make -j12 install

@davydden
Member
davydden commented Nov 11, 2016 edited

JSON worked. So here is my output for deal.II configured with gcc 5.4.0, 32bit integers and all optional packages (Filtered messages: 245, so it looks like it picked up more than what @kostyfisik has in his last analysis).

pvs_tasks.txt

To reproduce, the steps are

$ cmake -DCMAKE_EXPORT_COMPILE_COMMANDS=On <blah-blah-blah> ../
$ make all -j8
$ pvs-studio-analyzer analyze -l PVS-Studio.lic -o pvs.log -j8
$ plog-converter -a GA:1,2 -t tasklist -o pvs_tasks.txt pvs.log

p.s. i don't know why PVS did not do analysis from strace output for me. Maybe some size limitation, the file was around 0.5Gb.

@kostyfisik
Contributor

BTW, it looks like for static analysis you should compile deal.ii in debug mode only. At least for Clang it is explicitly recommended to do so, as soon as they use asserts to eliminate false positives, I think that PVS should be clever enough to do it too.

This was referenced Nov 11, 2016
@bangerth
Member

My apologies for the noise. I'm going to wait for a while till I can open issues again, then do so for each file. Some of them may be duplicates.

@bangerth
Member

OK, sanity restored. My apologies for the mess (and the many emails everyone must have gotten)!

@kostyfisik
Contributor

BTW, PVS-Studio is free now to use with open-source projects free enough to add specially formatted comments to the source. See the details http://www.viva64.com/en/b/0457/

@kostyfisik
Contributor

Putting it into Travis would be a next logical step if adding special comments can be accepted with deal.ii. However, this will obviously require masking of false positives...

@bangerth
Member
bangerth commented Dec 6, 2016

But doesn't it take a long time to run the entire step? We can't put it into Travis if it takes more than a few minutes or up to half an hour, as then we'd never get through our patch load on some days...

@kostyfisik
Contributor

PVS has an incremental mode http://www.viva64.com/en/m/0024/

@kostyfisik
Contributor

I am not sure it is available for linux now, but I bet it can be at least discussed with PVS devs.

@bangerth
Member
bangerth commented Jan 3, 2017

#3538 is the only open issue right now. I think that since we addressed all other ones, we can close this. We can always open a new issue (or reopen this one) if we have more to say here.

@bangerth bangerth closed this Jan 3, 2017
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment