function returns address of local variable (void)

rubadub · 10-27-2007, 04:29 PM

This function works but I can't seem to get to be able to get rid of the warning:

Code:

void * read_element_seq(void *type, int num, FILE *fp)
{
	void *v;
	if (fread(&v, sizeof(type), 1, fp) != 1)
	{
		if (feof(fp))
		{
			printf("Error: Premature end of file found whilst reading input file! \n");
		}
		else
		{
			printf("Error: Unable to read from input file! \n");
		}
	}
	return &v;
}

Here's the warning:

Quote:

warning: function returns address of local variable

I've tried malloc and realloc, but they don't like being sized from a void. Any suggestions / solutions greatly appreciated...

matthewg42 · 10-27-2007, 05:12 PM

The warning is because the scope of the variable is local to the function - once the function is returned, that variable is no longer in scope, and it's value is undefined.

For functions which do this sort of thing, is it typical that they accept a pointer to a variable in the calling function which is then modified by the function.

Another way to do it (although usually not such a good idea) is to have a global variable.

ntubski · 10-27-2007, 07:29 PM

Quote:

Originally Posted by rubadub

This function works but I can't seem to get to be able to get rid of the warning:

I wonder what you mean when say this function "works". It seems you want to able to read in any type of variable, but maybe you don't realize that sizeof(type) will always be 4 on a 32-bit machine (8 on 64-bit) because type has type void*. Since fread returns total bytes read and you are asking it to read 4 bytes, I'd be surprised if fread ever returns 1 (should only happen on the last byte of the file). Furthermore, as matthewg42 said, the value of a pointer to variable that has gone out of scope is undefined, so it is only luck that is stopping your program from crashing every time you use this function.

I can't see why you return &v, you could just return v, which is the only interesting value and is the same size as &v.

osor · 10-27-2007, 10:15 PM

@ntubski: read() returns the number of bytes read. fread() returns the number of elements read.

@rubadub: When you return the address of an automatic-storage variable (one that’s on the stack) what you get will most likely be clobbered by later calls to any other functions.

If you do happen to change the function so that it returns v (the value) not &v (the address on the stack) you would get something like this:

Code:

intptr_t read_element_seq(int dummy_1, int dummy_2, FILE *fp)
{
	intptr_t v;
	if (fread(&v, sizeof(intptr_t), 1, fp) != 1)
	{
		if (feof(fp))
		{
			printf("Error: Premature end of file found whilst reading input file! \n");
		}
		else
		{
			printf("Error: Unable to read from input file! \n");
		}
	}
	return v;
}

Where intptr_t is a typedef for long on almost all architectures. Since the first two parameters are unused, I gave them more meaningful names.

Basically, the function will read one long’s worth of binary data from a file pointer and return that data’s representation (as a long). If that’s what you intended, then yes—it works. Additionally, depending on how you use this function, it may not be portable since long has different representations (i.e., size and endianness) on different architectures (e.g., if you are reading from some sort of binary (i.e., non-textual) configuration file, such files may not be swappable among all machines). You might be better off reading one byte at a time or reading to a suitably defined struct.

atulsvasu · 10-28-2007, 12:25 AM

Try this:

Code:

void * read_element_seq(void *type, int num, FILE *fp)
{
	void *v = malloc (sizeof(type)*num);

	if (fread(v, sizeof(type), num, fp) != 1)
	{
		if (feof(fp))
		{
			printf("Error: Premature end of file found whilst reading input file! \n");
		}
		else
		{
			printf("Error: Unable to read from input file! \n");
		}
	}
	return v;
}

I hope you understand what was wrong. Don't forget to delete
the chunk returned.

It is better to refactor your code:

Code:

void* read_element_seq(size_t type_size, int num, FILE* fp) {
 // and replace all sizeof(type) by type_size.
}

// And where you call it, like this:

Code:

  int A[100];
  void *v = read_element_seq(sizeof(int), 100, fp);
  
  memcpy(A, v, sizeof(int)*100)
}

Even better refactoring would be to return null if fread returns
-1. If v=null, then don't do memcpy. You should also check
if malloc was a success.

atulsvasu · 10-28-2007, 12:30 AM

Another suggestion is to remove that helper completely, if your code is
not really that big.

instead, for

Code:

 int A[100];
 if (fread (A, sizeof(int), 100, fp)) < 100)
   reportError(fp);

Is better.

Code:

void reportError(FILE* fp) {
  if (feof(fp)) {
     ///
  } else {
     ///
  } 
}

rubadub · 10-28-2007, 06:28 AM

Good morning! Thanks for all the replies...

Understandably the scope of the variables' life is only for the function, yet at this stage I was just printing the result before anything else was happening, so assumption on my behalf was it should still be present. I'd like to say that i'd copy it next, however, the original intent was to pass a pointer as 'type' and use as the return, but then I used it to size the read (because it is initialised as appropriate variable type, (you may ask, why then is it still a pointer when you return with 'v', so do I)).

I like the 'atulsvasu' way of passing the size, however it isn't my original intent (the standard return was for a success/failure result). I am going to do it that way, then start over and do the original way, then develop to handle array's.

p.s. I'd like to say i'm an ok programmer, yet the whole pointer thing still gets me with c, so please bear with my atrocious contraptions, i'm getting there (as the Beatles said, with a little help...). Have fun an' cheers!

To be continued...

matthewg42 · 10-28-2007, 06:57 AM

Quote:

Originally Posted by rubadub

Good morning! Thanks for all the replies...

Understandably the scope of the variables' life is only for the function, yet at this stage I was just printing the result before anything else was happening, so assumption on my behalf was it should still be present.

The assumption is incorrect in many cases, and moreover it is dangerous. You should never use a pointer to a variable which has gone stale like this. You're asking for segfaults and or your hard disk being formatted.

rubadub · 10-28-2007, 07:37 AM

It's true, but I was having trouble with allocating the void's size, so I was taking a short cut... until...

Sorry for the size of this, but it's weird (to me anyway!):

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>

int test_write(char * fname, int ver)
{
	FILE *fp;
	float f = 12.34;
	int i = 123;
	char c = 'a';

	if((fp=fopen(fname, "wb"))==NULL)
	{	return -1;	}
	
	if(ver == 1)
	{
	}
	else
	{
		f = 12.34f; fwrite(&f, sizeof(float), 1, fp);
		f = 3.21f; fwrite(&f, sizeof(float), 1, fp);
		f = 5.55f; fwrite(&f, sizeof(float), 1, fp);
		f = 678.90f; fwrite(&f, sizeof(float), 1, fp);
		
		c = 'a'; fwrite(&c, sizeof(char), 1, fp);
		c = 'b'; fwrite(&c, sizeof(char), 1, fp);
		c = 'c'; fwrite(&c, sizeof(char), 1, fp);
		c = 'd'; fwrite(&c, sizeof(char), 1, fp);
		
		i = 111; fwrite(&i, sizeof(int), 1, fp);
		i = 222; fwrite(&i, sizeof(int), 1, fp);
		i = 333; fwrite(&i, sizeof(int), 1, fp);
		i = 444; fwrite(&i, sizeof(int), 1, fp);
		
	}
	fclose(fp);
	
	return 0;
}

void * read_element_seq(void * size, int num, FILE *fp)
{
	void *v = malloc (sizeof(size)*num);
	if (fread(v, sizeof(size), num, fp) != num)
	{
		if (feof(fp))
		{	printf("Error: Premature end of file found whilst reading input file! \n");	}
		else
		{	printf("Error: Unable to read from input file! \n");	}
	}
	return v;
}
int main(int argc, char *argv[])
{
	int iret, i;
	char * fname = "test.dat";
	
	/*	CREATE TEST DATA	*/
	iret = test_write(fname, 0);	/*char * fname, int ver*/
	
	/*	OPEN SUBJECT FILE	*/
	FILE *fp;
	if ((fp=fopen(fname, "rb"))==NULL)
	{
		printf("Error: Unable to open input file!!\n");
	}
	
	int size = 0;
	int *psize = &size;
	
	int num, j;
	
	//	FIRST SECTION
	num = 4;
	size = sizeof(float);
	float * ff = read_element_seq(psize, num, fp);
	for(j=0;j<num;j++, ff++)
	{
		printf("	: %f \n", *ff);
	}
	
	//	SECOND SECTION
	num = 4;
	size = sizeof(char);
	char * cc = read_element_seq(psize, num, fp);
	for(j=0;j<num;j++, cc++)
	{
		printf("	: %c \n", *cc);
	}
	
	//	THIRD SECTION
	num = 4;
	size = sizeof(int);
	int * ii = read_element_seq(psize, num, fp);
	for(j=0;j<num;j++, ii++)
	{
		printf("	: %d \n", *ii);
	}
	
	fclose(fp);
	return 0;
}

Here's my output:

Quote:

: 12.340000
: 3.210000
: 5.550000
: 678.900024
: a
: b
: c
: d
Error: Premature end of file found whilst reading input file!
: 444
: 0
: 0
: 0

1. Why am I getting the premature ending of file?
2. Why are the int's coming out wrong. If I swap the place of the int's with the place of the char's then this point is ok, why oh why?

matthewg42 · 10-28-2007, 08:08 AM

If you persist in writing illegal code which uses out-of-scope variables even when told that this will cause problems... it is your own business. I see no point in offering further advice, as it will likely be ignored too.

rubadub · 10-28-2007, 12:46 PM

I seem to have an antitrust in some things mr gates, so i've rehashed my way and here's the result.

Code:

int read_element_seq(void * ret, int size, int num, FILE *fp)
{
	if (fread(ret, size, num, fp) != num)
	{
		if (feof(fp))
		{
			printf("Error: Premature end of file found whilst reading input file! \n");
		}
		else
		{
			printf("Error: Unable to read from input file! \n");
		}
	}
	return 0;
}

I'm totally self taught working from books and forums, I do appreciate your time, comments and the community, this seems to work, anyways...

osor · 10-28-2007, 12:59 PM

Quote:

Originally Posted by matthewg42

If you persist in writing illegal code which uses out-of-scope variables even when told that this will cause problems... it is your own business.

Wait… are you talking about post 9? I think you should take a closer look.

Quote:

Originally Posted by rubadub

1. Why am I getting the premature ending of file?
2. Why are the int's coming out wrong. If I swap the place of the int's with the place of the char's then this point is ok, why oh why?

First off, when you increment your pointer like that, you seem to ignore any chance of its freedom (I will forgive since this is such a short example

. Aside from that, you missed what many of us said a few posts back—sizeof(size) will evaluate to the same number regardless if you passed a pointer to a float, char, int, or whatever. sizeof is a compile-time operator, it will return the machine’s pointer size.

All is not lost, however. Try something much simpler, like this:

Code:

#include <stdio.h>
#include <stdlib.h>
#include <string.h>
#include <sys/stat.h>

int test_write(char * fname, int ver)
{
	FILE *fp;
	float f = 12.34;
	int i = 123;
	char c = 'a';

	if((fp=fopen(fname, "wb"))==NULL)
	{	return -1;	}
	
	if(ver != 1)
	{
		f = 12.34f; fwrite(&f, sizeof(float), 1, fp);
		f = 3.21f; fwrite(&f, sizeof(float), 1, fp);
		f = 5.55f; fwrite(&f, sizeof(float), 1, fp);
		f = 678.90f; fwrite(&f, sizeof(float), 1, fp);
		
		c = 'a'; fwrite(&c, sizeof(char), 1, fp);
		c = 'b'; fwrite(&c, sizeof(char), 1, fp);
		c = 'c'; fwrite(&c, sizeof(char), 1, fp);
		c = 'd'; fwrite(&c, sizeof(char), 1, fp);
		
		i = 111; fwrite(&i, sizeof(int), 1, fp);
		i = 222; fwrite(&i, sizeof(int), 1, fp);
		i = 333; fwrite(&i, sizeof(int), 1, fp);
		i = 444; fwrite(&i, sizeof(int), 1, fp);
		
	}
	fclose(fp);
	
	return 0;
}

void * read_element_seq(size_t size, size_t num, FILE *fp)
{
	void *v = malloc (size * num);
	if (fread(v, size, num, fp) != num)
	{
		if (feof(fp))
		{	printf("Error: Premature end of file found whilst reading input file! \n");	}
		else
		{	printf("Error: Unable to read from input file! \n");	}
	}
	return v;
}

int main(int argc, char *argv[])
{
	int iret, i;
	char * fname = "test.dat";
	
	/*	CREATE TEST DATA	*/
	iret = test_write(fname, 0);	/*char * fname, int ver*/
	
	/*	OPEN SUBJECT FILE	*/
	FILE *fp;
	if ((fp=fopen(fname, "rb"))==NULL)
	{
		printf("Error: Unable to open input file!!\n");
	}
	
	int j;

	size_t num;
	size_t size = 0;

	//	FIRST SECTION
	num = 4;
	size = sizeof(float);
	float * ff = read_element_seq(size, num, fp);
	for(j=0;j<num;j++, ff++)
	{
		printf("	: %f \n", *ff);
	}
	
	//	SECOND SECTION
	num = 4;
	size = sizeof(char);
	char * cc = read_element_seq(size, num, fp);
	for(j=0;j<num;j++, cc++)
	{
		printf("	: %c \n", *cc);
	}
	
	//	THIRD SECTION
	num = 4;
	size = sizeof(int);
	int * ii = read_element_seq(size, num, fp);
	for(j=0;j<num;j++, ii++)
	{
		printf("	: %d \n", *ii);
	}
	
	fclose(fp);
	return 0;
}

Notice that I completely eliminated psize. I also pass a size to your function. This makes it much easier.

As a final note, with production code, always check to see if your call to malloc() succeeded, and always free() those pointers when your done with them. Additionally, don’t expect to be able to give your test.dat to someone with a different machine architecture.