edmPluginHelp bug fix, crash would occur if helper plugin contains itself recursively #43126
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43126/37390
|
|
A new Pull Request was created by @wddgit (W. David Dagenhart) for master. It involves the following packages:
@cmsbuild, @makortel, @Dr15Jones, @smuzaffar can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
please test |
|
abort test |
Also improves and fixes the printout of edmPluginHelp in the sections related PluginDescriptions. Add to existing unit test to cover these modifications.
wddgit
force-pushed
the
bugFixEdmPluginHelp
branch
from
8d922b4 to
f6200bd
Compare
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-43126/37396
|
|
Pull request #43126 was updated. @smuzaffar, @makortel, @cmsbuild, @Dr15Jones can you please check and sign again. |
|
please test The changes since the previous test run only implement Chris's suggestion. The significant changes are in the 3 FWCore/ParameterSet files, plus the extra text shows up in the reference files for the edmPluginHelp output. |
|
-1 Failed Tests: UnitTests RelVals-INPUT Unit TestsI found 1 errors in the following unit tests: ---> test testTauEmbeddingProducers had ERRORS RelVals-INPUTThe relvals timed out after 4 hours. Comparison SummarySummary:
|
|
The unit test that failed is also failing in the IBs already so that is not related to this PR. I can see the fix to the unit test is merged but not in any IB release yet. I could try again in a day or two if anyone thinks it is worth getting the test status green. Is it normal for RelVals to timeout these days? I cannot see how this PR could cause that. |
not really, let me re-start the input relvals test |
|
-1 Failed Tests: UnitTests RelVals-INPUT Unit TestsI found 1 errors in the following unit tests: ---> test testTauEmbeddingProducers had ERRORS RelVals-INPUT
Comparison SummarySummary:
|
|
Both the unit test and RelVal test that failed are also failing in the IB. |
|
+core |
cmsbuild
added
core-approved
fully-signed
and removed
core-pending
pending-signatures
labels
|
This pull request is fully signed and it will be integrated in one of the next master IBs (but tests are reportedly failing). This pull request will now be reviewed by the release team before it's merged. @antoniovilela, @sextonkennedy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
please test Try again and see if the tests pass now. At least one of the bug fixes is in (bugs were unrelated to this PR). |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-db51db/35507/summary.html Comparison SummarySummary:
|
|
Comparison differences are related to #39803 |
|
+1 |
PR description:
Bug fix in edmPluginHelp. A crash would occur if a module plugin contained a helper plugin that contained a helper plugin of the same category recursively. At a point in the hierarchy where a helper plugin is contained, edmPluginHelp prints the descriptions of all possible plugins in the category. If one of the possibilities is a plugin that recursively contains a helper plugin of the same category the infinite recursion would occur. Containment could be direct or indirect. Then either memory or the stack limit would be exceeded eventually.
This fixes the crash reported in Issue #42988. It also fixes the format issues with the printout that Chris reported in the same issue.
This situation is somewhat rare. It was noticed when printing for this module
edmPluginHelp -p CkfTrackCandidateMaker. That was the only one exhibiting the problem that I am aware of.I added some additional modifications to the printout related PluginDescription to make it easier to understand. There is some extra content to explain things. Also one level of numbering of sections is deleted. The section number hierarchy will now match the hierarchy of the ParameterSet, which is what we are trying to document, instead of the hierarchy of the description which has an extra level. It also makes this complex printout slightly simpler. The PluginDescription printout is still complex, but I don't see how to simplify it further. It is describing something complex.
Also adds to existing unit tests to cover these modifications. The extended unit tests would have detected the reported bug.
PR validation:
This should only affect the output of edmPluginHelp for modules with helper plugins and ParameterSet unit tests in FWCore/Integration. It shouldn't affect anything else.