kostyfisik commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

bangerth commented 8 years ago

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 commented 8 years ago

And the ghi script is from here:

curl -sL https://raw.githubusercontent.com/stephencelis/ghi/master/ghi > ghi

bangerth commented 8 years ago

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 commented 8 years ago

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

Rombur commented 8 years ago

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

kostyfisik commented 8 years ago

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 https://github.com/dealii/dealii/issues/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 commented 8 years ago

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

bangerth commented 8 years ago

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

kostyfisik commented 8 years ago

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 commented 8 years ago

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

kostyfisik commented 8 years ago

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 commented 8 years ago

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

Nice work everyone :tada:

bangerth commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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

kostyfisik commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

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

kostyfisik commented 8 years ago

@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 commented 8 years ago

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...

kostyfisik commented 8 years ago

@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

2) 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 commented 8 years ago

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 commented 8 years ago

trying with JSON now...

davydden commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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 commented 8 years ago

@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 commented 8 years ago

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 commented 8 years ago

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.

bangerth commented 8 years ago

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 commented 8 years ago

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

kostyfisik commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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 commented 7 years ago

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

kostyfisik commented 7 years ago

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

bangerth commented 7 years ago

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.

dealii / dealii

Check with static code analyser #3342

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

3360 #3363 #3367 #3370 #3378 #3402 #3408 #3411 #3423

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.