Uploaded image for project: 'ROOT'
  1. ROOT
  2. ROOT-3579

RooTreeDataStore not Cloning the tree properly (and const correctness)




      Line 45x-47x of RooTreeDataStore.cxx which reads:
      if (dynamic_cast<const TChain*>(t)) {
      tClone = (TTree*) t->Clone() ;
      } else {
      tClone = ((TTree*)t)->CloneTree() ;
      should be
      tClone = ((TTree*)t)->CloneTree() ;

      Yes, I saw the comment on the top of the function says
      // Clone source tree
      // WVE Clone() crashes on trees, CloneTree() crashes on tchains
      I don't know if this statement is still valid but with the Clone() function my program crashes ~1 in 5 times(forcing it to do CloneTree fixes the problem). I guess there is a subtle difference between Clone() and CloneTree() which I didn't look into (may be it's clone/clonetree bug). You can test it out with something as simple as (I compiled it with O2. This may be relevant)

      #include <TChain.h>
      #include <RooDataSet.h>
      #include <RooDataHist.h>
      #include <RooRealVar.h>
      #include <RooArgSet.h>
      #include <iostream>
      int main(int argc, const char* argv[]){
      TChain chain("cand" chain.Add("data/fitcache/bbbar_b.root"
      RooRealVar mes("Mes","Mes",5.28,5.24,5.29);
      RooRealVar weight("weight","weight",1,0,100);
      RooDataSet* data = new RooDataSet("dataset", "dataset", &chain,
      RooDataHist* hist = data->binnedClone();
      //just to prevent compiler trying to be smart and skip some code
      std::cout << hist << endl;
      delete hist;//invalid read here
      delete data;//and here too
      return 1;

      which valgrind will complain something like (it has some chance of crashing)
      ==5235== Invalid read of size 1
      ==5235== at 0x10025CBC6: TList::FindLink(TObject const*, int&amp const (in /usr/local/src/root5.28/lib/libCore.so)
      ==5235== by 0x10025D9B8: TList::Remove(TObject*) (in /usr/local/src/root5.28/lib/libCore.so)
      ==5235== by 0x10025B7E6: THashList::RecursiveRemove(TObject*) (in /usr/local/src/root5.28/lib/libCore.so)
      ==5235== by 0x100198F6D: TObject::~TObject() (in /usr/local/src/root5.28/lib/libCore.so)
      ==5235== by 0x1022B32AC: TTree::~TTree() (in /usr/local/src/root5.28/lib/libTree.so)
      ==5235== by 0x1037421F4: RooTreeDataStore::~RooTreeDataStore() (in /usr/local/src/root5.28/lib/libRooFitCore.so)
      ==5235== by 0x1035F9990: RooAbsData::~RooAbsData() (in /usr/local/src/root5.28/lib/libRooFitCore.so)
      ==5235== by 0x10366EE38: RooDataHist::~RooDataHist() (in /usr/local/src/root5.28/lib/libRooFitCore.so)
      ==5235== by 0x1000F2C59: main (in bin/roofittest)
      ==5235== Address 0x108e814ff is 15 bytes inside a block of size 600 free'd
      ==5235== at 0x10017ADAF: operator delete(void*) (vg_replace_malloc.c:387)
      ==5235== by 0x1001A1577: TStorage::ObjectDealloc(void*) (in /usr/local/src/root5.28/lib/libCore.so)
      ==5235== by 0x1037434E9: RooTreeDataStore::loadValues(TTree const*, RooFormulaVar const*, char const*, int, int) (in /usr/local/src/root5.28/lib/libRooFitCore.so)
      ==5235== by 0x103743786: RooTreeDataStore::RooTreeDataStore(char const*, char const*, RooArgSet const&, TTree&, char const*, char const*) (in /usr/local/src/root5.28/lib/libRooFitCore.so)
      ==5235== by 0x10367BBCB: RooDataSet::RooDataSet(char const*, char const*, TTree*, RooArgSet const&, char const*, char const*) (in /usr/local/src/root5.28/lib/libRooFitCore.so)
      ==5235== by 0x1000F2C1D: main (in bin/roofittest)

      My suggestion at the top is more like a workaround than a fix. It gives rise to another problem which is "t" was declared as a const TTree* but CloneTree is not defined as const function(I think because it keep track of cloned tree connected to it). May be the change in the header is required because compiler might decide to optimized it based on the const declaration of t.





            shageboe Stephan Hageboeck
            piti Piti Ongmongkolkul (Inactive)
            0 Vote for this issue
            4 Start watching this issue