Problem 1254

New users in my group seem to always run into memory handling issues when it comes to rotating volumes during placement. What makes this issue difficult is that a user has to have a good understanding of how G4VPhysicalVolume and its derived classes internally handle the memory management of the rotation matrices in order to be able to avoid problems in all situations.

In G4VPhysicalVolume, the rotation matrix is received by pointer via either the constructor or the function SetRotation(). Internally, G4VPhysicalVolume simply copies the pointer value it is sent, so the user must take care not to delete the associated memory as long as the G4VPhysicalVolume is still being used. One common pitfall is for users to allocate a rotation matrix on the stack during the ConstructDetector() method, which goes out of scope upon return of the function, resulting in a seg fault during navigation. Another common pitfall is to allocate the matrices on the heap and never delete them. This leads to orphaned memory at the end of execution, which can clutter the output of memory management tools such as valgrind. This can also lead to actual memory leaks in applications with multiple geometry re-initializations. To avoid these problems, the user must keep a list of all the rotation matrices allocated, and be sure to delete them and reallocate them at appropriate times.

It would be much simpler if G4VPhysicalVolume would instead copy the rotation matrix as necessary and handle the memory for its own rotation matrices by itself. I believe this can be accomplished by changing four functions: the G4VPhysicalVolume constructor and destructor, G4VPhysicalVolume::SetRotation, and the G4PVPlacement destructor:

G4VPhysicalVolume::G4VPhysicalVolume( G4RotationMatrix *pRot,
                                const G4ThreeVector &tlate,
                                const G4String& pName,
                                      G4LogicalVolume* pLogical,
                                      G4VPhysicalVolume* )
  : frot(NULL), ftrans(tlate), flogical(pLogical),
    fname(pName), flmother(0)
{ 
  if(pRot != NULL) frot = new G4RotationMatrix(*pRot);
  G4PhysicalVolumeStore::Register(this);
}

G4VPhysicalVolume::~G4VPhysicalVolume()
{
  delete frot;
  G4PhysicalVolumeStore::DeRegister(this);
}

inline
void G4VPhysicalVolume::SetRotation(G4RotationMatrix *pRot)
{
  if(pRot != NULL) frot = new G4RotationMatrix(*pRot);
  else frot=NULL;
}

G4PVPlacement::~G4PVPlacement()
{ 
  // remove: now done in ~G4VPhysicalVolume
  // if( fallocatedRotM ){ delete frot; }
}

This would be completely backwards compatible in the sense that no public or protected interface changes. And users would no longer have to concern themselves with the inner workings of this class in order to avoid memory leaks and segmentation faults. These changes would also make some of the machinery in G4PVPlacement for handling the frot memory unnecessary, such as fallocatedRotM, NewPtrRotMatrix, etc, simplifying that class considerably.

Thanks,
Jason Detwiler