help with type-punned warning please

D

David Ford

I have a macro that I use across the board for freeing ram. I'd like to
clean up my code so I don't get these warnings.

#define sfree(x) _internal_sfree((void **)&x)
#define _internal_sfree(x) ({ if(x && *x) { free(*x); *x=NULL; } })

void somefunction() {

char *x = malloc(10);
int *y = malloc(10);

sfree(x);
sfree(y);
}

results in:

warning: dereferencing type-punned pointer will break strict-aliasing
rules

What's the best way of fixing my lack of clue?

Thank you,
David
 
M

Martin Ambuhl

David said:
I have a macro that I use across the board for freeing ram. I'd like to
clean up my code so I don't get these warnings.

#define sfree(x) _internal_sfree((void **)&x)
#define _internal_sfree(x) ({ if(x && *x) { free(*x); *x=NULL; } })

void somefunction() {

char *x = malloc(10);
int *y = malloc(10);

sfree(x);
sfree(y);
}

results in:

warning: dereferencing type-punned pointer will break strict-aliasing
rules

What's the best way of fixing my lack of clue?

That's the least of your problems, it seems to me. Your code expands to

void somefunction()
{

char *x = malloc(10);
int *y = malloc(10);

( {
if ((void **) &x && *(void **) &x) {
free(*(void **) &x); *(void **) &x = 0;}}) ;
( {
if ((void **) &y && *(void **) &y) {
free(*(void **) &y); *(void **) &y = 0;}}) ;
}


which has two illegal instances of braced groups within expressions.
 
D

David Ford

That's the least of your problems, it seems to me. Your code expands to

void somefunction()
{

char *x = malloc(10);
int *y = malloc(10);

( {
if ((void **) &x && *(void **) &x) {
free(*(void **) &x); *(void **) &x = 0;}}) ;
( {
if ((void **) &y && *(void **) &y) {
free(*(void **) &y); *(void **) &y = 0;}}) ;
}


which has two illegal instances of braced groups within expressions.

This is what I get, and pardon, but how is this illegal? I've been using
this for years, it works fine. It's just with the newer version of gcc
that I'm getting this warning.


({
if ( (void **)&x && *(void **)&x ) {
free(*(void **)&x);
*(void **)&x = ((void *)0);
}
});


I'm not arguing, I just haven't had the time or opportunity to keep up to
date.

Thanx,
David
 
D

David Ford

I have a macro that I use across the board for freeing ram. I'd like to
clean up my code so I don't get these warnings.

#define sfree(x) _internal_sfree((void **)&x)
#define _internal_sfree(x) ({ if(x && *x) { free(*x); *x=NULL; } })

void somefunction() {

char *x = malloc(10);
int *y = malloc(10);

sfree(x);
sfree(y);
}

results in:

warning: dereferencing type-punned pointer will break strict-aliasing
rules

What's the best way of fixing my lack of clue?

Thank you,
David

Just for the curious, I've since changed my sfree() macro to the following:

#define sfree(x) \
({ \
__typeof(&x) T = (&x); \
\
if ( T && *T ) { \
free (*T); \
*T = NULL; \
} \
})
 
V

Vijay Kumar R Zanvar

David Ford said:
I have a macro that I use across the board for freeing ram. I'd like to
clean up my code so I don't get these warnings.

#define sfree(x) _internal_sfree((void **)&x)
#define _internal_sfree(x) ({ if(x && *x) { free(*x); *x=NULL; } })

void somefunction() {

char *x = malloc(10);
int *y = malloc(10);

sfree(x);
sfree(y);
}

results in:

warning: dereferencing type-punned pointer will break strict-aliasing
rules

What's the best way of fixing my lack of clue?

Thank you,
David

This should fix the warnings:

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

#define sfree(x) _internal_sfree((void **)&x)
#define _internal_sfree(x) do { \
if ( (x) && *(x) ) \
{ \
free(*(x)); \
*(x)=NULL; \
} \
}while (0)

void somefunction()
{
char *x = malloc(10);
int *y = malloc(10);

sfree(x);
sfree(y);
}
 
V

Vijay Kumar R Zanvar

David Ford said:
Just for the curious, I've since changed my sfree() macro to the following:

#define sfree(x) \
({ \
__typeof(&x) T = (&x); \
\
if ( T && *T ) { \
free (*T); \
*T = NULL; \
} \
})

ISO C forbids braced-groups within expressions
 
C

Chris Torek

I have a macro that I use across the board for freeing ram.

Both your original macro and your new replacement version depend
on gcc-specific features. (Also, many names beginning with underscore
are reserved to the implementation, so you should use them if and
only if you are the implementor, i.e., the guy supplying the
compiler. Some names are given over to users -- and a few are even
required, like _IOFBF -- but it is a lot easier to remember "avoid
leading underscore" than it is to remember "avoid leading underscore
except when used in limited scopes and followed by lowercase letters
or digits" and so on.)

You can obtain portable, warning-free code by expanding your
macro "by hand" as it were. Since free(NULL) is always allowed
(and is a no-op), and since &x is non-NULL for any valid
variable x, just replace the sfree() in:
char *x = malloc(10); ....
sfree(x);

with:

free(x), x = NULL;

As for the warning:
dereferencing type-punned pointer will break strict-aliasing rules

gcc is helpfully telling you that your code will actually fail on
real, existing implementations (including, in some cases, some
using gcc). Suppose x has, for instance, type "int *". Then:
#define sfree(x) _internal_sfree((void **)&x)
#define _internal_sfree(x) ({ if(x && *x) { free(*x); *x=NULL; } })

"means", on the Data General Eclipse series machine, "take the
address of x" -- which has type "int **" and is thus a word pointer
-- "and convert it to a new, different value of type void **".
Luckily "void **" is also still a word pointer, but now _internal_sfree
will read through that "void **" to find a "void *", which is not
a word pointer at all, but rather a byte pointer. Thus, the word
pointer stored in x will be compared against "byte-pointer NULL"[%];
if it is not equal, the actual 32-bit word pointer value will be
passed to free() as if it were a 32-bit byte pointer. This will
omit the crucial "shift right one bit" instruction that would be
generated for the call:

free(x)

and free() will crash. (The program will not proceed any further
at this point, so the "*x = NULL" -- using _sfree_internal's x --
never even happens.)

Ultimately, the problem boils down to the fact that, inside
_internal_sfree, the result of following the "void **" pointer is
assumed to be a value of type "void *", but invocations of the
sfree() macro are given a pointer of type "T *" where T is *not*
"void". This gives _internal_free pointers of type "T **" where
T is still not "void"; the forced conversion (via cast) to
"void **" papers over the type difference, but leaves the
underlying problem.

The tricky variant using gcc's "typeof" extension will work, but
why use a nonportable construct when the portable version above is
really just as simple and is required to work on any Standard C
system? Admittedly, invocations like:

sfree(*p++);

have to be expanded somewhat carefully:

free(*p), *p++ = NULL;

but the result is still a single expression (in case this is important).

[%footnote: As it happens, on the Eclipse, NULL is all-bits-zero
for both byte and word pointers. Valid pointers have an interesting
format, with ring and segment numbers in upper bits. There is an
unused-in-C high order bit in word pointers, and a low-order "byte
number" bit in byte pointers instead. To convert a byte pointer
to a word pointer, you shift left one bit, setting the byte number
to 0; to convert a word pointer to a byte pointer, shift right one
bit, discarding the byte number. The compiler generates these
shifts for you whenever it converts "void *" to "int *" and vice
versa.]
 
D

Dan Pop

In said:
This is what I get, and pardon, but how is this illegal? I've been using
this for years, it works fine. It's just with the newer version of gcc
that I'm getting this warning.


({
if ( (void **)&x && *(void **)&x ) {
free(*(void **)&x);
*(void **)&x = ((void *)0);
}
});


I'm not arguing, I just haven't had the time or opportunity to keep up to
date.

Have you *ever* read a C book? The language has NEVER accepted blocks
inside expressions. This is a GNU C extension.

And it is downright ludicrous to write such a monster, when what you
want to do can be achieved with:

#define sfree(p) (free(p), (p) = NULL)

Dan
 
A

Arthur J. O'Dwyer

I have a macro that I use across the board for freeing ram. I'd like to
clean up my code so I don't get these warnings.

#define sfree(x) _internal_sfree((void **)&x)
#define _internal_sfree(x) ({ if(x && *x) { free(*x); *x=NULL; } })

AFAICT, nobody has pointed out yet that all you need to do to
get rid of that "stupid cast" warning is... well... get rid of the
stupid cast! (Below, I've also disposed of the bizarre ({}) syntax.
It was silly.)

#define sfree(x) _internal_sfree(&x)
#define _internal_sfree(x) ((x && *x)? (free(*x), (*x)=NULL): NULL)

Of course, this is still not a good way to free a pointer. A
better way would be

#define sfree(x) free(x)

or possibly even

#define sfree(x) (free(x), x=NULL)

if you don't trust yourself yet with memory management. But
I've never found the NULL-assignment useful myself, given that
it never has any effect on a program unless you mess up somewhere
else; and usually IMLE mess-ups come in groups. (It's a style
thing.)

-Arthur
 
E

E. Robert Tisdale

David said:
I have a macro that I use across the board for freeing ram.
> cat somefunction.c
#include <stdlib.h>

#define sfree(x) _internal_sfree((void **)&x)
#define _internal_sfree(x) \
({ if(x && *x) { free(*x); *x = NULL; } })

void somefunction(void) {

char* x = (char*)malloc(10);
int* y = (int*)malloc(10);

sfree(x);
sfree(y);
}
> gcc -c somefunction.c
> gcc -Wall -std=c99 -pedantic -c somefunction.c
somefunction.c: In function `somefunction':
somefunction.c:12: warning: \
ISO C forbids braced-groups within expressions
somefunction.c:13: warning: \
ISO C forbids braced-groups within expressions
> gcc --version
gcc (GCC) 3.2 20020903 (Red Hat Linux 8.0 3.2-7)
 
D

David Ford

AFAICT, nobody has pointed out yet that all you need to do to
get rid of that "stupid cast" warning is... well... get rid of the
stupid cast! (Below, I've also disposed of the bizarre ({}) syntax.
It was silly.)

#define sfree(x) _internal_sfree(&x)
#define _internal_sfree(x) ((x && *x)? (free(*x), (*x)=NULL): NULL)

Of course, this is still not a good way to free a pointer. A
better way would be

#define sfree(x) free(x)

or possibly even

#define sfree(x) (free(x), x=NULL)

if you don't trust yourself yet with memory management. But
I've never found the NULL-assignment useful myself, given that
it never has any effect on a program unless you mess up somewhere
else; and usually IMLE mess-ups come in groups. (It's a style
thing.)

Thanks.

Actually the reason why I use sfree() instead of just free(), is because
every now and then I've muggled up a pointer somewhere unintentionally. I
use valgrind amongst others and in a large program, a lost bitty pointer
can be a bit troublesome to track down. I see *p = 0x897ffaed and use it,
not realizing that somewhere I made a change in one of my routines quite a
ways back in the code path and *p is actually free()'d. So by habit now
(since I'm quite far from perfect), I set my pointers to 0x0.

Trust me, I'm often forgetful ;) Problem is I often have rather complex
linked lists of structures within linked lists etc and since a lot of my
code happens to be used over a long period of time, I try hard to ensure
my memory use is limited and doesn't leak. So in the end it makes for a
lot of little bits of frustration due to my own faults.

Thank you for the commentary. On a side note, the ({ }) is just another
habit of mine.

David
 
D

David Ford

Have you *ever* read a C book? The language has NEVER accepted blocks
inside expressions. This is a GNU C extension.

And it is downright ludicrous to write such a monster, when what you
want to do can be achieved with:

#define sfree(p) (free(p), (p) = NULL)

Dan

Yes, actually, I have, but this was back in the 80s and early 90s. I
didn't say I was trying to be ISO C compliant. I was asking about the
type-punned warning. I've learned several things in this thread, including
the fact that I was writing a monster when I didn't need to. That's all
good and well, but don't deride me on a tangent question particularly when
others have already explained and provided the answer.

David
 
T

The Real OS/2 Guy

Thanks.

Actually the reason why I use sfree() instead of just free(), is because
every now and then I've muggled up a pointer somewhere unintentionally. I
use valgrind amongst others and in a large program, a lost bitty pointer
can be a bit troublesome to track down. I see *p = 0x897ffaed and use it,
not realizing that somewhere I made a change in one of my routines quite a
ways back in the code path and *p is actually free()'d. So by habit now
(since I'm quite far from perfect), I set my pointers to 0x0.

Trust me, I'm often forgetful ;) Problem is I often have rather complex
linked lists of structures within linked lists etc and since a lot of my
code happens to be used over a long period of time, I try hard to ensure
my memory use is limited and doesn't leak. So in the end it makes for a
lot of little bits of frustration due to my own faults.

Thank you for the commentary. On a side note, the ({ }) is just another
habit of mine.

Not only yours. It is proven an common praxis to set each pointer to
NULL whenever it losts its content. Free() a pointer and do nothing
when the pointer reaches itself its lifetime shortenly thereafter.
Free() the pointer and set it to NULL whenever the data it points to
gets unused but the pointer itself may be reused somewhere in the
code.

But with that void** make really no sense as it can never usede to be
dereferenced. casting from and to void is always nonsense as the
compiler will always convert any pointer to or from a data object from
and to void implicity - and each cast is a shock for an human reader.

Avoiding casts in any way is the best usage of casting one can ever
do. Mostenly when you things you needs a cast you may closely fallen
into a design flaw. Recheck the design of your data structures before
you starts casting.

I#ve learned since my very first days of programming in C that
documenting and use unsignded char in default (by telling the compiler
that any char is unsigned - except it gets explicity defined as signed
- is the best default one can use as it saves traps one falls easy in.
I've never seen a compiler that has not a switch one can set (e.g. in
makefile) to tell it that char is always unsigned.

Signed char makes rarely sense - and whenever it must be signed then
one can ever define it explicity as signed.
 
T

The Real OS/2 Guy

#include <stdlib.h>

Twitstale ignores the standard constantly!
#define sfree(x) _internal_sfree((void **)&x)
#define _internal_sfree(x) \
({ if(x && *x) { free(*x); *x = NULL; } })

Names starting with underscore are reserved for the environment.
Defining a single name so make the whole program incompatible - even
to the current used compiler.

Twitstale why does you NOT use ***************&&&&&&&&&&&&&&&&&x
instead of the cryptic rubbish? Making things unreadable is nothing
one does when one is not a twit as you. void ** is completely useless
too. Unnedded casting is writing twit code.


void somefunction(void) {

char* x = (char*)malloc(10);

Wrong! Use

((void*)((int*) (char**)) = (char*) (((char ****)(int*) (float**=))
double***) (unsingned long long) malloc(sizeof(char) * ((unsigned long
long) 345) / ((float 1.0 - ((char) 1)) + 0 / 345.0)) * (1000 / 10))

makes more sense in obfucasing the code.

int* y = (int*)malloc(10);

Wrong! You means
int* y = (int *)(char ****)(float***)(double**) (unsigned
short*)malloc(sizeof(signed char) * (unsigned short) 1000 / (unsigned
char) (0.1 * 10) / 10);

as you tries to obfuscate you should try a bit harder!
sfree(x);

Wrong again! You means

sfree(x + 350 - 175 * 2);


sfree(y);

same as above
}

somefunction.c: In function `somefunction':
somefunction.c:12: warning: \
ISO C forbids braced-groups within expressions
somefunction.c:13: warning: \
ISO C forbids braced-groups within expressions
gcc (GCC) 3.2 20020903 (Red Hat Linux 8.0 3.2-7)
shows only that twitstale is too dumb to write a simple obfusated C
program right.

#include <stdlib.h>
/* for security each pointer needs to be a NULL pointer when it points
to nothing */
#define free(x) free(x), x = NULL; /* when programmer fails to give a
pointer to an object received from malloc/calloc or a pointer that is
already a NULL pointer let the program fail! Makes sense to get a
crash before the program goes in production anyway. */


int *p2 = malloc(4712);
char *p1 = malloc(4711);

free(p2);
free(p1)

makes the same without casting, obfucasing, but 100% compatible to
ANSI C.

There is no need to test a pointer for NULL when free() must be called
because free() does simple nothing with a NULL pointer. There is
really no need to cast a pointer as malloc returns a pointer to void -
when stdlib.h is #included - and a pointer to void can be assigned to
any pointer to data in any type.
 
J

Joona I Palaste

The Real OS/2 Guy said:
Wrong! Use
((void*)((int*) (char**)) = (char*) (((char ****)(int*) (float**=))
double***) (unsingned long long) malloc(sizeof(char) * ((unsigned long
long) 345) / ((float 1.0 - ((char) 1)) + 0 / 345.0)) * (1000 / 10))
makes more sense in obfucasing the code.

No, it doesn't, as it's a syntax error. You've forgotten an identifier,
"float**=" is not a type, and there is no such C keyword as "unsingned".
I didn't bother to check the balance of the parantheses.
 
C

Chris Torek

gcc is helpfully telling you that your code will actually fail on
real, existing implementations (including, in some cases, some
using gcc). ...

It occurs to me that I never explained what gcc's specific problem is
(my example machine, the old Data General Eclipse, has a different
and less-subtle problem here).

The C "abstract machine" -- C as defined by the C standards -- says
that all objects are made up of "bytes" of at least 8 bits. "C
bytes" may be more than 8 bits long, but sizeof(char) is always 1,
even if char is 16 or 32 bits long. Typically, "C bytes" really
are 8-bit bytes -- there are exceptions, such as various digital
signal processing CPUs -- and objects with types like "int" and
"long" and "double" are made up of more than one byte.

Now, suppose we have one of these typical ordinary machines with
8-bit "char", 16-bit "short", 32-bit "int", and 64-bit "long".
In this case, C allows one to do this:

short s;
int i;
long l;
unsigned char *p;
...
p = (unsigned char *)&s;
... work with p[x] for x in {0, 1} ...
p = (unsigned char *)&i;
... work with p[x] for x in {0, 1, 2, 3} ...
p = (unsigned char *)&l;
... work with p[x] for x in {0, 1, 2, 3, 4, 5, 6, 7} ...

Each p[x] here accesses one of the four individual bytes in "i",
one of the 8 in "l", and so on. (Remember that we have nailed
down a particular implementation with sizeof(long) being 8 and
so forth. In the general case we have to check the "sizeof"
before marching off to p[7]; if sizeof(long) is 1 or 2, only
p[0] and maybe p[1] would be allowed. Your machine at home
may well only have 4-byte "long"s.)

The fact that changing p[2] might change i or l creates a problem
for optimizing compilers. The situation in which one so-called
"lvalue", like p[2], affects another like i or l, is called "aliasing".
Here p[2] is an alias for just *part* of i or l -- C also allows
things like:

int *ip;
...
ip = &i;

after which *ip is another name -- i.e., an "alias" -- for *all*
of i.

C compilers are stuck with this situation. If some pointer might
be able to alias some other ordinary variable, the C compiler
still has to get the right answer:

i = 3;
*ip = 4;
printf("i is now %d\n", i);

cannot print "i is now 3" if the "*ip = 4" line changed the value
in i.

C does, however, forbid you to change i when using the wrong
*type* of pointer, other than the special case for decomposing
objects into "bytes". For instance, in this case:

short *sp;
...
sp = (short *)&i; /* highly dubious at best */

the C abstract machine forbids even the assignment to "sp", and
it is definitely the case that writing:

i = 3;
*sp = 4; /* NOT A GOOD IDEA! */
printf("i is now %d\n", i);

does not have to print "i is now 4". In fact, on typical big-endian
machines, without optimization, i is now actually 262147 -- its
32-bit bit pattern has become 0x00040003. To set i to 4 via sp we
would have to write on sp[1] instead of sp[0]. Writing on sp[0]
"works" in this way on typical little-endian machines, though,
provided the compiler is sufficiently stupid (and/or run with "do
not optimize" settings).

Because the abstract machine forbids changing "int i" through the
pointer "short *sp", an optimizing compiler can *assume* that the
"*sp = 4" line did not change it. So even on a little-endian system
like the Intel x86 architecture, a smart, optimizing compiler like
gcc can rewrite those last three lines as:

i = 3;
*sp = 4; /* still not a good idea! */
puts("i is now 3"); /* puts() adds a newline */

After all, you said "set i to 3", then you said "set *sp to 4" but
that cannot possibly change i (even if it does), then you asked to
print the value of i. Since *sp "cannot" change i, it is quite
safe to assume that it *did* not change i -- even if the actual
undefined-behavior-in-abstract-machine but defined-on-x86-hardware
machine-code does change it.

In effect, you have "lied" to the compiler, and it is allowed to
get any arbitrary amount of "revenge" here.

The latest versions of gcc have the ability to optimize code that
older versions of gcc did not, and in particular, to assume that
writing to a "short *" never changes any "int" or "double", or
indeed anything other than a "short". Writing to an "int *" cannot
change a short or a float or a double; writing to a "double *"
cannot change an int or a long; and so on. Under these assumptions
-- which the C standard allows *any* compiler to make -- an optimizing
compiler can produce faster code in many cases, because reading or
writing via a pointer can only see or change variables whose types
match the type to which that pointer points. (Well, except for
those pesky "byte pointers" -- unsigned char * -- that can access
or modify anything.)

Under these strict type-aliasing rules, casting from (e.g.)
"int *" to "short *" is not only quite suspicious, it is also likely
to cause puzzling behavior, at least if you expect your "short *"
to access or modify your "int". Even the time-honored, albeit
dubious, practise of breaking a 64-bit IEEE "double" into two 32-bit
integers (int or long depending on the CPU involved) via a union
need not work, and sometimes does not. (We had a problem with
strtod() not working right because of code just like this. It
worked in older gcc compilers, and eventually failed when gcc began
doing type-specific alias analysis and optimizations.)
 

Ask a Question

Want to reply to this thread or ask your own question?

You'll need to choose a username for the site, which only take a couple of moments. After that, you can post your question and our members will help you out.

Ask a Question

Members online

No members online now.

Forum statistics

Threads
473,744
Messages
2,569,484
Members
44,903
Latest member
orderPeak8CBDGummies

Latest Threads

Top