Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Static analyzer report shrunk with LLVM update #41828

Closed
makortel opened this issue May 31, 2023 · 26 comments
Closed

Static analyzer report shrunk with LLVM update #41828

makortel opened this issue May 31, 2023 · 26 comments

Comments

@makortel
Copy link
Contributor

The static analyzer reports in 13_0_X about 39k issues (https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_13_0_X_2023-05-30-2300/el8_amd64_gcc11/llvm-analysis/index.html), whereas in 13_2_X it reports only about 4k issues (https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_13_2_X_2023-05-30-2300/el8_amd64_gcc11/llvm-analysis/index.html). In 13_0_X we had LLVM 12, and in 13_2_X LLVM 16. Likely something in the static analyzer got borken with the LLVM update.

@makortel
Copy link
Contributor Author

assign core

@cmsbuild
Copy link
Contributor

New categories assigned: core

@Dr15Jones,@smuzaffar,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

@cmsbuild
Copy link
Contributor

A new Issue was created by @makortel Matti Kortelainen.

@Dr15Jones, @perrotta, @dpiparo, @rappoccio, @makortel, @smuzaffar can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

@makortel
Copy link
Contributor Author

@gartung This is what we briefly talked about.

@gartung
Copy link
Member

gartung commented May 31, 2023

The biggest decrease is for the mutable member checker. I will start there.

@gartung
Copy link
Member

gartung commented May 31, 2023

It looks like LLVM is removing duplicate reports. The post processing script does this for older versions of llvm/clang. Comparing the filtered pages the number of mutable member entries is 450 for CMSSW_13_0_X and 355 for CMSSW_13_2_X. So the difference is orders of magnitude smaller compared to the unfiltered count of 23534 for CMSSW_13_0_X and 368 for CMSSW_13_2_X. The filtered vs unfiltered for CMSSW_13_2_X is only 368 vs 355.

@gartung
Copy link
Member

gartung commented May 31, 2023

@makortel I can dig deeper into the 450 vs 355 difference if you think it is worth the effort.

@gartung
Copy link
Member

gartung commented May 31, 2023

The unfiltered scan-build report has multiple entries per file which account for some difference with the filtered scan-build report. The unfiltered report is again sortable. The filtered report being unresponsive to sorting was reported previously in #37982

@makortel
Copy link
Contributor Author

It looks like LLVM is removing duplicate reports.

Could you check some other bug category as well if the same happens there? (probably does)

I can dig deeper into the 450 vs 355 difference if you think it is worth the effort.

I think one step deeper inspection could be useful. A difference of hundred feels large (although if genuine, very welcome!)

@gartung
Copy link
Member

gartung commented Jun 1, 2023

These two are no longer tagged

mutable std::condition_variable doit;

@makortel
Copy link
Contributor Author

makortel commented Jun 1, 2023

These two are no longer tagged

mutable std::condition_variable doit;

Ok, that makes sense.

@makortel
Copy link
Contributor Author

makortel commented Jun 1, 2023

Could you check the cause of the differences of

  • "const_cast used on pointer to class", decreases from 1461 to 205
  • "non-const static variable", decreases from 7571 to 891
  • "Dead nested assignment", decreases from 295 to 27

as an additional examples? Are they all caused by the reduction of duplicates, or is there some example case that doesn't get reported anymore?

I think (well, hope) if these would be a sufficient set of checks such that if nothing suspicious comes up, we can continue with the new report and close the issue.

@gartung
Copy link
Member

gartung commented Jun 1, 2023

@gartung
Copy link
Member

gartung commented Jun 1, 2023

These two variables of the same name but different class are tagged.

mutable std::condition_variable doit;

@makortel
Copy link
Contributor Author

makortel commented Jun 1, 2023

These two variables of the same name but different class are tagged.

Was the link correct?

@gartung
Copy link
Member

gartung commented Jun 1, 2023

Yes

@gartung
Copy link
Member

gartung commented Jun 1, 2023

I looked at the output of the FunctionDumper for the three versions of clang, 12, 14, and 16. The correct number of mutables are reported for anycp.cpp in all three cases. I will have to check what I did differently for mutables in FunctionDumper

@gartung
Copy link
Member

gartung commented Jun 1, 2023

Correction, the output files for ConstCastChecker, ConstCastAwayChecker and MutableMemberChecker are all sorted unique to produce const-checker.txt. The difference between clang 12 and clang 16 is

 diff llvm12/const-checker.txt llvm16/const-checker.txt 
5a6
> flagged class 'AlignableStackDet' const_cast used 
455a457,459
> flagged class 'hgcal::HGCalFrameGenerator' mutable member 'hgcal::HGCalFrameGenerator::emul_
> flagged class 'hgcal::HGCalFrameGenerator' mutable member 'hgcal::HGCalFrameGenerator::last_emul_event_
> flagged class 'hgcal::HGCalFrameGenerator' mutable member 'hgcal::HGCalFrameGenerator::last_slink_emul_info_

This is easily explained by changes in 132x relative to 130x.

@gartung
Copy link
Member

gartung commented Jun 1, 2023

The difference must be in how BugReporter is handling multiple reports of the same variable name from different classes in the same compilation unit.

@gartung
Copy link
Member

gartung commented Jun 1, 2023

For "Dead nested assignment" the difference in unique reports between clang 12 - 130X and clang 16 - 132x is attributable to changes in the code

diff nested_keys_130x.txt nested_keys_132x.txt 
10c10
< <td class="DESC">Dead nested assignment</td><td>/IOPool/TFileAdaptor/src/TStorageFactoryFile.cc</td><td class="DESC">WriteBuffer</td><td class="Q">491</td>
---
> <td class="DESC">Dead nested assignment</td><td>/IOPool/TFileAdaptor/src/TStorageFactoryFile.cc</td><td class="DESC">WriteBuffer</td><td class="Q">495</td>
21d20
< <td class="DESC">Dead nested assignment</td><td>/cvmfs/cms-ib.cern.ch/sw/x86_64/nweek-02787/el8_amd64_gcc11/external/geant4/10.7.2-0a5e38613906bb5debc9753e0e1d40b3/include/Geant4/G4DataVector.icc</td><td class="DESC">index</td><td class="Q">59</td>

@makortel
Copy link
Contributor Author

makortel commented Jun 1, 2023

Correction, the output files for ConstCastChecker, ConstCastAwayChecker and MutableMemberChecker are all sorted unique to produce const-checker.txt. The difference between clang 12 and clang 16 is

 diff llvm12/const-checker.txt llvm16/const-checker.txt 
5a6
> flagged class 'AlignableStackDet' const_cast used 
455a457,459
> flagged class 'hgcal::HGCalFrameGenerator' mutable member 'hgcal::HGCalFrameGenerator::emul_
> flagged class 'hgcal::HGCalFrameGenerator' mutable member 'hgcal::HGCalFrameGenerator::last_emul_event_
> flagged class 'hgcal::HGCalFrameGenerator' mutable member 'hgcal::HGCalFrameGenerator::last_slink_emul_info_

This is easily explained by changes in 132x relative to 130x.

These flagged cases all look suspicious (i.e. good to be flagged). But do I understand correctly that they are flagged in llvm16 but not in llvm12? (i.e. would not explain the cases reported in 12 but not in 16)

@gartung
Copy link
Member

gartung commented Jun 1, 2023

This is easily explained by changes in 132x relative to 130x.

These flagged cases all look suspicious (i.e. good to be flagged). But do I understand correctly that they are flagged in llvm16 but not in llvm12? (i.e. would not explain the cases reported in 12 but not in 16)

Yes. I am still trying to determine why some bug reports are missing. The text output is written to file bypassing the BugReporter.

@gartung
Copy link
Member

gartung commented Jun 3, 2023

I have a possible solution, art least for the missing reports for mutable members with the same name in different classes. There may be other cases.
cms-externals/llvm-project#12

@makortel
Copy link
Contributor Author

+core

I think this has been addressed

@cmsbuild
Copy link
Contributor

cms-bot internal usage

@cmsbuild
Copy link
Contributor

This issue is fully signed and ready to be closed.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

No branches or pull requests

3 participants