Skip to content

Commit

Permalink
fix: avoid integer wraparound in CalorimeterHitDigi (#1557)
Browse files Browse the repository at this point in the history
### Briefly, what does this PR introduce?
In some configurations (EcalBarrelImagingRawHits), the pedestal mean (14
a.u.) and sigma (5 a.u.) are such that negative hit pedestal channel
numbers are not uncommon in a simple Gaussian model. For low energy
hits, this pushes the ADC value into negative double range, and
conversion to unsigned long long results in integer wraparound. E.g.
https://github.com/eic/EICrecon/actions/runs/10239218932/job/28324710737#step:9:6119.
```
Error: /home/runner/work/EICrecon/EICrecon/src/algorithms/calorimetry/CalorimeterHitDigi.cc:186:38: runtime error: implicit conversion from type 'long long' of value -2 (64-bit, signed) to type 'unsigned long long' changed the value to 18446744073709551614 (64-bit, unsigned)
```

This PR prevents the hit pedestal value from being negative. This PR
does not address other reasons why we can get negative values, through
intentionally incorrect user input (negative dynamic range).

### What kind of change does this PR introduce?
- [x] Bug fix (issue: UBSan implicit-integer-sign-change in
CalorimeterHitDigi)
- [ ] New feature (issue #__)
- [ ] Documentation update
- [ ] Other: __

### Please check if this PR fulfills the following:
- [ ] Tests for the changes have been added
- [ ] Documentation has been added / updated
- [ ] Changes have been communicated to collaborators

### Does this PR introduce breaking changes? What changes might users
need to make to their code?
No.

### Does this PR change default behavior?
No.

---------

Co-authored-by: Dmitry Kalinkin <[email protected]>
  • Loading branch information
wdconinc and veprbl authored Aug 7, 2024
1 parent f3b5505 commit a8abfdb
Showing 1 changed file with 8 additions and 4 deletions.
12 changes: 8 additions & 4 deletions src/algorithms/calorimetry/CalorimeterHitDigi.cc
Original file line number Diff line number Diff line change
Expand Up @@ -23,6 +23,7 @@
#include <edm4hep/CaloHitContributionCollection.h>
#include <fmt/core.h>
#include <podio/RelationRange.h>
#include <algorithm>
#include <cmath>
#include <cstddef>
#include <gsl/pointers>
Expand Down Expand Up @@ -181,10 +182,13 @@ void CalorimeterHitDigi::process(
std::pow(m_cfg.eRes[2] / (edep), 2)
)
: 0;
double corrMeanScale_value = corrMeanScale(leading_hit);
double ped = m_cfg.pedMeanADC + m_gaussian(m_generator) * m_cfg.pedSigmaADC;
unsigned long long adc = std::llround(ped + edep * corrMeanScale_value * ( 1.0 + eResRel) / m_cfg.dyRangeADC * m_cfg.capADC);
unsigned long long tdc = std::llround((time + m_gaussian(m_generator) * tRes) * stepTDC);
double corrMeanScale_value = corrMeanScale(leading_hit);

double ped = m_cfg.pedMeanADC + m_gaussian(m_generator) * m_cfg.pedSigmaADC;

// Note: both adc and tdc values must be positive numbers to avoid integer wraparound
unsigned long long adc = std::max(std::llround(ped + edep * corrMeanScale_value * (1.0 + eResRel) / m_cfg.dyRangeADC * m_cfg.capADC), 0LL);
unsigned long long tdc = std::llround((time + m_gaussian(m_generator) * tRes) * stepTDC);

if (edep> 1.e-3) trace("E sim {} \t adc: {} \t time: {}\t maxtime: {} \t tdc: {} \t corrMeanScale: {}", edep, adc, time, m_cfg.capTime, tdc, corrMeanScale_value);
rawhits->create(
Expand Down

0 comments on commit a8abfdb

Please sign in to comment.