[LLVM] Added EventSetupRecord::get checker

[smuzaffar]

See cms-externals/llvm-project#10

[smuzaffar]

please test

[cmsbuild]

A new Pull Request was created by @smuzaffar (Malik Shahzad Muzaffar) for branch IB/CMSSW_11_3_X/master.

@smuzaffar, @mrodozov can you please review it and eventually sign? Thanks.
cms-bot commands are listed here

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-58280d/13534/summary.html
COMMIT: 0c7bb7e
CMSSW: CMSSW_11_3_X_2021-03-15-2300/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/6734/13534/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• No significant changes to the logs found
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 37
• DQMHistoTests: Total histograms compared: 2635087
• DQMHistoTests: Total failures: 5
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2635060
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
• Checked 155 log files, 37 edm output root files, 37 DQM output files
• TriggerResults: no differences found

[smuzaffar]

+externals
FYI @gartung , this should include your changes on top of llvm 11.1

[cmsbuild]

This pull request is fully signed and it will be integrated in one of the next IB/CMSSW_11_3_X/master IBs (tests are also fine). This pull request will now be reviewed by the release team before it's merged. @silviodonato, @dpiparo, @qliphy (and backports should be raised in the release meeting by the corresponding L2)

[smuzaffar]

@gartung , the PR tests use -enable-checker threadsafety -enable-checker cms -disable-checker cms.FunctionDumper options ( https://github.com/cms-sw/cms-bot/blob/master/pr_testing/test_multiple_prs.sh#L687 ). Do they look good ?

[smuzaffar]

please test with cms-sw/cmssw#33189
lets test with one of cmssw PR to see what static checks report

[gartung]

For the static analyzer that is correct. Wherever you enable the clang-tidy checks, you need to change/add cms.*.

[gartung]

There does not appear to be any restrictions on clang-tidy checkers so the new one should just run.
https://github.com/cms-sw/cmssw-config/blob/ef9d787cad2b4af040256fb0e14bbe7540a55351/SCRAM/GMake/Makefile.coderules#L24

[smuzaffar]

for clang-tidy, checks are picked up from https://github.com/cms-sw/cmssw/blob/master/.clang-tidy where we have non of cms checks enabled.

[smuzaffar]

There does not appear to be any restrictions on clang-tidy checkers so the new one should just run.
https://github.com/cms-sw/cmssw-config/blob/ef9d787cad2b4af040256fb0e14bbe7540a55351/SCRAM/GMake/Makefile.coderules#L24

the USER_CODE_CHECKS is empty for code checks ( https://github.com/cms-sw/cmssw-config/blob/ef9d787cad2b4af040256fb0e14bbe7540a55351/SCRAM/GMake/Makefile.coderules#L15 ) , so it just runs default checks from the .clang-tidy file

[gartung]

OK. I will submit a pull request to add cms.* to .clang-tidy.

[makortel]

I'm now confused. Would changing .clang-tidy make those checks run at code-checks time? Without that change, will they get run at the "PR test" time? (for this check we want the latter, at least for now)

[smuzaffar]

Yes, now after .clang-tidy change it will run at code-checks time.
If I understand correctly, this clang-tidy check only prints warning without any code change , so I think this will not break any thing. But this also means that no one will look in to those warnings ( unless we start reporting warnings in the code-checks summary results).

[gartung]

If we want the CMS checks to run any time clang-tidy is run then this PR is needed cms-sw/cmssw#33197
If we want the CMS checks to only run for pull request checks then the environment variable USER_CODE_CHECKS needs to be set.

[gartung]

The Event::getByToken check will actually change the code because it has a fixit replacement. I test that all of the replacements produced code that could be compiled when I wrote that checker.

[gartung]

Correction. The environment variable USER_CODE_CHECKS_ARGS needs to be set for pull request testing and the warnings recorded.

[makortel]

is clang-tidy currently run at the PR test phase (in addition to code-checks)?

[smuzaffar]

No, .clang-tidy is only used at the time of code-checks

[gartung]

Only for code-checks as far as I can tell. Running the code-checks with the environment variable could be added as part of the static analyzer check here https://github.com/cms-sw/cms-bot/blob/master/pr_testing/test_multiple_prs.sh#L687 or added as its own check in the same script and the warnings recorded.

[makortel]

Let's backtrack a bit.

What w'd want is some way to flag deprecated functionality in PRs with different levels of enforcement. I can think of

1. flag code in IBs without impacting the dashboard
2. flag code in PRs in all files in packages checked out with git cms-checkdeps -a
3. flag code only in user local area (during scram b or a separate checker)
4. flag code in PRs (not in IBs), limit only to files modified in the PR
   a. list warnings in the PR test report (allowing L2s to ignore if necessary for good reason)
   b. hard failure of PR tests
5. warnings/failures in IBs, visible in the dashboard

Currently we do

• 1 with static analyzer and various clang reports
• 2 with static analyzer
• 4b with code checks (clang-format and clang-tidy) and compiler itself (warnings and errors must be fixed)
• 5 (and 3) with compiler warnings and errors ([[deprecated]] suggested in Use [[deprecated]] to mark deprecated classes and functions cmssw#31021 would also fall here)

I'm currently looking for 4a for the EventSetupRecord::get(), likely elevating to 4b at some point in the future (eventually to 5).

3 or 4a could be useful for legacy EDModules as well (as mentioned in #31021).

Does 4a make sense, or would it be better to wait until we can enforce with 4b (in which case the clang-tidy at the code-checks step would be fine)?

If the answer is "yes", what would be the best way to do that? Make use of static analyzer information, use clang-tidy, use C++ attributes, something else?

[gartung]

I think only the files changed in a pull request have clang-tidy code format checks run on them. The CMS clang-tidy test could be run only on the files changed as part of the pull request tests.

[cmsbuild]

+1

Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-58280d/13554/summary.html
COMMIT: 0c7bb7e
CMSSW: CMSSW_11_3_X_2021-03-16-1100/slc7_amd64_gcc900
User test area: For local testing, you can use /cvmfs/cms-ci.cern.ch/week0/cms-sw/cmsdist/6734/13554/install.sh to create a dev area with all the needed externals and cmssw changes.

Comparison Summary

Summary:

• You potentially added 3754 lines to the logs
• Reco comparison results: 0 differences found in the comparisons
• DQMHistoTests: Total files compared: 37
• DQMHistoTests: Total histograms compared: 2639881
• DQMHistoTests: Total failures: 1
• DQMHistoTests: Total nulls: 0
• DQMHistoTests: Total successes: 2639858
• DQMHistoTests: Total skipped: 22
• DQMHistoTests: Total Missing objects: 0
• DQMHistoSizes: Histogram memory added: 0.0 KiB( 36 files compared)
• Checked 155 log files, 37 edm output root files, 37 DQM output files
• TriggerResults: no differences found

[makortel]

Actually I think the Event::getByToken() check (cms-handle) could be enabled in code-checks phase because it always proposes a fix and therefore applying the fix is trivial for the developer (and I'd imagine performing that check later in the PR test phase would be more annoying for the developer).

I'm fine with running clang-tidy again in the PR test phase for reporting warnings of the EventSetupRecord::get() check. Would a custom clang-tidy check be then the way we want to deal with similar API deprecation in the future (like legacy EDModules)?

[smuzaffar]

I enabled cms-handle check for full cmssw and it suggested 6.5K changes in cmssw ( https://cmssdt.cern.ch/SDT/jenkins-artifacts/jenkins-test-code-format/CMSSW_11_3_X_2021-03-16-1100/64/ ) . Yes we can enable it for code-checks but better to first run a campaign to first get most of these fixed

[smuzaffar]

I agree that for warnings about EventSetupRecord::get() check , we can run clang-tidy and any other cms check during PR static analysis tests. @gartung , can you please open a cms-bot PR to do this ?

[makortel]

Yes we can enable it for code-checks but better to first run a campaign to first get most of these fixed

Good point. It could also probably be useful to submit PRs first one-to-few packages manually before launching bigger campaign (and I have one question to Patrick first).

[gartung]

cms-sw/cms-bot#1513

[gartung]

Alternative based on running clang-tidy with custom CMS checkers enabled.
cms-sw/cms-bot#1514