Post at least line 33 of the file. Try moving line 33 to line 1 and see if the behavior changes. Have you fixed the memory leaks and the way values are returned?
-Lee
Got a tip for us?
Let us know
Become a MacRumors Supporter for $50/year with no ads, ability to filter front page stories, and private forums.
macOS Best way to convert
- Thread starter Miglu
- Start date
- Sort by reaction score
Post at least line 33 of the file. Try moving line 33 to line 1 and see if the behavior changes. Have you fixed the memory leaks and the way values are returned?
-Lee
Removing the line did not work.
0.612400; 0.353600; 0.707100;,
Every line ends in ;,
I deleted the malloc assign of x, y and z, but I am not sure what to do about the line one, as I did not fully understand your explanation of it. I tried deleting (char *) malloc(50), like I did for x, y and z, but then no lines were printed.
0.612400; 0.353600; 0.707100;,
Every line ends in ;,
I deleted the malloc assign of x, y and z, but I am not sure what to do about the line one, as I did not fully understand your explanation of it. I tried deleting (char *) malloc(50), like I did for x, y and z, but then no lines were printed.
OK, I turned this into C i could compile. How many lines are in the file? Somewhere between 525-550? Are you closing and reopening the file every time you are calling your function? If not, are you rewinding the FILE * in some other manner.
As for the free... it should be free(line), not free(&line).
-Lee
As for the free... it should be free(line), not free(&line).
-Lee
There are 600 lines. I tried closing and reopening it and got 9 more lines printed. I think that it is because there was more time.
Doing free(line) causes exactly the same result, SIGABRT and the same thing printed to the console, as when free(&line) is called.
Doing free(line) causes exactly the same result, SIGABRT and the same thing printed to the console, as when free(&line) is called.
If you expect someone to debug this, the first requirement is to replicate the problem. The only way to do that accurately is to process the same data that causes a crash.
Just because you don't see anything special in line 33 (or any other line) doesn't mean there isn't something there. Frankly, you're not the most reliable describer of data. You posted earlier about using fscanf, but failed to mention that the actual separator between numbers contained whitespace. It was necessary for you to post your data before someone else figured it out.
If there is some reason for not posting the data, then explain that reason.
Also, post your most recent code. I see a serious error in the most recently posted code, and I want to see if it's still in your code after your recent changes. The error is here:
Code:
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};
*a = coords;
}What this error confirms to me is that you have an inadequate understanding of pointers and arrays. You don't understand the correspondence between array and pointer, and you don't understand a stack-local array's lifetime (in C references, it's called lifetime of storage class 'auto').
You are now closing and reopening it every time you call this function? If not, try:
rewind(fp);
At the top of the function.
-Lee
EDIT: This isn't the right way to solve a problem, just trying to point out what the problem is. Don't read N lines each time you enter the function, just read the next one. If you have to be able to "randomly" seek to a line based on i you might be in some trouble, but if you just need to read "the next one" reading one line at a time in this function will suffice.
I think that mallocing is necessary for x, y and z in
Code:
char *x, *y, *z;
x = strsep(&line, ";");
y = strsep(&line, ";");
z = strsep(&line, ";");
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};You should not malloc or free these variables. If you ran my strsep example you'd see what tmp is set to each iteration, which is an address in the memory pointed to by the argument to strsep. If you malloc those variables as soon as you assign a new pointer value to them from strsep you lose the pointer from malloc so you can never free that memory. If you want to see this in your own code NSLog or printf with the %p specifier and display lime, then x,y,z after the strsep calls.
You could avoid malloc and free for line by declaring:
char line[50];
char *lineptr = line;
Then passing lineptr to strsep.
How you return your 3 floats is another problem. You cannot return a pointer to stack memory. I would have a large float coords[600][3] outside of this function, and just pass coords[x] (type float *) to the function. Then you can just assign the floats to [1],[2],[3] in the function. malloc would work, too, but you have been struggling with that so this would allow you to avoid it.
-Lee
Only use free on a pointer that was returned from malloc or friends (calloc, etc.). I recommended against using malloc in this case because you were having problems with it and it's not strictly necessary here. It can be difficult (I have known professional programmers and seen code written for-pay that do it wrong) which is a major reason for retain-release in Apple's code and the later addition of Garbage Collection. Note these methods only apply to Objective-C objects, so while you're working with primitives you don't get to use them.
You should use malloc when:
1) you need "a lot" of memory. How much that is is hard to pin down, but the stack has far less room to grow than the heap, so huge arrays or very large structs should be allocated from heap memory.
2) when you can't know how large of an array you'll need at compile-time. In this case most people guess a large number, they are eventually wrong, and a buffer-overflow occurs. This could lead to a crash or cause erroneous behavior. This is also a common source of security exploits.
3) When values need to live after the return of a function and there isn't a passed-in place to store them. After a function returns (its stack frame is popped from the stack) the automatic/stack-local variables' memory is no longer valid to use. If you return a pointer to it, the values stored there will change once a new stack frame is pushed onto the stack and its memory is manipulated in the new function.
There are probably others, but those are the main ones that spring to mind.
Note that manual memory management is hard, and programmers better than us have implemented other memory management schemes because they know that most of us are bad at it. It is definitely worth knowing, but it seems like right now you are just trying to get your feet wet. I don't know exactly because you didn't answer when I asked about what your experience level is and what the purpose of this project is (which would really help in terms of tailoring answers and assumptions about previous knowledge).
-Lee
Because strsep modifies the value you pass a pointer to. If you pass a pointer to a char[] the address of the char[] can't be modified like the pointer value of a char *. line cannot be changed if it is statically allocated, while a char * can be modified. Using malloc in this case for line is problematic because while line is a char * once you change it with strsep you don't have the original pointer to free.
To demonstrate how error-prone this can be, I made a few errors in my example (banged it out quickly). I never tried to free the calloc'd value, and couldn't have if I wanted to because the pointer was changed. A different pointer to a char[] is a better solution.
-Lee
To demonstrate how error-prone this can be, I made a few errors in my example (banged it out quickly). I never tried to free the calloc'd value, and couldn't have if I wanted to because the pointer was changed. A different pointer to a char[] is a better solution.
-Lee
This is the latest code. I can not see any memory leaks.
The amount of lines that are printed before I get EXC_BAD_ACCESS has varied throughout my debugging. It is now 16.
Code:
- (void) getCoords: (int) i Array: (float **) a {
fp2 = fopen("...Coordinates.txt", "r");
char line[50];
char *strptr = line;
for (int j = 0; j < i + 1; j++) {
strptr = fgets(strptr, 50, fp2);
}
char *x, *y, *z;
x = strsep(&strptr, ";");
y = strsep(&strptr, ";");
z = strsep(&strptr, ";");
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};
*a = coords;
}I'm not at a computer (not that you provided a compilable example), but you are now leaking file handles. I don't know that that's why things are going bad, but it's wrong and will cause a crash. Did you try rewinding fp as I suggested earlier?
Also, the way you're returning coords is still wrong.
-Lee
Also, the way you're returning coords is still wrong.
-Lee
Here coords is local to getCoords, so, when you are leaving scope it will disappear. Your supplied pointer a, is now pointing nowhere. You are not copying the content of coords to a, but it's address.
You could solve this by adding x, y and z directly to a, since you have the pointer available inside your function. Of course you also then need to make sure you have room for x, y and z in it.
Call stack:
#0 0x7fff88c4fbeb in __strtodg
#1 0x7fff88c4fac3 in strtof_l
#2 0x100001fbf in -[OpenGLView getCoords:Array:] at OpenGLView.m:192
#3 0x100001586 in -[OpenGLView initWithFrame:] at OpenGLView.m:64
#4 0x7fff803aa198 in -[NSCustomView nibInstantiate]
#5 0x7fff80302263 in -[NSIBObjectData instantiateObject:]
#6 0x7fff8030164e in -[NSIBObjectData nibInstantiateWithOwner:topLevelObjects:]
#7 0x7fff802ffcd9 in loadNib
#8 0x7fff802ff1e9 in +[NSBundle(NSNibLoading) _loadNibFile:nameTable:withZonewnerBundle:]
#9 0x7fff802ff021 in +[NSBundle(NSNibLoading) loadNibNamed
#10 0x7fff802fc5a3 in NSApplicationMain
#11 0x100001011 in main at main.m:13
As an exercise, take all of the declarations in your code and arrange them at the top, before the code of the routine. Then place comments before and after the declarations section that say " these are temporary variables that will not exist after this routine exits". Then examine your code and cross-reference what you to to the declarations block.
It is clearly a matter of style, but I find it easier to keep track of things when the declarations (locals) Are grouped at the top of a routine.
It is clearly a matter of style, but I find it easier to keep track of things when the declarations (locals) Are grouped at the top of a routine.
Luck? But I might be missing something here... BUT, your function does not work as it should, the amount of success varies and the error is EXC_BAD_ACCESS, correct?
Could you point out the problems:
Call stack:
Code:
- (void)makeDisplayList {
fp = fopen("...CoordinateGroups.txt", "r");
fp2 = fopen("...Coordinates.txt", "r");
char *line = (char *) malloc(50);
line = fgets(line, 50, fp);
char *lineNumber = (char *)malloc(5);
char *geometricType = (char *) malloc(1);
int num;
glNewList(1, GL_COMPILE);
while (line) {
geometricType = strsep(&line, ";");
num = strtol(geometricType, NULL, 10);
//This is for GL_QUADS. I have not yet added GL_TRIANGLES coordinate parsing code.
if (num == 4) {
float *coords1 = (float*)malloc(3*sizeof(float));
float *coords2 = (float*)malloc(3*sizeof(float));
float *coords3 = (float*)malloc(3*sizeof(float));
float *coords4 = (float*)malloc(3*sizeof(float));
for (int i = 0; i < 4; i++) {
lineNumber = strsep(&line, ",");
if(strlen(lineNumber) > 0) {
if(lineNumber[strlen(lineNumber)-1] == ';') lineNumber[strlen(lineNumber)-1] = '\0';
}
if (i == 0) {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords1];
printf("%f, %f, %f\n", coords1[0], coords1[1], coords1[2]);
} else if (i == 1) {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords2];
printf("%f, %f, %f\n", coords2[0], coords2[1], coords2[2]);
} else if (i == 2) {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords3];
printf("%f, %f, %f\n", coords3[0], coords3[1], coords3[2]);
} else {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords4];
printf("%f, %f, %f\n", coords4[0], coords4[1], coords4[2]);
}
}
glColor3f(1.0, 1.0, 1.0);
glBegin(GL_QUADS);
{
glVertex3f(coords1[0], coords1[1], coords1[2]);
glVertex3f(coords2[0], coords2[1], coords2[2]);
glVertex3f(coords3[0], coords3[1], coords3[2]);
glVertex3f(coords4[0], coords4[1], coords4[2]);
}
glEnd();
}
line = fgets(line, 50, fp);
}
glEndList();
free(line);
}
- (void) getCoords: (int) i Array: (float **) a {
rewind(fp2);
char line[50];
char *strptr = line;
for (int j = 0; j < i + 1; j++) {
strptr = fgets(strptr, 50, fp2);
}
char *x, *y, *z;
x = strsep(&strptr, ";");
y = strsep(&strptr, ";");
z = strsep(&strptr, ";");
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};
*a = coords;
}#0 0x7fff88c21d94 in __Balloc_D2A
#1 0x7fff88c2234b in __lshift_D2A
#2 0x7fff88c5103c in __strtodg
#3 0x7fff88c4fac3 in strtof_l
#4 0x100001fb9 in -[OpenGLView getCoords:Array:] at OpenGLView.m:196
#5 0x1000015f5 in -[OpenGLView makeDisplayList] at OpenGLView.m:74
#6 0x10000139b in -[OpenGLView initWithFrame:] at OpenGLView.m:39
#7 0x7fff803aa198 in -[NSCustomView nibInstantiate]
#8 0x7fff80302263 in -[NSIBObjectData instantiateObject:]
#9 0x7fff8030164e in -[NSIBObjectData nibInstantiateWithOwner:topLevelObjects:]
#10 0x7fff802ffcd9 in loadNib
#11 0x7fff802ff1e9 in +[NSBundle(NSNibLoading) _loadNibFile:nameTable:withZone
#12 0x7fff802ff021 in +[NSBundle(NSNibLoading) loadNibNamed
#13 0x7fff802fc5a3 in NSApplicationMain
#14 0x100001049 in main at main.m:13
I said to use rewind.
There is a high probability that you are reading off the end of your file, so x y and z are null, so atof fails. That was happening before.
Most likely you're going to get the same pointer returned from every call. So you'll have the last coords in all of your coordsX variables. It will work until another function is called that changes this memory. Do you think we're lying about returning pointers to stack-local memory being wrong?
-Lee
There is a high probability that you are reading off the end of your file, so x y and z are null, so atof fails. That was happening before.
Most likely you're going to get the same pointer returned from every call. So you'll have the last coords in all of your coordsX variables. It will work until another function is called that changes this memory. Do you think we're lying about returning pointers to stack-local memory being wrong?
-Lee
Code:
- (void)makeDisplayList {
fp = fopen("...CoordinateGroups.txt", "r"); [COLOR="Red"]// opened but never closed[/COLOR]
fp2 = fopen("...Coordinates.txt", "r"); [COLOR="Red"]// opened but never closed[/COLOR]
char *line = (char *) malloc(50); [COLOR="Red"]// will be null when free()'ed; reassigned[/COLOR]
line = fgets(line, 50, fp); [COLOR="Red"]// may assign null; 50 is magic number[/COLOR]
char *lineNumber = (char *)malloc(5); [COLOR="Red"]// never freed; reassigned[/COLOR]
char *geometricType = (char *) malloc(1); [COLOR="Red"]// never freed; reassigned; inadequate length[/COLOR]
int num; [COLOR="Red"]// not used outside while block, should be declared inside that block[/COLOR]
glNewList(1, GL_COMPILE);
while (line) {
geometricType = strsep(&line, ";"); [COLOR="Red"]// reassigns to malloc'ed ptr[/COLOR]
num = strtol(geometricType, NULL, 10); [COLOR="Red"]// long assigned to int: may lose precision[/COLOR]
//This is for GL_QUADS. I have not yet added GL_TRIANGLES coordinate parsing code.
if (num == 4) {
float *coords1 = (float*)malloc(3*sizeof(float)); [COLOR="Red"]// never freed; reassigned[/COLOR]
float *coords2 = (float*)malloc(3*sizeof(float)); [COLOR="Red"]// never freed; reassigned[/COLOR]
float *coords3 = (float*)malloc(3*sizeof(float)); [COLOR="Red"]// never freed; reassigned[/COLOR]
float *coords4 = (float*)malloc(3*sizeof(float)); [COLOR="Red"]// never freed; reassigned[/COLOR]
for (int i = 0; i < 4; i++) {
lineNumber = strsep(&line, ",");
if(strlen(lineNumber) > 0) {
if(lineNumber[strlen(lineNumber)-1] == ';')
lineNumber[strlen(lineNumber)-1] = '\0';
} [COLOR="Red"]// no else clause, possible bug: what happens when strlen is 0?[/COLOR]
if (i == 0) { [COLOR="Red"]// use switch/case instead of if/else chain[/COLOR]
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords1];
printf("%f, %f, %f\n", coords1[0], coords1[1], coords1[2]);
} else if (i == 1) {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords2];
printf("%f, %f, %f\n", coords2[0], coords2[1], coords2[2]);
} else if (i == 2) {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords3];
printf("%f, %f, %f\n", coords3[0], coords3[1], coords3[2]);
} else {
[self getCoords:strtol(lineNumber, NULL, 10) Array: &coords4];
printf("%f, %f, %f\n", coords4[0], coords4[1], coords4[2]);
}
[COLOR="Red"]// In all the above blocks, no error-check is done on strtol(),[/COLOR]
[COLOR="Red"]// which returns 0 with errno=EINVAL on errors.[/COLOR]
[COLOR="Red"]// Every call to getCoords:Array: overwrites a malloc'ed ptr[/COLOR]
[COLOR="Red"]// with an out-of-scope ptr to a stack-local float[].[/COLOR]
}
glColor3f(1.0, 1.0, 1.0);
glBegin(GL_QUADS);
{
glVertex3f(coords1[0], coords1[1], coords1[2]);
glVertex3f(coords2[0], coords2[1], coords2[2]);
glVertex3f(coords3[0], coords3[1], coords3[2]);
glVertex3f(coords4[0], coords4[1], coords4[2]);
}
glEnd();
}
line = fgets(line, 50, fp);
[COLOR="Red"]// This fgets() uses a value of line that was advanced by prior strsep() calls.[/COLOR]
[COLOR="Red"]// The sequential actions of strsep() will eventually advance line past the end[/COLOR]
[COLOR="Red"]// of the malloc'ed buffer, and subsequent fgets()'s will damage memory.[/COLOR]
}
glEndList();
free(line); [COLOR="Red"]// will always free null, since line is null after EOF from fgets()[/COLOR]
}
- (void) getCoords: (int) i Array: (float **) a {
rewind(fp2); [COLOR="Red"]// poor design choice: uses fp2 ivar instead of arg[/COLOR]
char line[50]; [COLOR="Red"]// magic number[/COLOR]
char *strptr = line;
for (int j = 0; j < i + 1; j++) {
strptr = fgets(strptr, 50, fp2); [COLOR="Red"]// doesn't break or return on null (EOF)[/COLOR]
}
char *x, *y, *z;
x = strsep(&strptr, ";");[COLOR="Red"]// 3X: doesn't handle null strptr [/COLOR]
y = strsep(&strptr, ";");
z = strsep(&strptr, ";");
float coords[] = {strtof(x, NULL), strtof(y, NULL), strtof(z, NULL)};[COLOR="Red"]// 3X: doesn't handle conversion failures[/COLOR]
*a = coords; [COLOR="Red"]// overwrites malloc'ed ptr; stores address of out-of-scope buffer[/COLOR]
}This code demonstrates a nearly complete lack of understanding of how malloc() and free() work. The design also betrays a lack of understanding of the correlation between a declared C array and a pointer to the array's type. In other words, if you understood the correlation between pointers and arrays, then a completely different and much simpler design would have resulted, and malloc would not even be needed, so the whole issue of using malloc properly would simply never occur.
This code contains so many serious and fundamental errors it would be simpler to rewrite it entirely, without the use of malloc(). This is not only possible, it is entirely practical, since none of the buffers need a dynamic length, nor are any of them large enough that stack space would be exhausted. Furthermore, redesigned code could simply use a 2D float array and eliminate any if/else or switch/case.
If you want the possibility of someone else helping to rewrite this code, you must post an example of the data being read from fp and from fp2. It's too difficult to decipher the data format from the code, since there are no comments or examples in the code itself. You should post enough example data for both files that someone else can actually write the new functions, and run them on the example data placed in test files.
If you are unwilling to post example data, it may take more time for someone to help you, because anyone wishing to do so will first have to make their own test data and run it through the existing code.