Persistent segmentation fault with nested classes

[bobsta]

Hi,

I'm not sure that my terminology of "nested classes" is strictly correct, but I hope you know what I mean. Starting from the top of my object "pyramid" I have a dynamic array of Beam objects each of which Beam object encloses a dynamic array of Segment objects. Each Segment object in turn encloses a SegmentShape object in addition to some other stuff.

i.e. Beam[i]->pSegment[j]->pSegmentShape where typically 5 < i < 10 and 3 < j < 100

For those who might be wondering: the CPP_Sequence *seq pointer is used to traverse a structured file containing the data required to "hydrate" my objects.

class Segment{
public:
	float *mlcLeaves,cumulativeMU,deltaMU,mlcArea;
	friend class Beam;
	friend class SegmentShape;
	SegmentShape *pSegmentShape;
	Beam *parentBeam;

	Segment();
	Segment(Beam *b2,int pairs); // additional constructor if no MLC found all 2*pairs leafs defined with position 0.0
	Segment(Beam *b2, CPP_Sequence *seq);
	void getArea();// calculate area of segment
	void setCumulativeMU(CPP_Sequence *seq); // set cumulative MU for segment
	void setDeltaMU(const int i); // set deltaMU for segment i.e. segment[i].cumulativeMU-segment[i-1].cumulativeMU : used to distinguish "step" and "shoot" segments
	~Segment();
};

class SegmentShape{
public:
	friend class Beam;
	friend class Segment;
	float **value;
	Beam *parentBeam; // link to "parent" beam object

	int x,y;

	SegmentShape();
	SegmentShape(Beam *b2);
	void setSegmentShape(Segment *seg);
	~SegmentShape();
};

Segment.cpp extract

Segment::Segment(Beam *b2,CPP_Sequence *seq):mlcArea(0),parentBeam(b2),cumulativeMU(0),deltaMU(0){
	int k;
	mlcLeaves=NULL;
	char sBuff[32678];
	pSegmentShape=NULL;

	std::cout << "In Segment constructor\n";
	std::cout << "Parent Beam pointer = " << parentBeam << "\n";
	mlcLeaves= new float[2*(parentBeam->leafPairs)];
	for (k=0;k<2*(parentBeam->leafPairs);k++) mlcLeaves[k]=0.0;

	seq->item(0x300A011C)->stringCopy(sBuff);

	// read in B and A leaves as 1D array 0 < k < leafPairs for B leaves and leafPairs < k < (2*leafPairs)-1 for A leaves
//	std::cout <<"Leaf Positions: ";
	mlcLeaves[0] = (float)atof(strtok(sBuff, "\\"));
//	std::cout << mlcLeaves[0] <<"\\";

	for (k=1;k<2*(parentBeam->leafPairs);k++){
		mlcLeaves[k] = (float)atof(strtok(NULL, "\\"));
//		std::cout << mlcLeaves[k]<<"\\";
	}
	// each Segment object has one SegmentShape object defined only AFTER mlc values have been read in
	std::cout << "\nParent Beam pointer = " << parentBeam;
	pSegmentShape=new SegmentShape(parentBeam);

SegmentShape.cpp extract

SegmentShape::SegmentShape(Beam *b2):parentBeam(b2),x(b2->xjaw[1]- b2->xjaw[0]),
										 y(b2->yjaw[1]-b2->yjaw[0]){
	int m,l;
	value=NULL;

	std::cout<<"\n\nIn SegmentShape constructor...\n";
	std::cout << "Parent Beam pointer = " << parentBeam;
	std::cout << "\nX = " << x << "\tY = " << y << "\n";

	value =new float*[y];
	std::cout << "\nNow trying to initialise each of " << y << " rows with " << x << " values  ...\n";

	for (m=0;m<y;m++){
		value[m]=NULL;
		value[m]=new float[x];
	}

	for (m=0,l=0;m<y,l<x;m++,l++) value[m][l]=0.0f;
	std::cout << "\nDone!\n";
}

Firstly I can compile the program creating the necessary static library and link without problem.
As you can see I have inserted various comment lines to check that the correct pointer values are passed from Segment constructor to SegmentShape constructor, i.e. the values of x(210) and y(160) are correctly defined. However when trying to execute the loop at line 13 of SegmentShape.cpp the program crashes producing the segmentation fault. Clearly I am doing something fundamentally wrong, but after several hours of code tinkering and head scratching am still no nearer a solution.

Extract from screen output


In Segment constructor
Parent Beam pointer = 0x806e4e8

In SegmentShape constructor...
Parent Beam pointer = 0x806e4e8
X = 210 Y = 160

Now trying to initialise each of 160 rows with 210 values ...
Segmentation fault


Thanks in advance for taking the time to read this and especially to those replying.

[PanGalactic]

Why are you doing all this dynamic allocation of arrays rather than using vectors?

Are you catching std::bad_alloc anywhere?

[kbw]

seq->item(0x300A011C)->stringCopy(sBuff);

	value =new float*[y];
	for (m=0;m<y;m++){
		value[m]=NULL;
		value[m]=new float[x];
	}

for (m=0,l=0;m<y,l<x;m++,l++) value[m][l]=0.0f;

You may have bigger problems than dealing with nested classes.

[bobsta]

Thanks to PanGalactic and kbw for the replies

PanGalactic: I cut my C++ teeth on dynamic arrays and am simply more at ease with them. But I take the point: vectors are probably the way to go. I thought that things would get a bit tricky with iterators...

Not quite sure what you are trying to say kbw: it's a bit too cryptic for an inexperienced C++ programmer like myself.

The seq->item(0x300A011C)->stringCopy(sBuff); reads in a hexadecimal string of (float) values as a character string which subsequently tokenized using strtok : this string contains the proper (non-zero) values of the mlcLeaves array (lines 16,19-22 of Segment.cpp). This code snippet functions exactly as it should. I didn't want to provide the other 1500 or so lines of code I inherited on which those extracts are built.

As for the second block of code float **value is defined in SegmentShape.cpp at line 22. I have used the for loop to define the value array of dimensions [m][l]. In second for loop I simply initialise all values to be zero.

However I reckon you realise this. I'm probably missing something blindingly obvious ...

[kbw]

What is 0x300A011C?

for (m=0,l=0;m<y,l<x;m++,l++) value[m][l]=0.0f;
should be:

    for (int m = 0; m < y; ++m)
        for (int l = 0; l < x; ++l)
            value[m][l] = 0.0l;

They're not the same thing.

Where is x and y initialised?

It's not good practice to assume that the stack is large enough to accomodate an declaration like:
char sBuff[32678];
You should consider getting that much memory from the heap.

[bobsta]

Thanks for the reply

the hex 0x300A0011C is a tag placed at the location of the data I need to read in to the sBuff. The data file I am reading in conforms to the DICOM 3 data format http://medical.nema.org/

x and y are initiliased within the initialisation list of SegmentShape constructor - see lines 1-2 of SegmentShape.cpp

I need this buffer to read in the relevant data (once located within file) and then to convert the hex/binary into floats i.e. by tokenising with strtok which I can then do stuff with.

I thought the int m = 0 was implicit since m and l are declared as integers at line 3 of SegmentShape.cpp - but it cannot hurt to be specific. I take your point about the prefix incrementation though.

[kbw]

I made an error, value[m][l] = 0.0l, should be as you had it, value[m][l]=0.0f;

The treatment of the stack is system dependent. Windows NT derivatives grow the stack dynamically, but you shouldn't expect the systems in general to cope with a 32K allocation on the stack. It's best to get that much memory from the heap (using new char[32*1024] for example).

In the end, I still haven't commented on your crash. But I'll add that your classes seem tightly coupled, and unncessarily so. Alarm bells should ring when classes need to be friends and when data isn't private. By decoupling your classes. by forcing them to use public interfaces, you increase the reliability of your application. One way to think about it is thinking about how you might change the data structures in one class without impacting the rest of the system.

[bobsta]

You're right. It is not necessary to specify that the classes be friends, since the data isn't private.

I'm running g++ under SuSE linux but I'll look into the stack issue you raise. The underlying code which deals with interpretation of DICOM datafile tags etc. was inherited so I'm not keen to tinker about with it when it's working.

Thanks for your help