avoid overflowing firstStrip_ in case of approximated clusters
#38735
Conversation
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38735/31048
|
|
A new Pull Request was created by @mmusich (Marco Musich) for master. It involves the following packages:
@jpata, @cmsbuild, @clacaputo can you please review it and eventually sign? Thanks. cms-bot commands are listed here |
|
test parameters:
|
|
@cmsbuild, please test for CMSSW_12_5_ASAN_X |
mmusich
changed the title
firstStrip_ in case of approximated clustersfirstStrip_ in case of approximated clusters
| @@ -28,7 +28,7 @@ SiStripCluster::SiStripCluster(const SiStripApproximateCluster cluster) : error_ | |||
| amplitudes_.resize(cluster.width(), cluster.avgCharge()); | |||
|
|
|||
| //initialize firstStrip_ | |||
| firstStrip_ = cluster.barycenter() - cluster.width() / 2; | |||
| firstStrip_ = std::max(0.f, cluster.barycenter() - cluster.width() / 2); | |||
There was a problem hiding this comment.
Maybe you can recycle here the same barycenter_ already computed a few lines above...
There was a problem hiding this comment.
sure. I have more doubts though:
-
given
should it be divided bycluster.width()here is auint8_t2.fto avoid truncating the half-width of one half-strip? -
I am not sure if there's a better (more efficient) way of computing the max since this should be a pretty intensive call in production.
There was a problem hiding this comment.
Hi,
I think Marco is correct on item 1, we should be dividing by a float here. I am not familiar with the most efficient way of implementing item 2, although we could check the timing if it is a strong concern.
There was a problem hiding this comment.
plase 0.5f*float(cluster.width())
max is not slower than the above (and much faster than cluster.width() / 2.)
|
-1 Failed Tests: RelVals The following merge commits were also included on top of IB + this PR after doing git cms-merge-topic:
You can see more details here: RelVals
|
|
the other problem we have uncovered now is coming from the other "side" of the module, see log. By adding: diff --git a/RecoLocalTracker/SiStripClusterizer/src/SiStripClusterInfo.cc b/RecoLocalTracker/SiStripClusterizer/src/SiStripClusterInfo.cc
index dcdf774f0ef..58d1cc53b97 100644
--- a/RecoLocalTracker/SiStripClusterizer/src/SiStripClusterInfo.cc
+++ b/RecoLocalTracker/SiStripClusterizer/src/SiStripClusterInfo.cc
@@ -42,6 +42,8 @@ std::vector<float> SiStripClusterInfo::stripNoisesRescaledByGain() const {
std::vector<float> results;
results.reserve(width());
for (size_t i = 0, e = width(); i < e; i++) {
+ std::cout << "i: " << i << " firstStrip: " << firstStrip() << " getting strip:" << firstStrip() + i << std::endl;
+
results.push_back(siStripNoises_->getNoise(firstStrip() + i, detNoiseRange) /
siStripGain_->getStripGain(firstStrip() + i, detGainRange));
}I get: i: 0 firstStrip: 757 getting strip:757
i: 1 firstStrip: 757 getting strip:758
i: 2 firstStrip: 757 getting strip:759
i: 3 firstStrip: 757 getting strip:760
i: 4 firstStrip: 757 getting strip:761
i: 5 firstStrip: 757 getting strip:762
i: 6 firstStrip: 757 getting strip:763
i: 7 firstStrip: 757 getting strip:764
i: 8 firstStrip: 757 getting strip:765
i: 9 firstStrip: 757 getting strip:766
i: 10 firstStrip: 757 getting strip:767
i: 11 firstStrip: 757 getting strip:768
=================================================================
==3626964==ERROR: AddressSanitizer: heap-buffer-overflow on address 0x7f8e8c3717ff at pc 0x7f8f1bb0beba bp 0x7ffe3fda2080 sp 0x7ffe3fda2078
READ of size 1 at 0x7f8e8c3717ff thread T0so it can happen that |
mmusich
force-pushed
the
fixRawPrimeWorkflow
branch
from
9330786
to
bd09b48
Compare
cmsbuild
added
code-checks-pending
tests-pending
and removed
tests-rejected
code-checks-approved
labels
|
with bd09b48 the failing workflow runs, but it doesn't address the case of the 4 APV modules:
|
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38735/31068
|
|
Pull request #38735 was updated. @jpata, @cmsbuild, @clacaputo can you please check and sign again. |
cmsbuild
added
code-checks-pending
tests-pending
and removed
code-checks-approved
labels
|
+code-checks Logs: https://cmssdt.cern.ch/SDT/code-checks/cms-sw-PR-38735/31128
|
|
Pull request #38735 was updated. @jpata, @cmsbuild, @clacaputo can you please check and sign again. |
|
@cmsbuild, please test for CMSSW_12_5_ASAN_X |
|
-1 Failed Tests: UnitTests Unit TestsI found errors in the following unit tests: ---> test test_ProduceSystematicMisalignment had ERRORS ---> test testAlignmentOfflineValidation had ERRORS ---> test test_PixelBaryCentreTool had ERRORS ---> test testPedeCampaign had ERRORS and more ... |
Failures are unrelated, already seen in the IB: https://cmssdt.cern.ch/SDT/cgi-bin/showBuildLogs.py/el8_amd64_gcc10/www/mon/12.5.ASAN-mon-11/CMSSW_12_5_ASAN_X_2022-07-18-1100?utests |
|
+reconstruction
|
cmsbuild
added
fully-signed
reconstruction-approved
and removed
reconstruction-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. @perrotta, @dpiparo, @qliphy, @rappoccio (and backports should be raised in the release meeting by the corresponding L2) |
|
please test |
|
+1 Summary: https://cmssdt.cern.ch/SDT/jenkins-artifacts/pull-request-integration/PR-7fab06/26349/summary.html Comparison Summary@slava77 comparisons for the following workflows were not done due to missing matrix map:
Summary:
|
|
+1 |
resolves #38729
PR description:
See #38729 (comment)
PR validation:
Tested running 140.58 successfully under
CMSSW_12_5_ASAN_X_2022-07-13-1100If this PR is a backport please specify the original PR and why you need to backport that PR. If this PR will be backported please specify to which release cycle the backport is meant for:
N/A