[GCC13] Possible dangling reference in PixelBaryCentreAnalyzer.cc

iarspider commented 6 months ago

GCC13 warns about possible dangling reference in PixelBaryCentreAnalyzer.cc:

src/Alignment/OfflineValidation/plugins/PixelBaryCentreAnalyzer.cc: In member function 'virtual void PixelBaryCentreAnalyzer::analyze(const edm::Event&, const edm::EventSetup&)':
src/Alignment/OfflineValidation/plugins/PixelBaryCentreAnalyzer.cc:263:27: warning: possibly dangling reference to a temporary [-Wdangling-reference]
  263 |     const AlignTransform& globalCoordinates = align::DetectorGlobalPosition(*globalPositions, DetId(DetId::Tracker));
      |                           ^~~~~~~~~~~~~~~~~
src/Alignment/OfflineValidation/plugins/PixelBaryCentreAnalyzer.cc:263:76: note: the temporary was destroyed at the end of the full expression 'align::DetectorGlobalPosition((* & globalPositions.std::unique_ptr<const Alignments>::operator*()), DetId(((uint32_t)DetId::Tracker)))'
  263 |     const AlignTransform& globalCoordinates = align::DetectorGlobalPosition(*globalPositions, DetId(DetId::Tracker));
      |                                               ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~

iarspider commented 6 months ago

assign Alignment/OfflineValidation

cmsbuild commented 6 months ago

New categories assigned: alca

@saumyaphor4252,@perrotta,@consuegs you have been requested to review this Pull request/Issue and eventually sign? Thanks

cmsbuild commented 6 months ago

cms-bot internal usage

cmsbuild commented 6 months ago

A new Issue was created by @iarspider.

@smuzaffar, @sextonkennedy, @rappoccio, @Dr15Jones, @antoniovilela, @makortel can you please review it and eventually sign/assign? Thanks.

cms-bot commands are listed here

makortel commented 6 months ago

I guess it's the DetId temporary GCC is worried about. Given the definition of the DetectorGlobalPosition() function https://github.com/cms-sw/cmssw/blob/7e39f77df910152dc5c26df153b8f967d9269331/CondFormats/Alignment/src/DetectorGlobalPosition.cc#L9-L21 the warning looks like a false positive.

Since DetId is cheap to copy (actually cheaper to copy than pass by pointer/reference), in this case the id argument could be changed to be taken by value, that I'd expect to silence the warning.

About this warning in general, I came across https://gcc.gnu.org/bugzilla/show_bug.cgi?id=109642 showing others have had false positive experiences with the -Wdangling-reference warning as well, and it is not going to be improved in GCC 13 (they decided to move it from -Wall to -Wextra).

@iarspider Do you know how many warnings we get from -Wdangling-reference? I'm thinking it might be good to first understand if many of those are false positives, and depending on the needed workarounds, maybe we should consider disabling the warning for GCC 13, and see if things improve in 14?

iarspider commented 3 months ago

@makortel in the last IB six were reported:

one in Alignment/OfflineValidation (the subject of this issue)

src/Alignment/OfflineValidation/plugins/PixelBaryCentreAnalyzer.cc: In member function 'virtual void PixelBaryCentreAnalyzer::analyze(const edm::Event&, const edm::EventSetup&)':
src/Alignment/OfflineValidation/plugins/PixelBaryCentreAnalyzer.cc:262:27: warning: possibly dangling reference to a temporary [-Wdangling-reference]
262 |     const AlignTransform& glbCoord = align::DetectorGlobalPosition(iSetup.getData(gprToken_), DetId(DetId::Tracker));
  |                           ^~~~~~~~
src/Alignment/OfflineValidation/plugins/PixelBaryCentreAnalyzer.cc:262:67: note: the temporary was destroyed at the end of the full expression 'align::DetectorGlobalPosition((* &(& iSetup)->edm::EventSetup::getData<Alignments, GlobalPositionRcd>(((PixelBaryCentreAnalyzer*)this)->PixelBaryCentreAnalyzer::gprToken_)), DetId(((uint32_t)DetId::Tracker)))'
262 |     const AlignTransform& glbCoord = align::DetectorGlobalPosition(iSetup.getData(gprToken_), DetId(DetId::Tracker));
  |                                      ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~

one in CalibTracker/SiStripCommon

src/CalibTracker/SiStripCommon/plugins/TkDetMapESProducer.cc: In member function 'std::unique_ptr<TkDetMap> TkDetMapESProducer::produce(const TrackerTopologyRcd&)':
src/CalibTracker/SiStripCommon/plugins/TkDetMapESProducer.cc:297:15: warning: possibly dangling reference to a temporary [-Wdangling-reference]
297 |   const auto& geomDet = tTopoRcd.getRecord<IdealGeometryRecord>().get(geomDetToken_);
  |               ^~~~~~~
src/CalibTracker/SiStripCommon/plugins/TkDetMapESProducer.cc:297:70: note: the temporary was destroyed at the end of the full expression 'edm::eventsetup::DependentRecordImplementation<RecordT, ListT>::getRecord() const [with DepRecordT = IdealGeometryRecord; RecordT = TrackerTopologyRcd; ListT = edm::mpl::Vector<IdealGeometryRecord, PTrackerParametersRcd>]().IdealGeometryRecord::<anonymous>.edm::eventsetup::DependentRecordImplementation<IdealGeometryRecord, edm::mpl::Vector<GeometryFileRcd> >::<anonymous>.edm::eventsetup::EventSetupRecordImplementation<IdealGeometryRecord>::get<GeometricDet>(((TkDetMapESProducer*)this)->TkDetMapESProducer::geomDetToken_)'
297 |   const auto& geomDet = tTopoRcd.getRecord<IdealGeometryRecord>().get(geomDetToken_);
  |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~

three in CalibTracker/SiStripESProducers

src/CalibTracker/SiStripESProducers/plugins/fake/SiStripBackPlaneCorrectionFakeESSource.cc: In member function 'SiStripBackPlaneCorrectionFakeESSource::ReturnType SiStripBackPlaneCorrectionFakeESSource::produce(const SiStripBackPlaneCorrectionRcd&)':
src/CalibTracker/SiStripESProducers/plugins/fake/SiStripBackPlaneCorrectionFakeESSource.cc:68:15: warning: possibly dangling reference to a temporary [-Wdangling-reference]
68 |   const auto& geomDet = iRecord.getRecord<TrackerTopologyRcd>().get(m_geomDetToken);
  |               ^~~~~~~
src/CalibTracker/SiStripESProducers/plugins/fake/SiStripBackPlaneCorrectionFakeESSource.cc:68:68: note: the temporary was destroyed at the end of the full expression 'edm::eventsetup::DependentRecordImplementation<RecordT, ListT>::getRecord() const [with DepRecordT = TrackerTopologyRcd; RecordT = SiStripBackPlaneCorrectionRcd; ListT = edm::mpl::Vector<TrackerTopologyRcd>]().TrackerTopologyRcd::<anonymous>.edm::eventsetup::DependentRecordImplementation<TrackerTopologyRcd, edm::mpl::Vector<IdealGeometryRecord, PTrackerParametersRcd> >::get<GeometricDet, IdealGeometryRecord>(((SiStripBackPlaneCorrectionFakeESSource*)this)->SiStripBackPlaneCorrectionFakeESSource::m_geomDetToken)'
68 |   const auto& geomDet = iRecord.getRecord<TrackerTopologyRcd>().get(m_geomDetToken);
  |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~
(...)
src/CalibTracker/SiStripESProducers/plugins/fake/SiStripHashedDetIdFakeESSource.cc: In member function 'virtual std::unique_ptr<SiStripHashedDetId> SiStripHashedDetIdFakeESSource::produce(const SiStripHashedDetIdRcd&)':
src/CalibTracker/SiStripESProducers/plugins/fake/SiStripHashedDetIdFakeESSource.cc:34:15: warning: possibly dangling reference to a temporary [-Wdangling-reference]
34 |   const auto& geomDet = record.getRecord<TrackerDigiGeometryRecord>().get(geomDetToken_);
  |               ^~~~~~~
src/CalibTracker/SiStripESProducers/plugins/fake/SiStripHashedDetIdFakeESSource.cc:34:74: note: the temporary was destroyed at the end of the full expression 'edm::eventsetup::DependentRecordImplementation<RecordT, ListT>::getRecord() const [with DepRecordT = TrackerDigiGeometryRecord; RecordT = SiStripHashedDetIdRcd; ListT = edm::mpl::Vector<TrackerDigiGeometryRecord>]().TrackerDigiGeometryRecord::<anonymous>.edm::eventsetup::DependentRecordImplementation<TrackerDigiGeometryRecord, edm::mpl::Vector<IdealGeometryRecord, TrackerAlignmentRcd, TrackerAlignmentErrorExtendedRcd, TrackerSurfaceDeformationRcd, GlobalPositionRcd, TrackerTopologyRcd, PTrackerParametersRcd, PTrackerAdditionalParametersPerDetRcd> >::get<GeometricDet, IdealGeometryRecord>(((SiStripHashedDetIdFakeESSource*)this)->SiStripHashedDetIdFakeESSource::geomDetToken_)'
34 |   const auto& geomDet = record.getRecord<TrackerDigiGeometryRecord>().get(geomDetToken_);
  |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~
(...)
src/CalibTracker/SiStripESProducers/plugins/fake/SiStripLorentzAngleFakeESSource.cc: In member function 'SiStripLorentzAngleFakeESSource::ReturnType SiStripLorentzAngleFakeESSource::produce(const SiStripLorentzAngleRcd&)':
src/CalibTracker/SiStripESProducers/plugins/fake/SiStripLorentzAngleFakeESSource.cc:186:15: warning: possibly dangling reference to a temporary [-Wdangling-reference]
186 |   const auto& geomDet = iRecord.getRecord<TrackerTopologyRcd>().get(m_geomDetToken);
  |               ^~~~~~~
src/CalibTracker/SiStripESProducers/plugins/fake/SiStripLorentzAngleFakeESSource.cc:186:68: note: the temporary was destroyed at the end of the full expression 'edm::eventsetup::DependentRecordImplementation<RecordT, ListT>::getRecord() const [with DepRecordT = TrackerTopologyRcd; RecordT = SiStripLorentzAngleRcd; ListT = edm::mpl::Vector<TrackerTopologyRcd>]().TrackerTopologyRcd::<anonymous>.edm::eventsetup::DependentRecordImplementation<TrackerTopologyRcd, edm::mpl::Vector<IdealGeometryRecord, PTrackerParametersRcd> >::get<GeometricDet, IdealGeometryRecord>(((SiStripLorentzAngleFakeESSource*)this)->SiStripLorentzAngleFakeESSource::m_geomDetToken)'
186 |   const auto& geomDet = iRecord.getRecord<TrackerTopologyRcd>().get(m_geomDetToken);
  |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~~~~~~~~~~~~~~~

one in CalibTracker/SiStripQuality

src/CalibTracker/SiStripQuality/plugins/SiStripQualityHotStripIdentifier.cc: In member function 'void SiStripQualityHotStripIdentifier::bookHistos()':
src/CalibTracker/SiStripQuality/plugins/SiStripQualityHotStripIdentifier.cc:170:83: warning: possibly dangling reference to a temporary [-Wdangling-reference]
170 |   for (const auto& it : SiStripDetInfoFileReader::read(fp_.fullPath()).getAllData()) {
  |                                                                                   ^
src/CalibTracker/SiStripQuality/plugins/SiStripQualityHotStripIdentifier.cc:170:82: note: the temporary was destroyed at the end of the full expression 'SiStripDetInfoFileReader::read(std::string)().SiStripDetInfo::getAllData()'
170 |   for (const auto& it : SiStripDetInfoFileReader::read(fp_.fullPath()).getAllData()) {
  |                         ~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~^~

Dr15Jones commented 3 months ago

one in Alignment/OfflineValidation (the subject of this issue) one in CalibTracker/SiStripCommon three in CalibTracker/SiStripESProducers

These are all false positives. There is a temporary that goes out of scope, but the temporary is just a wrapper around a pointer to a long lived object and the data comes from the long lived object.

one in CalibTracker/SiStripQuality

that one IS a real problem. The container being referenced in the loop goes out of scope before the loop begins!

cms-sw / cmssw

[GCC13] Possible dangling reference in PixelBaryCentreAnalyzer.cc #44619