PDA

View Full Version : C program printing out junk; possible pointer issue




mac2x
Nov 27, 2010, 01:22 AM
First off, I'm sure this is because I've just started working seriously with pointers. I'm sure I'm doing something amiss.

Extensive testing has told me that the contents of the arrFor are being corrupted somehow between the allocateArray function and the display function. I *suspect* that it has something to do with passing arrFor to the displayResults function. However, I've reached the point where I can't see how to fix it anymore and need outside help. Thanks!

N.B. arrBack remains uncorrupted; that's why i suspect the function call to displayResults.



/* This program uses pointers to read into an array from a file, dynamically
allocate a second array to fit the data read exactly, and fill the new array
with the reverse of the original array entirely using pointer arithmetic. */

const int MAX_SIZE = 51;

#include <stdio.h>
#include <stdlib.h>

int getData( FILE * is, const int MAX_SIZE, int * );
void allocateArray( int *, int * numberRead );
void displayResults( FILE * os, int *, int *, int * numberRead );

int main()
{
int arrayForward[MAX_SIZE];
int * arrayForPtr;
arrayForPtr = arrayForward;
// int *arrayBackward;

FILE * fin;
fin = fopen( "array.txt", "r" );
if ( fin == NULL )
{
perror( "Error opening input file\n" );
// system( "pause" ); only for Dev C++
exit(1);
}

//get data
int numberRead = 0;
numberRead = getData( fin, MAX_SIZE, arrayForPtr );

fclose( fin );

allocateArray( arrayForPtr, &numberRead );

// displayResults( stdout, arrayForward, arrayBackward, &numberRead );

return 0;
}

int getData( FILE * is, const int MAX_SIZE, int * arrFor )
{
int nu = 0;
int used = 0;
int * arrayEnd = arrFor + (MAX_SIZE - 1);

while ( arrFor < arrayEnd && fscanf( is, "%i", arrFor ) == 1 )
{
++arrFor;
// printf("%4i\n", arrFor[nu - 1]);
used++;
}

//temporary print to see the data read
/* int u;
for ( u = 0 ; u < nu ; u++ )
{
printf("%4i\n", arrFor[u]);

} */

return used;
}

void allocateArray( int * arrFor, int * numberRead )
{
int * arrBack;
arrBack = (int *)calloc( *numberRead, sizeof(int) ); //allocate array
// arrayBackPtr = arrayBack;
int * arrayEnd = arrFor + (*numberRead); //find bounds
// int * end;
// end = &arrayEnd;

//fill new array with contents of first in reverse order

//use subscripts
/* int u = 0, count = *numberRead;
while ( u < *numberRead )
{
if ( count > 0 )
{
arrayBack[count - 1] = arrFor[u];
count--;
}
u++;
} */

//use pointer arithmetic
while ( arrFor < arrayEnd )
{
if ( arrayEnd + 1 > arrFor )
{
*(arrBack - 1) = *arrFor;
arrBack--;
}
arrFor++;
}

//possibly faulty call to displayResults
displayResults( stdout, arrFor, arrBack, numberRead );

//temporary print

/* printf("\n");
for ( int count = 0 ; count < *numberRead ; count++ )
{
printf("%4i\n", arrBack[count]);
} */

}

void displayResults( FILE * os, int * arrFor, int * arrBack, int * numberRead )
{
int * arrayEndFor = arrFor + (*numberRead); //bounds for original array
int * arrayEndBack = arrBack + (*numberRead); //bounds for reversed array

fprintf( os, "%24s\n\n%14s%17s\n", "The Arrays", "Original", "Reverse" );

int nu = 0;
/* while ( nu < *numberRead )
{
printf("%4i\n", arrFor[nu] );
nu++;
} */
while ( arrFor < arrayEndFor && arrBack < arrayEndBack )
{
fprintf( os, "%14i%17i\n", *arrFor, *arrBack );
arrFor++, arrBack++;
}
free( arrBack );

}

input file (array.txt):


62
55
83
90
97
104
107
148
156
160
186
190
209
217
226
232
240
283
291
295

output (to stdout):


The Arrays

Original Reverse
1606419960 295
32767 291
7 283
0 240
1606664232 232
32767 226
1606416440 217
32767 209
1606416384 190
32767 186
1606424274 160
32767 156
0 148
0 107
1606418464 104
32767 97
0 90
0 83
1606416424 55
32767 62



mac2x
Nov 27, 2010, 01:34 AM
Please note that anyone who has read the OP as of this post has NOT seen the latest version of the code. I fixed a few flaws (like not freeing the dynamically allocated memory). However, the output is still partially junk.

lee1210
Nov 27, 2010, 04:39 AM
Please note that anyone who has read the OP as of this post has NOT seen the latest version of the code. I fixed a few flaws (like not freeing the dynamically allocated memory). However, the output is still partially junk.

Then post the latest so we aren't chasing specters.

-Lee

mac2x
Nov 27, 2010, 11:33 AM
I did. When I said I fixed them, I meant exactly what I said. I fixed them, both here and in the source file. :)

chown33
Nov 27, 2010, 12:13 PM
Add the code hilited in red:


/* This program uses pointers to read into an array from a file, dynamically
allocate a second array to fit the data read exactly, and fill the new array
with the reverse of the original array entirely using pointer arithmetic. */

const int MAX_SIZE = 51;

#include <stdio.h>
#include <stdlib.h>

int getData( FILE * is, const int MAX_SIZE, int * );
void allocateArray( int *, int * numberRead );
void displayResults( FILE * os, int *, int *, int * numberRead );

static void
showArray( int * array, int len )
{
printf( "--- len: %d ---\n", len );
while ( len-- > 0 )
{ printf( "%4d\n", *array++ ); }
}


int main()
{
int arrayForward[MAX_SIZE];
int * arrayForPtr;
arrayForPtr = arrayForward;
// int *arrayBackward;

FILE * fin;
fin = fopen( "array.txt", "r" );
if ( fin == NULL )
{
perror( "Error opening input file\n" );
// system( "pause" ); only for Dev C++
exit(1);
}

//get data
int numberRead = 0;
numberRead = getData( fin, MAX_SIZE, arrayForPtr );

fclose( fin );

showArray( arrayForPtr, numberRead );

allocateArray( arrayForPtr, &numberRead );

// displayResults( stdout, arrayForward, arrayBackward, &numberRead );

showArray( arrayForPtr, numberRead );

return 0;
}

Sample output:
--- len: 20 ---
62
55
83
90
97
104
107
148
156
160
186
190
209
217
226
232
240
283
291
295
The Arrays

Original Reverse
-1610547840 : 295
-1073743601 : 291
-1073743640 : 283
-1879043646 : 240
-1610547840 : 232
12314 : 226
-1880799136 : 217
-1879044454 : 209
-1879044454 : 190
0 : 186
-1073743576 : 160
-1879044387 : 156
-1610547840 : 148
-1610547588 : 107
1 : 104
-1073743392 : 97
-1073743300 : 90
-1073743400 : 83
-1073743504 : 55
-1881067089 : 62
--- len: 20 ---
62
55
83
90
97
104
107
148
156
160
186
190
209
217
226
232
240
283
291
295

Clearly, the original array is not corrupted. Which means something else is wrong.

I suggest adding additional calls to showArray() at strategic points in other functions, so you can see what happens. I've already found the problem, but it's better that you gain the debugging experience so I'm not going to give the answer yet.

Also, you have some very weird design choices, like passing numberRead by its address instead of by value. There's no reason the pass by address, and it makes certain things clumsy or error prone. That's not the error, but it's one waiting to happen.

You also have a redundant test in this code:
//use pointer arithmetic
while ( arrFor < arrayEnd )
{
if ( arrayEnd + 1 > arrFor )
{
*(arrBack - 1) = *arrFor;
arrBack--;
}
arrFor++;
}

The 'while' condition has already been evaluated by the time the 'if' condition is evaluated. Since the one is subsumed by the other, there is no purpose for the if: i.e. it can never happen.

There are other oddness issues, but you need to find the bug first.

willieva
Nov 27, 2010, 12:14 PM
You make the call to displayResults( stdout, arrFor,...) right after a loop where you incremented the arrFor pointer. arrFor is no longer pointing at the beginning of the array.

SidBala
Nov 27, 2010, 01:17 PM
You are decrementing off arrBack in allocate Array.

//use pointer arithmetic
while ( arrFor < arrayEnd )
{
if ( arrayEnd + 1 > arrFor )
{
*(arrBack - 1) = *arrFor;
arrBack--;
}
arrFor++;
}


arrBack is pointing to the start of the backwards array and not the end of it!

So it is running into garbage memory and you are not really assigning anything into the real backwards array. It is getting assigned into a memory location in front of the backwards array.

SidBala
Nov 27, 2010, 01:23 PM
Also in allocarray:

//possibly faulty call to displayResults
displayResults( stdout, arrFor, arrBack, numberRead );

you can't do that since you have modified arrFor. arrFor is now pointing to the end of the forwards array and not to the start. Hence it is corrupted since there is nothing stored beyond the end of the forwards array.

If you made the change I suggested in the previous post, then arrBack should be pointing in the correct place.



PS: When you do arrFor ++, you are changing arrFor permanently. So when you want to do something again with arrFor, you need to get it back to the original location it was pointing to. So use a "position" or something as the pointer you increment and decrement:


int * forPos = arrFor;
int * backPos = arrBack's last element;


while(forPos < the end of arrFor)
{

*backPos = *forPos;

backPos --;
forPos ++;
}

display(arrFor,arrBack)
free(arrFor);
free(arrBack);

mac2x
Nov 27, 2010, 01:28 PM
Thanks for the pointers (pun intended!!). I made a few fundamental changes, such as returning the pointer to my backwards array to main and making the call to displayResults() in main rather than in allocateArray(), which was making things way too convoluted.

chown33: I would be interested to hear what is weird.....I feel this revision has fixed some of that though. BTW, your extra code caused a segfault; I added it exactly as indicated. I spotted the issue without it anyway (no I hadn't seen willieva's post yet).

/* This program uses pointers to read into an array from a file, dynamically
allocate a second array to fit the data read exactly, and fill the new array
with the reverse of the original array entirely using pointer arithmetic. */

const int MAX_SIZE = 51;

#include <stdio.h>
#include <stdlib.h>

int getData( FILE * is, const int MAX_SIZE, int * );
int * allocateArray( int *, int numberRead );
void displayResults( FILE * os, int *, int *, int numberRead );

int main()
{
int arrayForward[MAX_SIZE];
int * arrayForPtr = 0;

FILE * fin;
fin = fopen( "pointer.txt", "r" );
if ( fin == NULL )
{
perror( "Error opening input file\n" );
// system( "pause" ); only for Dev C++
exit(1);
}

//get data
int numberRead = 0;
numberRead = getData( fin, MAX_SIZE, arrayForward );

fclose( fin );

int * arrayBackward = allocateArray( arrayForward, numberRead );

displayResults( stdout, arrayForward, arrayBackward, numberRead );

return 0;
}

/* This function takes the parameters: is (type file pointer), const int
* MAX_SIZE (51), and arrFor (pointer to an array of ints).
*/


int getData( FILE * is, const int MAX_SIZE, int * arrFor )
{
int nu = 0;
int used = 0;
int * arrayEnd = arrFor + (MAX_SIZE - 1);

while ( arrFor < arrayEnd && fscanf( is, "%i", arrFor ) == 1 )
{
++arrFor;
used++;
}

return used;
}

/* This function takes the parameters: arrFor (pointer to an array of ints),
* and numberRead (pointer to an int).
*/

int * allocateArray( int * arrFor, int numberRead )
{
int * arrBack;
arrBack = (int *)calloc( numberRead, sizeof(int) ); //allocate array
int * arrayEnd = arrFor + ( numberRead ); //find bounds

//fill new array with contents of first in reverse order

//using subscripts
/* int u = 0, count = *numberRead;
while ( u < *numberRead )
{
if ( count > 0 )
{
arrayBack[count - 1] = arrFor[u];
count--;
}
u++;
} */

//using pointer arithmetic
while ( arrFor < arrayEnd )
{
*(arrBack - 1) = *arrFor;
arrBack--;
arrFor++;
}

return arrBack;
}

/* This function takes the parameters: os (type file pointer), arrFor
* (pointer to original array of ints), arrBack (pointer to reversed
* array of ints), and numberRead (pointer to int).
*/

void displayResults( FILE * os, int * arrFor, int * arrBack, int numberRead )
{
int * arrayEndFor = arrFor + (numberRead); //bounds for original array
int * arrayEndBack = arrBack + (numberRead); //bounds for reversed array

fprintf( os, "%24s\n\n%14s%17s\n", "The Arrays", "Original", "Reverse" );

while ( arrFor < arrayEndFor && arrBack < arrayEndBack )
{
fprintf( os, "%14i%17i\n", *arrFor, *arrBack );
arrFor++, arrBack++;
}

free( arrBack ); //free dynamically allocated memory
}

chown33
Nov 27, 2010, 04:19 PM
chown33: I would be interested to hear what is weird.....I feel this revision has fixed some of that though.

They're mostly fixed.

You still have a bug here:
free( arrBack ); //free dynamically allocated memory

Look at what comes before that, and tell us whether it's freeing the pointer that was allocated, or is it freeing some other pointer value.


BTW, your extra code caused a segfault; I added it exactly as indicated. I spotted the issue without it anyway (no I hadn't seen willieva's post yet).

You'll have to post the complete compilable code that causes a segfault. It doesn't fail here, compiled for i386 or x86_64.

mac2x
Nov 27, 2010, 05:18 PM
They're mostly fixed.

You still have a bug here:
free( arrBack ); //free dynamically allocated memory

Look at what comes before that, and tell us whether it's freeing the pointer that was allocated, or is it freeing some other pointer value.

I was wondering about that. valgrind indicated that the #allocs != #frees. Let me think about how to solve this one. :)


You'll have to post the complete compilable code that causes a segfault. It doesn't fail here, compiled for i386 or x86_64.

I will worry about that another time, since I fixed most my own problems.

chown33
Nov 27, 2010, 05:51 PM
I was wondering about that. valgrind indicated that the #allocs != #frees. Let me think about how to solve this one. :)


The cause of the bug should be familiar.

gnasher729
Nov 27, 2010, 07:02 PM
void displayResults( FILE * os, int * arrFor, int * arrBack, int numberRead )
{
int * arrayEndFor = arrFor + (numberRead); //bounds for original array
int * arrayEndBack = arrBack + (numberRead); //bounds for reversed array

fprintf( os, "%24s\n\n%14s%17s\n", "The Arrays", "Original", "Reverse" );

while ( arrFor < arrayEndFor && arrBack < arrayEndBack )
{
fprintf( os, "%14i%17i\n", *arrFor, *arrBack );
arrFor++, arrBack++;
}

free( arrBack ); //free dynamically allocated memory
}

Just as an example, I would have written this function as

void displayResults (FILE* os, int arrForward [], int arrBackward [], int numberRead )
{
fprintf( os, "%24s\n\n%14s%17s\n", "The Arrays", "Original", "Reverse" );
for (int i = 0; i < numberRead; ++i)
fprintf (os, "%14i%17i\n", arrForward [i], arrBackward [i]);

free (arrBackward);
}

Lots shorter, lots more readable, and calls free () for the right pointer. Except that this function shouldn't call free (arrBack); I should be able to call a function called "displayResults" once, twice, or not at all, whatever I like, and I can't the way it is written because it calls free () for the "arrBack" array

mac2x
Nov 27, 2010, 09:48 PM
Thanks to valgrind, %p, and much manual work with the addresses, I think I finally arrived at a much improved program logically.

@gnasher: I would do that, except for the fact that I want to use only pointer arithmetic in this program. ;) Otherwise, I agree with you 100%. ;)

/* This program uses pointers to read into an array from a file, dynamically
allocate a second array to fit the data read exactly, and fill the new array
with the reverse of the original array entirely using pointer arithmetic. */

const int MAX_SIZE = 51;

#include <stdio.h>
#include <stdlib.h>

int getData( FILE * is, const int MAX_SIZE, int * );
int * allocateArray( int *, int numberRead );
void displayResults( FILE * os, int *, int *, int numberRead );

int main()
{
int arrayForward[MAX_SIZE];
int * arrayForPtr = 0;

FILE * fin;
fin = fopen( "pointer.txt", "r" );
if ( fin == NULL )
{
perror( "Error opening input file\n" );
// system( "pause" ); only for Dev C++
exit(1);
}

//get data
int numberRead = 0;
numberRead = getData( fin, MAX_SIZE, arrayForward );

fclose( fin );

int * arrayBackward = allocateArray( arrayForward, numberRead );

displayResults( stdout, arrayForward, arrayBackward, numberRead );

free( arrayBackward ); //free dynamically allocated memory

return 0;
}

/* This function takes the parameters: is (type file pointer), const int
* MAX_SIZE (51), and arrFor (pointer to an array of ints).
*/


int getData( FILE * is, const int MAX_SIZE, int * arrFor )
{
int nu = 0;
int used = 0;
int * arrayEnd = arrFor + (MAX_SIZE - 1);

while ( arrFor < arrayEnd && fscanf( is, "%i", arrFor ) == 1 )
{
++arrFor;
used++;
}

return used;
}

/* This function takes the parameters: arrFor (pointer to an array of ints),
* and int numberRead.
*/

int * allocateArray( int * arrFor, int numberRead )
{
int * arrBack;
arrBack = (int *)calloc( numberRead, sizeof(int) ); //allocate array
int * arrayEnd = arrFor + ( numberRead - 1 ); //find bounds
int * arrBackPtr = arrBack + (numberRead - 1 ); //bounds for reversed array

//fill new array with contents of first in reverse order
while ( arrFor <= arrayEnd )
{
*(arrBackPtr) = *arrFor;
arrFor++;
arrBackPtr--;
}

return arrBack;
}

/* This function takes the parameters: os (type file pointer), arrFor
* (pointer to original array of ints), arrBack (pointer to reversed
* array of ints), and int numberRead.
*/

void displayResults( FILE * os, int * arrFor, int * arrBack, int numberRead )
{
int * arrayEndFor = arrFor + (numberRead); //bounds for original array
int * arrayEndBack = arrBack + (numberRead); //bounds for reversed array

fprintf( os, "%24s\n\n%14s%17s\n", "The Arrays", "Original", "Reverse" );

while ( arrFor < arrayEndFor && arrBack < arrayEndBack )
{
fprintf( os, "%14i%17i\n", *arrFor, *arrBack );
arrFor++, arrBack++;
}

arrBack -= numberRead;
}

Sander
Nov 28, 2010, 07:32 AM
Except that this function shouldn't call free (arrBack); I should be able to call a function called "displayResults" once, twice, or not at all, whatever I like, and I can't the way it is written because it calls free () for the "arrBack" array

This, in bold-faced red flashing letters please.

lee1210
Nov 28, 2010, 08:05 AM
You don't need to "fix" the pointer in displayResults now, since you moved the free and it's passed in by value.

Also, if the purpose of this is pointer manipulation, why store a reversed copy at all? The logic in displayResults would be exactly the same except one pointer would walk backwards. Saves on storage and eliminates the pesky calloc/free.

-Lee