False Positive in Clang Analyzer Report for Path.cc

[Dr15Jones]

The clang analyzer states

  src/FWCore/Framework/src/Path.cc:246:7: warning: Value stored to 'shouldContinue' is never read [deadcode.DeadStores]
   246 |       shouldContinue = false;
      |       ^                ~~~~~
1 warning generated.

but the variable IS later read https://github.com/cms-sw/cmssw/blob/e54f434a789ce6ffaf824788a7b9bf79a50adf1a/FWCore/Framework/src/Path.cc#L276

I believe the static analyzer is confused by the try...catch statements in between the writing and reading of the variable.

[Dr15Jones]

assign core

[cmsbuild]

New categories assigned: core

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

[cmsbuild]

cms-bot internal usage

[cmsbuild]

A new Issue was created by @Dr15Jones.

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

cms-bot commands are listed here

[Dr15Jones]

it MIGHT be possible to silence the warning by moving shouldContinue = false; from line 246 to after the end of the last catch block (i.e. line 273).

[makortel]

FYI @gartung

[makortel]

Wait, is this "Clang analyzer" the same as our "Static analyzer" or something different? I see both in our IB dashboard. @smuzaffar

[Dr15Jones]

In fact, all the warnings reported in FWCore/Framework are false positives.

The memory leak in StreamSchedule is incorrect (the pointer is owned by a std::unique_ptr held elsewhere with a lifetime longer than the pointer) and the null value used in ComponentFactory is also incorrect (the value is either directly created or obtained from a std::map where the value must be non null to be added to the map)

[smuzaffar]

@makortel , Clang analyzer and Static analyzer are result of same job ib-static analyzer. Static analyzer is generated by running clang's scan-build and Clang analyzer is generated by parsing it's build logs using our normal bot log analyzer

[gartung]

This warning is from one of the built-in checkers.

[smuzaffar]

it MIGHT be possible to silence the warning by moving shouldContinue = false; from line 246 to after the end of the last catch block (i.e. line 273).

yes, I think analyzer thinks that std::rethrow_exception(*iException); call at https://github.com/cms-sw/cmssw/blob/master/FWCore/Framework/src/Path.cc#L249 will not return so setting shouldContinue = false; is deadcode. moving this assignment after the try/catch block should avoid the warning