understanding how to use (and test) `CMS_SA_ALLOW`

[missirol]

(I might have to improve the title of this issue later on.)

I was trying to check the report of the static analyzer for a recent IB. For what I understand, one such report is the following.
https://cmssdt.cern.ch/SDT/jenkins-artifacts/ib-static-analysis/CMSSW_14_0_X_2024-01-01-2300/el8_amd64_gcc12/llvm-analysis/

There, I see that many warnings related to HLT packages come from calls to edm::ParameterSet::exists and edm::ParameterSet::existsAs in HLTConfig{Data,Provider}.cc. Since those calls seem justified [*], I wanted to silence the corresponding SA warnings. I understood this could be done using the macro CMS_SA_ALLOW.

First, I think I reproduced the SA warnings with the following 'pseudo recipe' (guessed based on run_class_checker.sh).

[..]
git cms-addpkg HLTrigger/HLTcore
git cms-addpkg Utilities/StaticAnalyzers
scram b
USER_LLVM_CHECKERS="-enable-checker threadsafety -enable-checker optional.ClassChecker -enable-checker cms -enable-checker deprecated -disable-checker cms.FunctionDumper" scram b -k checker  SCRAM_IGNORE_PACKAGES=Fireworks/% SCRAM_IGNORE_SUBDIRS=test > class+function-checker.log 2>&1

Then, I added CMS_SA_ALLOW calls to the HLT classes in question: missirol@7ee17d0. The issue is that, after re-running the recipe above (scram b checker [..]), I still see the original SA warnings, so I don't understand if I'm using (and testing) CMS_SA_ALLOW correctly.

In my attempt, I followed what was done in #42889. If I check a recent SA report (e.g. here), I see that the two calls modified by #42889 are still reported by the SA (as if that PR did not silence those warnings ?), so I'm even more confused.

Questions.

• In general, what's the proper way to silence SA warnings, and to test such updates ? (e.g. reproducing SA reports before and after a given change)
• For this specific use case, how could one silence the SA warnings coming from calls to edm::ParameterSet::exists{,As} in HLTConfig{Data,Provider}.cc ?

[*] HLTConfigData and HLTConfigProvider need to inspect the global "process PSet" to work as intended. In this case, the use of exists and existsAs calls seems justified and unavoidable (and is unrelated to the lack of fillDescriptions methods).

[cmsbuild]

cms-bot internal usage

[cmsbuild]

A new Issue was created by @missirol Marino Missiroli.

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

cms-bot commands are listed here

[Dr15Jones]

@gartung FYI

[makortel]

assign core

[cmsbuild]

New categories assigned: core

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

[gartung]

As far as I can recall this check was just meant for information. It does not included the attribute check for skipping the check with CMS_SA_ALLOW

cmssw/Utilities/StaticAnalyzers/src/PsetExistsFCallChecker.cpp

Line 91 in 9fca027

void PEFWalker::Report(const std::string &mname, const std::string &pname, const Expr *CE) const {

[gartung]

I can add an attribute check but it can only be applied to a declaration. I would have to look at the AST clang generates for that statement to see where to locate the CMS_SA_ALLOW statement.

[gartung]

@makortel @Dr15Jones did the canonical name of existsAs change? There is a veto on the canonical function name for
"edm::ParameterSet::exists"
"edm::ParameterSet::existsAs"
"edm::ParameterDescription"

[gartung]

Correction: it is not a veto. I checks the function name against the first two but vetos if the the class(?) is the third.

[gartung]

Correction it checks if the function is called within a class with the third string.

[makortel]

Is the if block very different from the try block (where the CMS_SA_ALLOW works) in the clang AST?