PFRecHitTopologyESProducer may access and invalid SoA index

[fwyzard]

Enabling range checks shows that PFRecHitTopologyESProducer may access and invalid SoA index:

----- Begin Fatal Exception 21-Apr-2024 12:07:14 CEST-----------------------
An exception of category 'StdException' occurred while
   [0] Processing  Event run: 379617 lumi: 3 event: 467845 stream: 0
   [1] Running path 'DST_PFScouting_SingleMuon_v2'
   [2] Prefetching for module PFClusterSoAProducer@alpaka/'hltParticleFlowClusterHBHESoA'
   [3] Prefetching for EventSetup module PFRecHitHCALTopologyESProducer@alpaka/'hltESPPFRecHitHCALTopology'
   [4] Calling method for EventSetup module PFRecHitHCALTopologyESProducer@alpaka/'hltESPPFRecHitHCALTopology'
Exception Message:
A std::exception was thrown.
Out of range index in mutable neighbours(size_type index): index -1 vs size 23328
----- End Fatal Exception -------------------------------------------------

[fwyzard]

type pf

[fwyzard]

assign heterogeneous

[cmsbuild]

New categories assigned: heterogeneous

@fwyzard,@makortel you have been requested to review this Pull request/Issue and eventually sign? Thanks

[cmsbuild]

cms-bot internal usage

[cmsbuild]

A new Issue was created by @fwyzard.

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

cms-bot commands are listed here

[fwyzard]

A minimal fix is available in #44790 (for 14.1.x) and #44791 (for 14.0.x).

[fwyzard]

@jsamudio @fllor @hatakeyamak while looking into this, I noticed a realised there is a minor inconsistency in the code:

• neighbours is a matrix of int32_t (signed)
• detId2denseId returns an uint32_t (unsigned)

Is it intended ?
If it is intended, could you add a static_cast in the appropriate places to mark it as such ?
If it is not intended, could you look for a way to make it uniform ?

In either case, could you introduce a named constant to identify invalid dense ids, instead of using -1 or 0xffffffff ?

Thank you !

[fllor]

You are right. A denseId should always be a uint_32t with 0xffffffff representing an invalid entry. The named constant also makes sense.

@jsamudio @hatakeyamak Since I do not have access to the CMS infrastructure anymore, could one of you do the commit?
For the int32_t, I think you just need to change these two lines:

cmssw/DataFormats/ParticleFlowReco/interface/PFRecHitSoA.h

Line 14 in 3fb8b0e

using PFRecHitsNeighbours = Eigen::Matrix<int32_t, 8, 1>;

cmssw/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/PFRecHitProducerKernel.dev.cc

Line 158 in 3fb8b0e

pfRecHits.neighbours(i)(n) = (int32_t)pfRecHit_neighbour;

For the named constant, just do grep -rw 0xffffffff RecoParticleFlow/PFRecHitProducer/ to find all occurrences. Though I am not quite sure right now what is happening in this line:

cmssw/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/PFRecHitProducerKernel.dev.cc

Line 33 in 3fb8b0e

const uint32_t j = alpaka::atomicInc(acc, num_pfRecHits, 0xffffffff, alpaka::hierarchy::Blocks{});

This should probably be left unchanged.

Also introduce the constant here:

cmssw/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/CalorimeterDefinitions.h

Line 104 in 3fb8b0e

return 0;

cmssw/RecoParticleFlow/PFRecHitProducer/plugins/alpaka/CalorimeterDefinitions.h

Line 188 in 3fb8b0e

return 0;

This will then also solve #44608

[jsamudio]

I am currently traveling, but will tackle this once I reach my destination.