Skip to content
Snippets Groups Projects

Draft: Fix segfault at exit of unpacking macros

Closed Draft: Fix segfault at exit of unpacking macros
f.uhlig/cbmroot:exit_segfault_fix into master
3 unresolved threads
Closed Florian Uhlig requested to merge f.uhlig/cbmroot:exit_segfault_fix into master
3 unresolved threads
  • Prevent FairrootManager from touching the source object trhogh its pointer after the run is over
  • Explicitely delete the dynamically allocated Run and Source objects in the macro (give up share ownership of config objects)
  • Give up ownership of the config objects, which triggers their destruction before reaching end of the macro (last owner of shared pointer)

Problem was that the FairrootManager was never going out of scope and was not calling the Source destructor. So all shared pointers and the source objects were destroyed only when going out of scope in the ROOT session, at which time ROOT already deleted some TF1's, THx's and Canvases without asking anybody (global Lists). The source and config destructors were then calling destructors for their members and member's members, where we tried to cleanup the memory we assigned but which ROOT already deleted.

Merge request reports

Loading
Loading

Activity

Filter activity
  • Approvals
  • Assignees & reviewers
  • Comments (from bots)
  • Comments (from users)
  • Commits & branches
  • Edits
  • Labels
  • Lock status
  • Mentions
  • Merge request status
  • Tracking
core/detectors/sts/CbmStsParAsic.h
41 41 double noise, double znr);
42 42
43 43
44 /** @brief Copy constructor **/
44 /** @brief Copy constructor (implicitely disable move constructor and assignment)**/
  • Volker Friese
    Volker Friese @v.friese
    Owner

    Can you explain this comment?

  • Pierre-Alain Loizeau
    Pierre-Alain Loizeau @p.-a.loizeau
    Maintainer

    I seems that be default C++11 (or C++17 I lost track) adds all 4 default copy and move constructor/operator as long as non of them is explicitly declared. But is any of them is declared, the other 3 have to be

    • either explicitly provided
    • or explicitly deleted, in which case they exist but are "null" (see next thread)
    • or not be provided at all
  • Pierre-Alain Loizeau
    Pierre-Alain Loizeau @p.-a.loizeau
    Maintainer

    In fact after remembering that we had many classes at some point with the deleted methods in the private part, I tried it and it does not works.
    False: So if we explicitly delete these constructors and operators, they just have to be made private to prevent the compiler from trying to use them instead of the copy

    Edit: Actually I had tried it on CbmStsParAsic where the compiler did not try anything funny. Doing the same on CbmStsParModule leads to a compilation error even set to private so we really have to rely on the implicit trick:

    /scratch/loizeau/cbmroot/MQ/mcbm/CbmDeviceUnpack.cxx: In member function ‘Bool_t CbmDeviceUnpack::InitContainers()’:
/scratch/loizeau/cbmroot/MQ/mcbm/CbmDeviceUnpack.cxx:213:52: error: use of deleted function ‘CbmStsParModule& CbmStsParModule::operator=(CbmStsParModule&&)’
  213 |       walkMap[0x10107C02] = CbmStsParModule(*parMod);  // Make a copy for storage
      |                                                    ^
In file included from /scratch/loizeau/cbmroot/reco/detectors/sts/unpack/CbmStsUnpackConfig.h:24,
                 from /scratch/loizeau/cbmroot/MQ/mcbm/CbmDeviceUnpack.cxx:21:
/scratch/loizeau/cbmroot/core/detectors/sts/CbmStsParModule.h:125:20: note: declared here
  125 |   CbmStsParModule& operator=(CbmStsParModule&& other) = delete;
      |                    ^~~~~~~~
/scratch/loizeau/cbmroot/MQ/mcbm/CbmDeviceUnpack.cxx:214:52: error: use of deleted function ‘CbmStsParModule& CbmStsParModule::operator=(CbmStsParModule&&)’
  214 |       walkMap[0x101FFC02] = CbmStsParModule(*parMod);  // Make a copy for storage
      |                                                    ^
In file included from /scratch/loizeau/cbmroot/reco/detectors/sts/unpack/CbmStsUnpackConfig.h:24,
                 from /scratch/loizeau/cbmroot/MQ/mcbm/CbmDeviceUnpack.cxx:21:
/scratch/loizeau/cbmroot/core/detectors/sts/CbmStsParModule.h:125:20: note: declared here
  125 |   CbmStsParModule& operator=(CbmStsParModule&& other) = delete;
      |                    ^~~~~~~~
/scratch/loizeau/cbmroot/MQ/mcbm/CbmDeviceUnpack.cxx:223:95: error: use of deleted function ‘CbmStsParModule& CbmStsParModule::operator=(CbmStsParModule&&)’
  223 |         if (walkMap.find(sensor) == walkMap.end()) { walkMap[sensor] = CbmStsParModule(*parMod); }
      |                                                                                               ^
In file included from /scratch/loizeau/cbmroot/reco/detectors/sts/unpack/CbmStsUnpackConfig.h:24,
                 from /scratch/loizeau/cbmroot/MQ/mcbm/CbmDeviceUnpack.cxx:21:
/scratch/loizeau/cbmroot/core/detectors/sts/CbmStsParModule.h:125:20: note: declared here
  125 |   CbmStsParModule& operator=(CbmStsParModule&& other) = delete;
    Edited by Pierre-Alain Loizeau
  • Please register or sign in to reply
  • Volker Friese
    Volker Friese @v.friese started a thread on the diff
    • core/detectors/sts/CbmStsParAsic.h
    41 41 double noise, double znr);
    42 42
    43 43
    44 /** @brief Copy constructor **/
    44 /** @brief Copy constructor (implicitely disable move constructor and assignment)**/
    45 45 CbmStsParAsic(const CbmStsParAsic&);
    46 46
    47 47
    48 /** @brief Move constructor (disabled) **/
    • Volker Friese
      Volker Friese @v.friese
      Owner

      Why is it necessary to remove the move constructor and move assignment?

    • Pierre-Alain Loizeau
      Pierre-Alain Loizeau @p.-a.loizeau
      Maintainer

      Because if they are present (which is the case also when they are = deleted) they are used by default by the compiler for some of the container operations (vectors or maps, I forgot) instead of the copy operators. Which fails the compilation if deleted.

      So if we have any pointer in the class, either we have to provide all 4 of them in a clean and complete way or we have to make sure they are not present at all.

      The question is whether we have a need to move the objects, in which case we should also take care of moving the ownership of the TF1. For now only the automatic usage by the compiler seems to be present as removing them did not break any compilation, and I have the feeling that these Par objects are small enough that we do not care.

    • Please register or sign in to reply
  • Volker Friese
    Volker Friese @v.friese started a thread on the diff
    • core/detectors/sts/CbmStsParAsic.cxx
    128 gROOT->cd();
    129
    130 /// Increment the index to avoid name collision
    131 if (0 == fInstanceIdx) {
    132 ++gAsicInstanceIndex;
    133 fInstanceIdx = gAsicInstanceIndex;
    134 }
    135
    136 fNoiseCharge = new TF1(Form("Noise_Charge_%d", fInstanceIdx), "TMath::Gaus(x, [0], [1])", fThreshold, 10. * fNoise);
    146 137 fNoiseCharge->SetParameters(0., fNoise);
    147 138 fIsInit = kTRUE;
    139
    140 /// Restore old global file and folder pointer to avoid messing with FairRoot
    141 gFile = oldFile;
    142 gDirectory = oldDir;
    148 143 }
    • Comment on lines -145 to 148
      Volker Friese
      Volker Friese @v.friese
      Owner

      OK, if such kind of gymnastics are needed, this basically tells me that one should not use TF1 for this purpose in this class.

    • Pierre-Alain Loizeau
      Pierre-Alain Loizeau @p.-a.loizeau
      Maintainer

      It seems Florian found an easier way with an option telling ROOT to ignore the new TF1 and not store it in its Lists. Which should also fix the naming collision. I will update the commit to use it also here.

    • Please register or sign in to reply
  • CBM Dummy User
    CBM Dummy User @cbmwww
    Developer

    Dear @v.friese, @f.uhlig, @p.-a.loizeau, @v.singhal, @s.lebedev, @n.herrmann, @i.deppner, @a.bercuci, @p.kaehler,

    you have been identified as code owner of at least one file which was changed with this merge request.

    Please check the changes and approve them or request changes.

  • CBM Dummy User added CodeOwners label

    added CodeOwners label

  • Florian Uhlig closed

    closed

    • Please register or sign in to reply