SIGABRT: TH1D array and reading from TFile

ROOT
1

Hi folks,

I’ve got a problem trying to read TH1D histograms from files given in the command line to an array of histograms. The minimal example looks like the following

#include <iostream>
#include "TFile.h"
#include "TH1.h"

using namespace std;

int main(int argc, char **argv) {

  TH1D* hist = new TH1D[1];

  for (int i = 0; i < 1; i++) {
    TFile *tmpFile = new TFile(argv[1+i], "READ");
    hist[i] = *((TH1D *) tmpFile->Get("h1"));
    delete tmpFile;
  }
}

which results in a SIGABRT error gdb says it is related to the delete tmpFile; line. But if I don’t delete tmpFile the SIGABRT error will arise at the end of the program anyway. Of course normally I would create a TH1D* pointing to an array of argc-1 histograms.

Now comparing to this code

#include <iostream>
#include "TFile.h"
#include "TH1.h"

using namespace std;

int main(int argc, char **argv) {

  TH1D* hist = new TH1D();

  for (int i = 0; i < 1; i++) {
    TFile *tmpFile = new TFile(argv[1+i], "READ");
    hist[i] = *((TH1D *) tmpFile->Get("h1"));
    delete tmpFile;
  }
}

where I only create a pointer to a default histogram, no such error occurs.
I am compiling the code via

g++ -g -o test.exe $(root-config --cflags --glibs) minex.cxx

and call via

gdb --args test.exe bla.root

What am I doing wrong? Is it something stupid?

2

Instead of:
hist[i] = *((TH1D ) tmpFile->Get(“h1”));
try:
tmpFile->GetObject(“h1”, hist[i]); if (hist[i]) hist[i]->SetDirectory(0);
and properly define:
TH1D hist[1];
or … try: [code]#include “TFile.h”
#include “TH1.h”

int main(int argc, char **argv)
{
if (argc < 2) return -1; // just a precaution

TH1D *hist = new TH1D[(argc - 1)];

for (int i = 0; i < (argc - 1); i++) {
TFile *tmpFile = TFile::Open(argv[(i + 1)], “READ”);
if (tmpFile) {
tmpFile->GetObject(“h1”, hist[i]);
if (hist[i]) hist[i]->SetDirectory(0);
} else hist[i] = 0;
delete tmpFile;
}

for (int i = 0; i < (argc - 1); i++) delete hist[i];
delete[] hist;

return 0;
}[/code]

3

The line

hist[i].SetDirectory(0);

indeed did the trick.

So the following code now works

#include <iostream>
#include "TFile.h"
#include "TH1.h"

using namespace std;

int main(int argc, char **argv) {

  TH1D* hist = new TH1D[argc-1];

  for (int i = 0; i < argc-1; i++) {
    TFile *tmpFile = new TFile(argv[1+i], "READ");
    hist[i] = *((TH1D *) tmpFile->Get("h1"));
    hist[i].SetDirectory(0);
    delete tmpFile;
  }
}

Thank you for the help.

So this looks like an ownership issue, if it is solved by SetDirectory, right? But then I don’t understand why the example in my first post without array is working (since there I don’t use SetDirectory).

4

[quote=“fkoenig”]The line

  TH1D* hist = new TH1D[argc-1]; // (1)
  ///
  hist[i] = *((TH1D *) tmpFile->Get("h1")); //(2)

So this looks like an ownership issue, if it is solved by SetDirectory, right? But then I don’t understand why the example in my first post without array is working (since there I don’t use SetDirectory).[/quote]
In (1) you assign an an array of TH1Ds to a pointer to a TH1D. This works
because arrays decay to pointers in C, but is still ugly and confusing.

In (2) you can then use your array. The type of hist[0] is not TH1D*, but
TH1D so you need to dereference the return value of tmpFile->Get to make this
compile. You then try to copy-construct inside the value pointed to by
hist[0]. This does not work for me, but segfaults.

What you probably really wanted to do is store the pointers returned from the
file in an array and keep them from being deleted by the TFile destructor.
Your first example did something completely different.

5

Here I was assuming this would be the thing to do in C++ (instead of using malloc). Like suggested here cplusplus.com/reference/new/ … new%5B%5D/

Actually because I didn’t want to get in trouble with ROOT’s ownership policies, what I wanted to do is the following:

Create an array of TH1Ds dynamically (that’s why I used new).
Read one file at a time.
Get a pointer to the histogram stored in the file and then copy the histogram pointed to (i.e. the value the pointer points at) into my array of histograms, so line (2) I wrote down intentionally.
Then I assumed the deletion or closure of the TFile wouldn’t affect the histograms in the array since those should be copies of those owned by the file.

Well thanks again for the input. I guess I’ll just be happy that it is working now.

6

Hi fkoenig,

[quote=“fkoenig”][quote=“honk”]
Actually because I didn’t want to get in trouble with ROOT’s ownership
policies, what I wanted to do is the following:

Create an array of TH1Ds dynamically (that’s why I used new[]).
Read one file at a time.
Get a pointer to the histogram stored in the file and then copy the histogram pointed to (i.e. the value the pointer points at) into my array of histograms, so line (2) I wrote down intentionally.
Then I assumed the deletion or closure of the TFile wouldn’t affect the histograms in the array since those should be copies of those owned by the file.

[/quote][/quote]

There is no need to use low-level dynamic-sized arrays in C++ since it comes
with a number of types which can do what you want. The simplest is
probably std::vector which can be resized and behaves largely like a C-array.

int main(int argc, const char *argv[]) {
  // create a vector holding TH1D*, size argc-1 and all values initialized to 0
  std::vector<TH1D*> hs(argc-1, 0);

  for (int i=0; i<argc-1; ++i) {
    TFile f(argv[i+1]); // no need to makes these pointers, default mode is READ
    f.Get("h1", hs[i]); // get a pointer to "h1" from file and put it into hs[i]

    if (hs[i]) {
      hs[i]->SetDirectory(0); // if we got h1 take ownership
    }
  }

  for (unsigned i=0; i<hs.size(); ++i) {
    // do something with hs[i], e.g.
    hs[i]->Draw();
  }

  // since we own the hs clean up their memory
  for (unsigned i=0; i<hs.size(); ++i) {
    delete hs[i];
  }
  // no need to clean up the vector itself
}

HTH,

b.