C program printing out junk; possible pointer issue

Discussion in 'Mac Programming' started by mac2x, Nov 26, 2010.

  1. mac2x, Nov 26, 2010
    Last edited: Nov 26, 2010

    macrumors 65816

    Joined:
    Sep 19, 2009
    #1
    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.

    Code:
    
    /* 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):

    Code:
    62
    55
    83
    90
    97
    104
    107
    148
    156
    160
    186
    190
    209
    217
    226
    232
    240
    283
    291
    295
    output (to stdout):

    Code:
     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
    
     
  2. thread starter macrumors 65816

    Joined:
    Sep 19, 2009
    #2
    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.
     
  3. macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #3
    Then post the latest so we aren't chasing specters.

    -Lee
     
  4. thread starter macrumors 65816

    Joined:
    Sep 19, 2009
    #4
    I did. When I said I fixed them, I meant exactly what I said. I fixed them, both here and in the source file. :)
     
  5. macrumors 603

    Joined:
    Aug 9, 2009
    #5
    Add the code hilited in red:
    Code:
    
    /* 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 );
    
    [COLOR="Red"]static void
    showArray( int * array, int len )
    {
    	printf( "--- len: %d ---\n", len );
    	while ( len-- > 0 )
    	{  printf( "%4d\n", *array++ );  }
    }
    [/COLOR]
    
    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 );
    	
    [COLOR="red"]	showArray( arrayForPtr, numberRead );
    [/COLOR]
    	allocateArray( arrayForPtr, &numberRead );
    	
    //	displayResults( stdout, arrayForward, arrayBackward, &numberRead );
    	
    [COLOR="red"]	showArray( arrayForPtr, numberRead );
    [/COLOR]
    	return 0;
    }
    
    Sample output:
    Code:
    --- 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:
    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.
     
  6. macrumors 6502

    Joined:
    Mar 12, 2010
    #6
    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.
     
  7. SidBala, Nov 27, 2010
    Last edited: Nov 27, 2010

    macrumors 6502a

    Joined:
    Jun 27, 2010
    #7
    You are decrementing off arrBack in allocate Array.

    Code:
    	//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.
     
  8. SidBala, Nov 27, 2010
    Last edited: Nov 27, 2010

    macrumors 6502a

    Joined:
    Jun 27, 2010
    #8
    Also in allocarray:

    Code:
            //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:

    Code:
    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);
     
  9. thread starter macrumors 65816

    Joined:
    Sep 19, 2009
    #9
    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).

    Code:
    /* 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
    }
     
  10. macrumors 603

    Joined:
    Aug 9, 2009
    #10
    They're mostly fixed.

    You still have a bug here:
    Code:
    	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.


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

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


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

    Joined:
    Aug 9, 2009
    #12
    The cause of the bug should be familiar.
     
  13. macrumors G5

    gnasher729

    Joined:
    Nov 25, 2005
    #13
    Just as an example, I would have written this function as

    Code:
    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
     
  14. thread starter macrumors 65816

    Joined:
    Sep 19, 2009
    #14
    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%. ;)

    Code:
    /* 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;
    }
     
  15. macrumors 6502

    Joined:
    Apr 24, 2008
    #15
    This, in bold-faced red flashing letters please.
     
  16. macrumors 68040

    lee1210

    Joined:
    Jan 10, 2005
    Location:
    Dallas, TX
    #16
    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
     

Share This Page