Segmentation Fault

[dansnyderECE]

I'm having an annoying issue with this code. I get a segmentation fault because of the loop wrapped in comments. Any ideas? This is the goal of this program:

First I wrote a simple c++ program and compiled a binary file for it. I then use readelf to dump the hex instructions into a file which I then read with this program. I read each hex entry as a string, cull the lines in the binary file I don't want, and then convert each hex to a decimal number which I then place into an array. This array represents instruction memory in a processor (I designed a simple MIPS I processor and and trying to run the simple c++ program on it). I use Verilator along with the Verilog modules to test the processor. I'm stuck on this issue though. Any help would be greatly appreciated. I'm still quite new to C++ so it's likely I'm missing something obvious...

Thanks, DS

#include "VMIPS.h"
#include <verilated.h> 
#include <stdio.h>
#include <cmath>
#include <iostream>
#include <fstream>
#include <iomanip>
#include <string>
#include <cstdlib>

using namespace std;

void outputLine( const string );

string array[274815];
unsigned int main_time = 0;
int i=0;
int k=0;
int z = 0;
int main(int argc, char **argv) 
{
        //define local variables/arrays
        int temp;
        char Hex[8];
        bool x = 0;
        
        //define objects
        ifstream inClientFile( "DAN_FILE.txt",ios::in );
        Verilated::commandArgs(argc, argv);
	VMIPS *top = new VMIPS;
	top->CLK = 0;
        
        //test if instruction file can be opened
        if ( !inClientFile )
        {
            cerr << "File couldn't be opened" << endl;
            exit( 1 );
        }
/////////////////////////////////////////////////////////////////////////////////
        //fill string array with all file values and determines length of program
        while ( inClientFile >> Hex )
        {
            //fills array[..] with strings
            outputLine( Hex );
            i++;
        }
/////////////////////////////////////////////////////////////////////////////////
        z = i;
        int DataMem[z];
        int InstructionMemory[z];
        string tempInstructionMemory[z];

        //cut out undesired strings from array
        int u = 0;
        for(i=0;i<274815;i++)
        {
                if ((array[i].length() == 8)&&((array[i].find("fs24")!=1)&&(array[i].find("f0")!=1)))
                {
                        tempInstructionMemory[k] = array[i];
                        k++;
                }
                else
                {
                     u++;   
                }
        }

        //convert string hex to numerical decimal
        for( int j=0;j<(z-u);j++ )//(z-u);j++ )
        {
                for( int y=7;y>=0;y--)
                {
                        if( tempInstructionMemory[j].at(y) ==  '0' ) temp = (0*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  '1' ) temp = (1*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  '2' ) temp = (2*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  '3' ) temp = (3*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  '4' ) temp = (4*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  '5' ) temp = (5*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  '6' ) temp = (6*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  '7' ) temp = (7*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  '8' ) temp = (8*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  '9' ) temp = (9*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  'a' ) temp = (10*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  'b' ) temp = (11*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  'c' ) temp = (12*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  'd' ) temp = (13*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  'e' ) temp = (14*pow(16.,(int)(7-y)));
                        else if( tempInstructionMemory[j].at(y) ==  'f' ) temp = (15*pow(16.,(int)(7-y)));
                        InstructionMemory[j]=InstructionMemory[j]+temp;
                }
                
                temp = 0;

                //cout << "Index:" << j << " HEX:" << tempInstructionMemory[j] << " DEC:" << InstructionMemory[j]<<endl;
        }

        //MIPS I processor interface
	while (!Verilated::gotFinish()) 
	{	
		
		x=!x;
		top->CLK=x;
                top->Iin = InstructionMemory[top->Iaddr];
                top->eval();
                top->Din = DataMem[top->Daddr];
                DataMem[top->Daddr] = top->Dout;
		main_time++;
                //printf("\nIaddr:%d Iin:%d CLK:%d Time:%d",top->Iaddr,top->Iin,top->CLK,main_time);
		if(x)
                {
                    printf("(%d)\n****************************************\n",main_time);
                }
                if(main_time>90)
		{
			printf("\n********** Complete **********\n\n");
			return 0;
		}
	}

        return 0;
}
void outputLine( const string Hex )
{
        array[i] = Hex;
}
/*
 ***** Root Command Call *****

cd Desktop && cd verilator-3.802 && export VERILATOR_ROOT=//Users/dansnyder/Desktop/verilator-3.802 && cd test_MIPS && $VERILATOR_ROOT/bin/verilator --cc MIPS.v --exe sim_main.cpp && cd obj_dir/ && make -j -f VMIPS.mk VMIPS && cd .. && obj_dir/VMIPS && cd .. && cd .. && cd ..

*/

[Aranarth]

My guess is that there are words in the file that are longer than the 7 chars that fit in Hex.
Never use char arrays unless you know exactly what you're doing - use string instead.

By the way, the code inside the for( int y=7;y>=0;y--) loop is unnecessarily redundant.
With the following helper function:

static inline int hexCharValue(char ch)
{
  if (ch>='0' && ch<='9')return ch-'0';
  if (ch>='a' && ch<='f')return ch-'a'+10;
  return 0; //might want to replace that with a throw or assert(false)
}

you can reduce the entire loop body to:

InstructionMemory[j]+=hexCharValue(tempInstructionMemory[j][y])<<(4*(7-y));

Using bit operations instead of pow is approximately ten times faster.
1<<x equals pow(2,x)
1<<(4*x) equals pow(16,x)
y<<(4*x) equals y*pow(16,x)

[daviddoria]

Yes, always use std::string!!

What I really wanted to say was MAKE ABSOLUTELY SURE you comment any lines like 1<<x; EXTREMELY well and TEST THEM INDIVIDUALLY FIRST. I would be extremely annoyed if I found some code like that without a comment with the equivalent math or pow() style statement as a comment. Even though it is 10x faster :)

Dave

[dansnyderECE]

I'll definitely make those changes. I'm working out of my Deitel & Deitel textbook so everything I write is mostly basics... The definitely a more concise way to write this. I was unable to find an elegant way of doing a hex conversion so I just cooked this up in a hurry...

About the seg. fault, lines 55-66 are what I'm using to exclude all unwanted char arrays (anything with length that doesn't equal 8). Isn't this enough? I'll try using a string and see if the issue goes away. I still don't quite understand why this is happening. As far as I understand I'm limiting all char arrays to length 8 and I'm iterating through the main arrays without overstepping the boundaries of each.

[Aranarth]

About the seg. fault, lines 55-66 are what I'm using to exclude all unwanted char arrays (anything with length that doesn't equal 8). Isn't this enough? I'll try using a string and see if the issue goes away. I still don't quite understand why this is happening. As far as I understand I'm limiting all char arrays to length 8 and I'm iterating through the main arrays without overstepping the boundaries of each.

The crash happens long before this part of the code is ever reached:

char Hex[8];
while ( inClientFile >> Hex );

Hex can just hold 7 chars (plus the terminating 0), so if there's a word longer than that, it will overstep the array boundaries.

[dansnyderECE]

How would I go about using a string in this context as opposed to a character array? When I change Hex to a string adn increase the array size arbitrarily to 20 (larger than the longest word in file) then I get a warning about the ">>" istream operator and I also get this error:

../sim_main.cpp:63: error: conversion from ‘std::string*’ to non-scalar type ‘std::string’ requested
make: *** [sim_main.o] Error 1

Which references line 44 in the code I have posted. Any ideas?

[Aranarth]

Hex isn't supposed to be an array of strings! It's just a single one.
I think that's pretty obvious, though. Since you already used std::string in your code, you should know what a string is.

[dansnyderECE]

Oh, right, that makes sense. So, the idea previously was that I had an array of characters. If I did want to use the character array whats stopping me from just making the char array arbitrarily large? I tried this and still got the segfault. I don't quite understand why that is. Does the array have to match the word length exactly?

[Aranarth]

If I did want to use the character array whats stopping me from just making the char array arbitrarily large?

Nothing, but there is no point.
Using character arrays
a) limits the max word length
b) is unsafe
c) wastes memory regardless of word length

I tried this and still got the segfault. I don't quite understand why that is. Does the array have to match the word length exactly?

No, it just has to be larger than the word length.
If it still crashes, it just means that there are more problems. Run it with the debugger to see in which line the problem occurs.

[dansnyderECE]

Hmm, line 124 (in posted code) seems to be the issue meaning array is the wrong size. In the case of this line it seems as those the only plausible declaration (assuming that I don't know how many lines are contained in my file) is to make this array very large. If I do this I still get a segfault. Maybe I'm a bit confused about how exactly the line "array = Hex;"works within the context of the loop it inhabits. Keep in mind that that segment of code was taken directly from a textbook (not of my own conception.)

[dansnyderECE]

Okay, so it turns out that you were right, the issue occurs deeper within the operation chain. "u" is the issue.

[dansnyderECE]

So, I am able to shift the error from segfault to abort trap by the changing of line 69. From what I understand an abort trap occurs when I iterate for too long through this loop and the seg fault occurs when I don't iterate long enough. The issue is that when I change my maximum value in the loop from z-u to z-u-1 I get a seg fault and then an abort trap. Would anyone mind explaining what exactly is happening here?

[Aranarth]

This array of strings has the same problems I mentioned in my previous post, just on a much bigger scale.
You should use a vector instead.

[dansnyderECE]

If I make a string vector will I need to make any major changes to my code? Does vector have length, at and find functions? If I just change my array to vector<string> I get errors pertaining to my use of string operations as those listed above. When I comment them out I still get errors.

Update, so, if I stick with just a string array, I have tracked the fact that this line: "InstructionMemory[j]=InstructionMemory[j]+temp;" is the culprit. If I comment out the addition of temp the seg fault disappears. Strange... I think I'm going to implement your improvement now and see if the issue is still there. It seems as though "pow" is the issue in one way or another. We'll see. Thank you for the help by the way. Your incite has been very useful.

[dansnyderECE]

This is a mystery to me. I implemented the efficient hex conversion and it works fine. I'm going to refer back to my redundant code though as it is more demonstrative of the problem. So, if i comment the following lines:

...
else if( tempInstructionMemory[j].at(y) ==  '4' ) temp = 4<<(4*(7-y));                      
...
else if( tempInstructionMemory[j].at(y) ==  'a' ) temp = 10<<(4*(7-y));
...

then I do not get a segfault. If I change the shift value of the first line (replace (7-y)) to 0-6 and 8+ the fault disappears. Any value within this range gives me a segfault. Same issue with the 2nd line. I can't for the life of me figure out why this is happening as the other conditional statements do not cause a segfault. I'm just adding a constant to temp and then to InstructionMemory[]. The fault also disappears when I comment out the line:

InstructionMemory[j]=InstructionMemory[j]+temp;

or when I change the addition value from temp to some constant. Is there some nuance I'm missing having to do with my int array? I assume that it doesn't have to do with the fact that I'm using int instead of long or something. The conditions later on generate much larger integers and they are fine.

Any ideas?