Skip to content

Fix valgrind#4323

Merged
pinkenburg merged 2 commits into
sPHENIX-Collaboration:masterfrom
pinkenburg:fix-valgrind
Jun 27, 2026
Merged

Fix valgrind#4323
pinkenburg merged 2 commits into
sPHENIX-Collaboration:masterfrom
pinkenburg:fix-valgrind

Conversation

@pinkenburg

@pinkenburg pinkenburg commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Types of changes

  • [ X] Bug fix (non-breaking change which fixes an issue)
  • New feature (non-breaking change which adds functionality)
  • Breaking change (fix or feature that would cause existing functionality to not work for users)
  • Requiring change in macros repository (Please provide links to the macros pull request in the last section)
  • I am a member of GitHub organization of sPHENIX Collaboration, EIC, or ECCE (contact Chris Pinkenburg to join)

What kind of change does this PR introduce? (Bug fix, feature, ...)

This should fix the valgrind errors from the CaloFitting test

TODOs (if applicable)

Links to other PRs in macros and calibration repositories (if applicable)

This PR addresses valgrind failures in the CaloFitting test by tightening memory cleanup in a couple of reconstruction components. Please use best judgment when reviewing this summary, as AI-generated summaries can make mistakes.

Motivation / context

  • Fix leaks and stale ROOT fitter state reported by valgrind in calorimeter-related tests.
  • Improve destructor cleanup for objects created during run initialization.

Key changes

  • CaloTowerBuilder:
    • Deletes cdbttree_sepd_map in the destructor to release the SEPD-specific CDB tree allocated during InitRun.
    • Minor include reordering only; no functional change.
  • MbdSig:
    • Adds ROOT header dependencies needed for fitter-related cleanup.
    • Resets the global ROOT fitter cache with TVirtualFitter::SetFitter(nullptr, 0) in the destructor before deleting member objects.
  • MbdSig.h:
    • Reworks ROOT type usage with forward declarations and header cleanup to reduce coupling.

Potential risk areas

  • ROOT/global state cleanup may affect shared fitter behavior if other code assumes the cached fitter persists.
  • Destructor changes could subtly alter reconstruction lifecycle behavior or memory ownership assumptions.
  • Header dependency changes should be checked for build consistency and any downstream compile impacts.
  • No IO format changes are expected, but reconstruction outputs should still be validated.

Possible future improvements

  • Audit similar reconstruction classes for missing destructor cleanup and global ROOT state handling.
  • Add targeted regression tests or valgrind-focused checks for these components.
  • Review ownership patterns around CDB trees and fitter objects to make cleanup more explicit.

@coderabbitai

coderabbitai Bot commented Jun 26, 2026

Copy link
Copy Markdown
Contributor

Review Change Stack

No actionable comments were generated in the recent review. 🎉

ℹ️ Recent review info
⚙️ Run configuration

Configuration used: Path: .coderabbit.yaml

Review profile: CHILL

Plan: Pro

Run ID: de253145-c5bd-4809-b4d5-6174789c45f8

📥 Commits

Reviewing files that changed from the base of the PR and between c89585e and dcde384.

📒 Files selected for processing (3)
  • offline/packages/CaloReco/CaloTowerBuilder.cc
  • offline/packages/mbd/MbdSig.cc
  • offline/packages/mbd/MbdSig.h

📝 Walkthrough

Walkthrough

The PR reorders includes in two modules, adds deletion of a calibration mapping tree in one destructor, and updates another module’s ROOT declarations plus destructor to reset ROOT’s cached fitter before member deletion.

Changes

CaloTowerBuilder changes

Layer / File(s) Summary
Include order and tree deletion
offline/packages/CaloReco/CaloTowerBuilder.cc
The include block is reordered, and the destructor deletes cdbttree_sepd_map with the existing members.

MbdSig ROOT declarations and fitter reset

Layer / File(s) Summary
ROOT declarations
offline/packages/mbd/MbdSig.h
The header adds <Rtypes.h> and <limits> and replaces the direct TH1.h include with ROOT forward declarations.
Source includes and fitter reset
offline/packages/mbd/MbdSig.cc
The source include block adds TH1.h and TVirtualFitter.h, and the destructor calls TVirtualFitter::SetFitter(nullptr, 0) before deleting members.

Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out.

❤️ Share

Comment @coderabbitai help to get the list of available commands.

@sphenix-jenkins-ci

Copy link
Copy Markdown

Build & test report

Report for commit dcde38478d4bf977cd43cea963b961e5dcb28396:
Jenkins passed


Automatically generated by sPHENIX Jenkins continuous integration
sPHENIX             jenkins.io

@pinkenburg pinkenburg merged commit 7b251ee into sPHENIX-Collaboration:master Jun 27, 2026
23 checks passed
@pinkenburg pinkenburg deleted the fix-valgrind branch June 27, 2026 14:44
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

1 participant