3

I have this code:

 1 #include <stdio.h>                                                               
 2 #include <string.h>                                                              
 3 #define LENGTH(a) sizeof(a)/sizeof(a[0]);                                        
 4                                                                                  
 5 int *getNbits(int, int, char *);                                                 
 6                                                                                  
 7 int main() {                                                                     
 8    char ins[] = "00001111000011110000111100001111";                              
 9    int *x = getNbits(0, 32, ins);                                                
 10                                                                                  
 11    for (int i = 0; i < LENGTH(x) ; i++){                                                
 12                                                                                  
 13       printf("%d", *(x + i));                                                        
 14    }                                                                             
 15    return 0;                                                                     
 16 }                                                                                
 17                                                                                  
 18 int* getNbits(int start, int offset, char instr[33]) {                           
 19                                                                                  
 20    int result[offset - 1];                                                       
 21    for (int i = 0; i < offset; i++) {                                            
 22        result[i] = instr[i + start] == '1' ? 1 : 0;   //- '0';                   
 23    }                                                                             
 24    return result;                                                                
 25 }

Basically, getNbits() gets an array of chars (which are 0's or 1's) and returns an array of ints, each element is a 0 or a 1.

If I try to print the array "result" as I am creating it in the for loop (using printf("%d",result[i]) ) it will return 000011110000.....

But I am having trouble with the return type, it gives me a warning: function returns address of local variable. And the for loop in main just prints garbage values.

Improve this question
6
  • int result in your getNbits function will exist only for the duration of the the getNbits call. It's an array allocated on the stack. When the function returns, that stack space is released and will no longer be usable by the calling code. You MAY luck out and still have that part of the stack be untouched, but it's a race condition and at some point that stack area will be overriten by other code Commented May 26, 2014 at 17:46
  • #define LENGTH(a) sizeof(a)/sizeof(a[0]); --> #define LENGTH(a) sizeof(a)/sizeof(a[0]) Commented May 26, 2014 at 17:47
  • 1
    @MarcB: "You MAY luck out and still have that part of the stack be untouched" - That would be decidedly unlucky Commented May 26, 2014 at 17:48
  • Should I then use malloc() to get the array in the heap to be able to retrieve the array after the method call? Commented May 26, 2014 at 17:55
  • 2
    no need to use malloc, you can declare a local array from the calling function and pass it to the called Commented May 26, 2014 at 18:21

3 Answers 3

Reset to default
3

The result variable is local to getNbits. This means it can be deallocated as soon as the function returns.

Don't return the address of a LOCAL variable as a pointer - it can and will be deallocated.

Instead, allocate the integers like this:

int* retVal =  malloc(sizeof(int) * 10);

Then return retVal from getNbits.

See this URL:

http://ww2.cs.mu.oz.au/~mgiuca/253/malloc.html

NOTE: If you use malloc above, you must also use free to release the allocated memory after it is no longer needed:

/* When the array is no longer needed */ 
free(x);

At the point of the free, the variable name is x because that was the lvalue assigned from the call to getNbits. Don't call free from getNbits.

Improve this answer
Sign up to request clarification or add additional context in comments.

6 Comments

Thanks a lot! I found the information about malloc very useful. It even uses something like my code as an explicit example of bad coding
No problem; let me know if there is anything else I can do.
Casting the result of malloc is bad practice (see stackoverflow.com/questions/605845/…), you should remove that from your answer.
Updated to remove the typecast; malloc will return a pointer to void, so the typecast will be implied..
That is true; I used malloc because in the code in this question, the definition of getNBits assumed the return value was a newly allocated array.
|
2 
int* getNbits(int start, int offset, char instr[33]) {                           
   int result[offset - 1];                                                       
   for (int i = 0; i < offset; i++) {                                            
      result[i] = instr[i + start] == '1' ? 1 : 0;   //- '0';                   
   }                                                                             
   return result;     
}

The int array result is declaired (by default) on the stack. This causes it to have a scope (lifespan) limited to inside the getNbits() function. Passing a reference to this array outside the function is not a good idea; as the stack space where the array formally resided will be used for another purpose.

The same operation might be better accomplished by allocating the array on the heap:

int* getNbits(int start, int offset, char instr[33]) {                           
   int *result = malloc(offset * sizeof(*result));  // As indicated by BLUEPIXY's comment
   if(NULL == result)
      {
      fprintf(stderr, "malloc() failed\n");
      goto CLEANUP;
      }

   for (int i = 0; i < offset; i++) {                                            
      result[i] = instr[i + start] == '1' ? 1 : 0;   //- '0';                   
   }    

CLEANUP:                                                                         
   return result;     
}

Of course, in the above case, the caller of getNbits() will have to remember to call free() on the returned array when it is no longer needed to return the allocated memory back to the heap.

Improve this answer

Comments

0

It's simpler and less error prone, to allocate the array in main, where you call the results array.

int result[ sizeof ins];    /* Size known at compile time */

getNbits(0, 32, ins, result);  /* Pass arrays by reference */

void getNbits(int start, int offset, const char instr[], int result[]) {

That's using automatic storage on stack which is automatically recycled when a routine returns. As the code in questions stands, printf's variables allocated on the stack, will overwrite the values pointed to by result.

An advantage is generalising the getNbits subroutine, so it would work with longer length versions of "ins". Of course a real program, would have an error status returned by getNbits, to catch errors.

Not good practice to malloc, memory in a function, and have the caller responsible for freeing, it's likely to leak or be freed later multiple times, with more complicated programs and these are often hard to track down bugs.

Traditional code often defined structs or arrays, passed from functions like int result[] as static storage, so the address was not on the stack. That worked, but is not thread safe, and any values stored in data structures had to be copied anyway.

If you do write a library functions that dynamically constructs a data structure, then pair it with a routine to tidy up, rather than leak implementation details into the callers program!

Improve this answer

Comments

Start asking to get answers

Find the answer to your question by asking.

Ask question

Explore related questions

See similar questions with these tags.