Fix assert condition in CSCDMBHeader::cfebAvailable

[davidlt]

This should resolve 30+ warnings from GCC 7.1.1.

Signed-off-by: David Abdurachmanov David.Abdurachmanov@cern.ch

[cmsbuild]

A new Pull Request was created by @davidlt for master.

It involves the following packages:

EventFilter/CSCRawToDigi

@perrotta, @cmsbuild, @slava77, @davidlange6 can you please review it and eventually sign? Thanks.
@Martin-Grunewald, @ptcox, @barvic this is something you requested to watch as well.
@davidlange6 you are the release manager for this.

cms-bot commands are listed here

[slava77]

@cmsbuild please test

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20584/console Started: 2017/06/15 08:58

[cmsbuild]

-1

Tested at: 328b8a1

You can see the results of the tests here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19236/20584/summary.html

I found follow errors while testing this PR

Failed tests: RelVals AddOn

• RelVals:

When I ran the RelVals I found an error in the following worklfows:
4.22 step2

runTheMatrix-results/4.22_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC/step2_RunCosmics2011A+RunCosmics2011A+RECOCOSD+ALCACOSD+SKIMCOSD+HARVESTDC.log

8.0 step3

runTheMatrix-results/8.0_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS/step3_BeamHalo+BeamHalo+DIGICOS+RECOCOS+ALCABH+HARVESTCOS.log

140.53 step2

runTheMatrix-results/140.53_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI/step2_RunHI2011+RunHI2011+RECOHID11+HARVESTDHI.log

4.53 step3

runTheMatrix-results/4.53_RunPhoton2012B+RunPhoton2012B+HLTD+RECODR1reHLT+HARVESTDR1reHLT/step3_RunPhoton2012B+RunPhoton2012B+HLTD+RECODR1reHLT+HARVESTDR1reHLT.log

1000.0 step2

runTheMatrix-results/1000.0_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT/step2_RunMinBias2011A+RunMinBias2011A+TIER0+SKIMD+HARVESTDfst2+ALCASPLIT.log

1001.0 step2

runTheMatrix-results/1001.0_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5/step2_RunMinBias2011A+RunMinBias2011A+TIER0EXP+ALCAEXP+ALCAHARVD1+ALCAHARVD2+ALCAHARVD3+ALCAHARVD4+ALCAHARVD5.log

9.0 step3

runTheMatrix-results/9.0_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST/step3_Higgs200ChargedTaus+Higgs200ChargedTaus+DIGI+RECO+HARVEST.log

25.0 step3

runTheMatrix-results/25.0_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT/step3_TTbar+TTbar+DIGI+RECOAlCaCalo+HARVEST+ALCATT.log

1003.0 step2

runTheMatrix-results/1003.0_RunMinBias2012A+RunMinBias2012A+RECODDQM+HARVESTDDQM/step2_RunMinBias2012A+RunMinBias2012A+RECODDQM+HARVESTDDQM.log

• AddOn:

I found errors in the following addon tests:

cmsDriver.py RelVal -s HLT:Fake,RAW2DIGI,L1Reco,RECO --mc --scenario=pp -n 10 --conditions auto:run1_mc_Fake --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --processName=HLTRECO --filein file:RelVal_Raw_Fake_MC.root --fileout file:RelVal_Raw_Fake_MC_HLT_RECO.root : FAILED - time: date Thu Jun 15 10:08:12 2017-date Thu Jun 15 10:00:27 2017 s - exit: 34304
cmsDriver.py RelVal -s HLT:Fake,RAW2DIGI,L1Reco,RECO --data --scenario=pp -n 10 --conditions auto:run1_data_Fake --relval 9000,50 --datatier "RAW-HLT-RECO" --eventcontent FEVTDEBUGHLT --customise=HLTrigger/Configuration/CustomConfigs.L1THLT --processName=HLTRECO --filein file:RelVal_Raw_Fake_DATA.root --fileout file:RelVal_Raw_Fake_DATA_HLT_RECO.root : FAILED - time: date Thu Jun 15 10:09:06 2017-date Thu Jun 15 10:06:43 2017 s - exit: 34304

[cmsbuild]

Comparison not run due to runTheMatrix errors (RelVals and Igprof tests were also skipped)

[davidlange6]

interesting...

[davidlt]

I will leave experts to figure out what is right and wrong here.

[ptcox]

What does this say about how the original
icfeb < (theFirmwareVersion==2013)?7:5;
was parsed? Was it parsed as
( icfeb < (theFirmwareVersion==2013) ) ?7:5;
so that the assert could never fail?
It's pretty clear the new version is the intended meaning. We changed from a max of 5 CFEBs to 7 after LS1. It seems like the function must be getting called with icfeb values larger than the max if failures are now occurring. Victor, Help!

[davidlt]

From compiler point of view: EventFilter/CSCRawToDigi/interface/CSCDMBHeader.h:30:44: warning: ?: using integer constants in boolean context, the expression will always evaluate to 'true' [-Wint-in-bool-context]

Then it probably always assume 7.

[ptcox]

David, I guess I still don't understand what the compiler warning means. Is the boolean context either 'assert 7' or 'assert 5', following my hypothesized parse above?

[davidlt]

Here is my interpretation:
The original expression: icfeb < (theFirmwareVersion==2013)?7:5.
(theFirmwareVersion==2013) results in a bool value (true, false).
Then icfeb is unsigned char. So, it assumes you are comparing boolean with integer.

[ptcox]

Unsigned int not char, I guess, but I get the point. Thanks.

[davidlt]

Yeah, correct. I guess, my hands are too much used to writing unsigned char :)

[barvic]

I think this is a bug.
That buggy assert statement is never triggered in the original version.
But it will be triggered constantly in changed version with older DMB pre-LS1 format data (theFirmwareVersion != 2013), because upstream calling function
https://github.com/cms-sw/cmssw/blob/master/EventFilter/CSCRawToDigi/src/CSCEventData.cc#L259 will always pass icfeb values >= 5 as an argument to it.
...
for (int icfeb = 0; icfeb < MAX_CFEB; ++icfeb)
{
theCFEBData[icfeb] = 0;
int cfeb_available = theDMBHeader.cfebAvailable(icfeb);
...
MAX_CFEB is a constant defined as 7

That assert statement is not really needed there in this form.
It will work just fine without it.
Could you please just comment it out for now?

[davidlt]

Updated to remove the assert statement.

[davidlt]

please test

[cmsbuild]

The tests are being triggered in jenkins.

[cmsbuild]

Pull request #19236 was updated. @perrotta, @cmsbuild, @slava77, @davidlange6 can you please check and sign again.

[slava77]

@cmsbuild please test
it looks like the request for tests has to be made after the bot recognizes that there was a commit

[cmsbuild]

The tests are being triggered in jenkins.
https://cmssdt.cern.ch/jenkins/job/ib-any-integration/20659/console Started: 2017/06/17 00:23

[cmsbuild]

+1
Tested at: 90def8d
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19236/20659/summary.html

The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
ede1923
You can see more details here:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19236/20659/git-log-recent-commits
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19236/20659/git-merge-result

[cmsbuild]

Comparison job queued.

[cmsbuild]

Comparison is ready
https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-19236/20659/summary.html

Comparison Summary:

• You potentially added 5 lines to the logs
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 22
• DQMHistoTests: Total histograms compared: 1803136
• DQMHistoTests: Total failures: 15026
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 1787944
• DQMHistoTests: Total skipped: 166
• DQMHistoTests: Total Missing objects: 0
• Checked 90 log files, 14 edm output root files, 22 DQM output files

[slava77]

+1

for #19236 90def8d

• removed an assert (which was never functional)
• jenkins tests pass

The DQM plots show some small changes in L1TEMU uGMT plots. These are apparently random (perhaps some uninitialized variables), because logically this PR didn't change anything for running workflows. The tests included recompilation of related code, which could trigger differences in random memory locations.

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next master IBs (tests are also fine). This pull request requires discussion in the ORP meeting before it's merged. @davidlange6, @smuzaffar

[slava77]

@dmitrijus @ptcox
please alert L1TEMU DQM developers to take a look (maybe run valgrind)
the evidence for random differences is e.g. here in wf 136.761 relmon:
https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_9_2_X_2017-06-16-1100+19236/20683/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2/L1TEMU_L1TdeStage2uGMT_data_vs_emulator_comparison.html

[ptcox]

Not sure why I'm being asked this - I have no idea about L1TEMU DQM. I'm sure you know better than me. On June 17, 2017 at 14:42:26, Slava Krutelyov (notifications@github.com) wrote: @dmitrijus @ptcox please alert L1TEMU DQM developers to take a look (maybe run valgrind) the evidence for random differences is e.g. here in wf 136.761 relmon: https://cmssdt.cern.ch/SDT/jenkins-artifacts/baseLineComparisons/CMSSW_9_2_X_2017-06-16-1100+19236/20683/136.731_RunSinglePh2016B+RunSinglePh2016B+HLTDR2_2016+RECODR2_2016reHLT_skimSinglePh_HIPM+HARVESTDR2/L1TEMU_L1TdeStage2uGMT_data_vs_emulator_comparison.html — You are receiving this because you were mentioned. Reply to this email directly, view it on GitHub, or mute the thread.

[davidlange6]

+1