compile warning

Alvaro Herrera wrote:

I'm seeing this compile warning on today's CVS tip:

$ make src/backend/commands/tablecmds.o
gcc -O2 -g -Wall -Wmissing-prototypes -Wmissing-declarations -I./src/include -D_GNU_SOURCE -c -o src/backend/commands/tablecmds.o src/backend/commands/tablecmds.c
src/backend/commands/tablecmds.c: In function `validateForeignKeyConstraint':
src/backend/commands/tablecmds.c:3528: warning: dereferencing type-punned pointer will break strict-aliasing rules

$ gcc --version
gcc (GCC) 3.3.1 (Mandrake Linux 9.2 3.3.1-2mdk)
Copyright (C) 2003 Free Software Foundation, Inc.
This is free software; see the source for copying conditions. There is NO
warranty; not even for MERCHANTABILITY or FITNESS FOR A PARTICULAR PURPOSE.

$ src/bin/pg_config/pg_config --configure
'--enable-debug' '--enable-nls=es' '--enable-integer-datetimes'

If you change the offending line to:

fcinfo.context = (struct Node *) &trigdata;

I know it shouldn't make a difference, but it is worth a try.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Thu, Oct 09, 2003 at 11:51:09PM -0400, Bruce Momjian wrote:

Alvaro Herrera wrote:

I'm seeing this compile warning on today's CVS tip:

$ make src/backend/commands/tablecmds.o
gcc -O2 -g -Wall -Wmissing-prototypes -Wmissing-declarations -I./src/include -D_GNU_SOURCE -c -o src/backend/commands/tablecmds.o src/backend/commands/tablecmds.c
src/backend/commands/tablecmds.c: In function `validateForeignKeyConstraint':
src/backend/commands/tablecmds.c:3528: warning: dereferencing type-punned pointer will break strict-aliasing rules

If you change the offending line to:

fcinfo.context = (struct Node *) &trigdata;

I know it shouldn't make a difference, but it is worth a try.

Nope, same warning. I don't know what it means though. I tried some
other things to correct it, but I can't find exactly what it's
complaining about. What is a "type-punned pointer"?

Looking in Google finds this thread first:
http://www.mail-archive.com/freebsd-current@freebsd.org/msg58957.html
which is full of a very ugly kernel macro (I'm happy to stay away from
that):

http://www.mail-archive.com/freebsd-current@freebsd.org/msg58957.html

This other guy actually posted an useful excerpt from the GCC manpage:
http://www.ethereal.com/lists/ethereal-dev/200309/msg00342.html

So, I still don't understand what's the noise about. However I think
there's no way to silence the warning without uglifying the structs a
lot by means of some union.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"I would rather have GNU than GNOT." (ccchips, lwn.net/Articles/37595/)

Alvaro Herrera wrote:

On Thu, Oct 09, 2003 at 11:51:09PM -0400, Bruce Momjian wrote:

Alvaro Herrera wrote:

I'm seeing this compile warning on today's CVS tip:

$ make src/backend/commands/tablecmds.o
gcc -O2 -g -Wall -Wmissing-prototypes -Wmissing-declarations -I./src/include -D_GNU_SOURCE -c -o src/backend/commands/tablecmds.o src/backend/commands/tablecmds.c
src/backend/commands/tablecmds.c: In function `validateForeignKeyConstraint':
src/backend/commands/tablecmds.c:3528: warning: dereferencing type-punned pointer will break strict-aliasing rules

If you change the offending line to:

fcinfo.context = (struct Node *) &trigdata;

I know it shouldn't make a difference, but it is worth a try.

Nope, same warning. I don't know what it means though. I tried some
other things to correct it, but I can't find exactly what it's
complaining about. What is a "type-punned pointer"?

Looking in Google finds this thread first:
http://www.mail-archive.com/freebsd-current@freebsd.org/msg58957.html
which is full of a very ugly kernel macro (I'm happy to stay away from
that):

http://www.mail-archive.com/freebsd-current@freebsd.org/msg58957.html

This other guy actually posted an useful excerpt from the GCC manpage:
http://www.ethereal.com/lists/ethereal-dev/200309/msg00342.html

So, I still don't understand what's the noise about. However I think
there's no way to silence the warning without uglifying the structs a
lot by means of some union.

I am still confused. I understand the example listed in the last URL:

union a_union {
int i;
double d;
};

int f() {
a_union t;
t.d = 3.0;
return t.i;
}

will work fine because you are accessing the values through the union.
However in this example, also from that URL:

int f() {
a_union t;
int* ip;
t.d = 3.0;
ip = &t.i;
return *ip;
}

there is a problem because it assumes that the int and double _start_
at the same address in the structure. It is probabably a bad idea to be
taking passing pointers out of a union.

However, we aren't using unions in the query being complainted about.

In our code mentioned above, we have:

fcinfo.context = (Node *) &trigdata;

We are taking the address of a structure (not a union), but we are
assuming that a "struct TriggerData" pointer can be accessed through a
"struct Node" pointer. Now, both structures begin with a NodeTag
element, so it should be OK, but I guess the compiler guys don't know
that. However, I thought the cast shouldn't cause a problem at all.

Can someone with gcc 3.3.1 make up a little test program to illustrate
the bug? Does taking the adddress of any structure an casting it cause
this warning? I would think not because we must do that lots of places
in our code.

This seems to be a bug in gcc-3.3.1. -fstrict-aliasing is enabled by
-O2 or higher optimization in gcc 3.3.1.

Now that I think of it, they might be talking about an optimization
called register aliasing, where they are taking the structure and
mapping it to a CPU register for some optimization, and what we are
doing is to store a different structure in there that will not fit in a
register. A Node will fit in a register (it is only an enum) but the
TriggerData structure will not, meaning the code has to spill the
register to memory, then access the full structure, or something like
that.

Here are the places reported to generated warnings in our code by the
Cygwin guys:

tablecmds.c:3528: warning: dereferencing type-punned pointer will break strict-aliasing rules
execQual.c:749: warning: dereferencing type-punned pointer will break strict-aliasing rules
execQual.c:995: warning: dereferencing type-punned pointer will break strict-aliasing rules
pg_shmem.c:368: warning: passing arg 1 of `shmdt' from incompatible pointer type
proc.c:1016: warning: dereferencing type-punned pointer will break strict-aliasing rules
proc.c:1057: warning: dereferencing type-punned pointer will break strict-aliasing rules
proc.c:1123: warning: dereferencing type-punned pointer will break strict-aliasing rules
command.c:1283: warning: dereferencing type-punned pointer will break strict-aliasing rules

Looking at the proc.c cases, we have:

MemSet(&timeval, 0, sizeof(struct itimerval));

MemSet is passing struct itimerval * to an *int32, again a case of passing
a structure pointer to something to a data type that will fit in a
register.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

Bruce Momjian wrote:

This seems to be a bug in gcc-3.3.1. -fstrict-aliasing is enabled by
-O2 or higher optimization in gcc 3.3.1.

Now that I think of it, they might be talking about an optimization
called register aliasing, where they are taking the structure and
mapping it to a CPU register for some optimization, and what we are
doing is to store a different structure in there that will not fit in a
register. A Node will fit in a register (it is only an enum) but the
TriggerData structure will not, meaning the code has to spill the
register to memory, then access the full structure, or something like
that.

Did you mean register renaming? If so, it is only turned on by -O3. But I can see that strict aliasing does help the compiler make the right guess as to which registers to use for what, even without register renaming.

http://gcc.gnu.org/onlinedocs/gcc-3.3.1/gcc/Optimize-Options.html#Optimize%20Options
says:

|-O|

|-O1|
Optimize. Optimizing compilation takes somewhat more time, and a lot
more memory for a large function.

With |-O|, the compiler tries to reduce code size and execution
time, without performing any optimizations that take a great deal of
compilation time.

|-O| turns on the following optimization flags:

-fdefer-pop
-fmerge-constants
-fthread-jumps
-floop-optimize
-fcrossjumping
-fif-conversion
-fif-conversion2
-fdelayed-branch
-fguess-branch-probability
-fcprop-registers

|-O| also turns on |-fomit-frame-pointer| on machines where doing so
does not interfere with debugging.

|-O2|
Optimize even more. GCC performs nearly all supported optimizations
that do not involve a space-speed tradeoff. The compiler does not
perform loop unrolling or function inlining when you specify |-O2|.
As compared to |-O|, this option increases both compilation time and
the performance of the generated code.

|-O2| turns on all optimization flags specified by |-O|. It also
turns on the following optimization flags:

-fforce-mem
-foptimize-sibling-calls
-fstrength-reduce
-fcse-follow-jumps -fcse-skip-blocks
-frerun-cse-after-loop -frerun-loop-opt
-fgcse -fgcse-lm -fgcse-sm
-fdelete-null-pointer-checks
-fexpensive-optimizations
-fregmove
-fschedule-insns -fschedule-insns2
-fsched-interblock -fsched-spec
-fcaller-saves
-fpeephole2
-freorder-blocks -freorder-functions
-fstrict-aliasing
-falign-functions -falign-jumps
-falign-loops -falign-labels

Please note the warning under |-fgcse| about invoking |-O2| on
programs that use computed gotos.

|-O3|
Optimize yet more. |-O3| turns on all optimizations specified by
|-O2| and also turns on the |-finline-functions| and
|-frename-registers| options.

In the Linux kernel, you can see this in include/linux/tcp.h:

/*
* The union cast uses a gcc extension to avoid aliasing problems
* (union is compatible to any of its members)
* This means this part of the code is -fstrict-aliasing safe now.
*/
union tcp_word_hdr {
struct tcphdr hdr;
__u32 words[5];
};

#define tcp_flag_word(tp) ( ((union tcp_word_hdr *)(tp))->words [3])

Maybe this gives us a clue.

cheers

andrew

Andrew Dunstan wrote:

Bruce Momjian wrote:

This seems to be a bug in gcc-3.3.1. -fstrict-aliasing is enabled by
-O2 or higher optimization in gcc 3.3.1.

According to the C standard, it's illegal to access a data with a
pointer of the wrong type. The only exception is "char *".
This can be used by compilers to pipeline loops, or to reorder instructions.
For example

void dummy(double *out, int *in, int len)
{
int j;
for (j=0;j<len;j++)
out[j] = 1.0/in[j];
}

Can be pipelined if a compiler relies on strict aliasing: it's
guaranteed that writing to out[5] won't overwrite in[6].

I think MemSet violates strict aliasing: it writes to the given address
with (int32*). gcc might move the instructions around.
I would disable strict aliasing with -fno-strict-aliasing.

In the Linux kernel, you can see this in include/linux/tcp.h:

/*
* The union cast uses a gcc extension to avoid aliasing problems
* (union is compatible to any of its members)
* This means this part of the code is -fstrict-aliasing safe now.
*/

The kernel is still compiled with -fno-strict-aliasing - I'm not sure if
there are outstanding problems, or if it's just a safety precaution.

--
Manfred

----- Original Message -----
From: "Manfred Spraul" <manfred@colorfullife.com>

The kernel is still compiled with -fno-strict-aliasing - I'm not sure if
there are outstanding problems, or if it's just a safety precaution.

We should probably do likewise, at least until this is cleaned up, if that's
what we want to do, again probably from an overabundance of caution.

cheers

andrew

I have a fix for this which I will post to patches - essentially you cast
the pointers to (void *) and the compiler doesn't complain. It would be a
pity to turn off strict aliasing altogether, as it is known to improve
performance in some cases.

Tested on Cygwin/GCC 3.3.1

cheers

andrew

----- Original Message -----
From: "Andrew Dunstan" <andrew@dunslane.net>
To: "PostgreSQL Hackers Mailing List" <pgsql-hackers@postgresql.org>
Sent: Saturday, October 11, 2003 8:58 AM
Subject: Re: [HACKERS] compile warning

----- Original Message -----
From: "Manfred Spraul" <manfred@colorfullife.com>

The kernel is still compiled with -fno-strict-aliasing - I'm not sure if
there are outstanding problems, or if it's just a safety precaution.

We should probably do likewise, at least until this is cleaned up, if

that's

Andrew Dunstan wrote:

I have a fix for this which I will post to patches - essentially you cast
the pointers to (void *) and the compiler doesn't complain. It would be a
pity to turn off strict aliasing altogether, as it is known to improve
performance in some cases.

Tested on Cygwin/GCC 3.3.1

I am not sure about the patch. I know it fixes it, but is the compiler
actually reporting a valid concern, or is it broken? Is it complaining
about passing a struct pointer of one type to another? Don't we do that
all over the place?

I hate to add a patch just to fix a buggy version of a compiler. If we
do apply this patch, I think we should cast to (void *), then to the
valid type, and add a comment in each instance about its purpose.

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>
To: "Andrew Dunstan" <andrew@dunslane.net>
Cc: "PostgreSQL Hackers Mailing List" <pgsql-hackers@postgresql.org>
Sent: Saturday, October 11, 2003 11:26 AM
Subject: Re: [HACKERS] compile warning

Andrew Dunstan wrote:

I have a fix for this which I will post to patches - essentially you

cast

the pointers to (void *) and the compiler doesn't complain. It would be

a

pity to turn off strict aliasing altogether, as it is known to improve
performance in some cases.

Tested on Cygwin/GCC 3.3.1

I am not sure about the patch. I know it fixes it, but is the compiler
actually reporting a valid concern, or is it broken? Is it complaining
about passing a struct pointer of one type to another? Don't we do that
all over the place?

I hate to add a patch just to fix a buggy version of a compiler. If we
do apply this patch, I think we should cast to (void *), then to the
valid type, and add a comment in each instance about its purpose.

This is not a compiler bug. The compiler is behaving perfectly correctly.
See http://www.gnu.org/software/gcc/bugs.html#nonbugs_c.

See also http://mail-index.netbsd.org/tech-kern/2003/08/11/0001.html for
more info.

Actually, the fact that we get so few warnings on this is a monument to how
clean our code is, although the warnings are not guaranteed to catch every
instance of illegal type-punning.

If you look through the code you will see that we are casting to void * all
over the place. (I count 772 instances of the string "void *" in the code).

AFAIK this patch will inhibit the compiler from making type alias
assumptions which could result in nasty reordering of operations, but I
could be wrong. The other problem could be pointer misalignment, but if so
we would surely have seen it from straight casts by now - I'd be more
worried about operation reordering than misalignment, but maybe this would
need to be tested on a platform with heavier alignment restrictions than any
machine I have (a SPARC, say?).

If you don't want to do this we could turn off strict-aliasing. You might
take a performance hit, though - see
http://www.freetype.org/pipermail/devel/2003-June/009452.html for info on
what the FreeType people found.

There are more fundamental ways of addressing this, but they are far more
intrusive to the code, and presumably we don't want that at this stage in
the cycle. Incidentally, I understand that other compilers than gcc are
starting to implment this part of the standard, so even if we turn it off
for gcc we'll have to deal with it eventually.

cheers

andrew

cheers

andrew

Agreed. Patch applied. I was confused because the original posted URL
mentioned unions, which was a different alignment issue from just
structure pointer compatibility.

---------------------------------------------------------------------------

Andrew Dunstan wrote:

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>
To: "Andrew Dunstan" <andrew@dunslane.net>
Cc: "PostgreSQL Hackers Mailing List" <pgsql-hackers@postgresql.org>
Sent: Saturday, October 11, 2003 11:26 AM
Subject: Re: [HACKERS] compile warning

Andrew Dunstan wrote:

I have a fix for this which I will post to patches - essentially you

cast

the pointers to (void *) and the compiler doesn't complain. It would be

a

pity to turn off strict aliasing altogether, as it is known to improve
performance in some cases.

Tested on Cygwin/GCC 3.3.1

I am not sure about the patch. I know it fixes it, but is the compiler
actually reporting a valid concern, or is it broken? Is it complaining
about passing a struct pointer of one type to another? Don't we do that
all over the place?

I hate to add a patch just to fix a buggy version of a compiler. If we
do apply this patch, I think we should cast to (void *), then to the
valid type, and add a comment in each instance about its purpose.

This is not a compiler bug. The compiler is behaving perfectly correctly.
See http://www.gnu.org/software/gcc/bugs.html#nonbugs_c.

See also http://mail-index.netbsd.org/tech-kern/2003/08/11/0001.html for
more info.

Actually, the fact that we get so few warnings on this is a monument to how
clean our code is, although the warnings are not guaranteed to catch every
instance of illegal type-punning.

If you look through the code you will see that we are casting to void * all
over the place. (I count 772 instances of the string "void *" in the code).

AFAIK this patch will inhibit the compiler from making type alias
assumptions which could result in nasty reordering of operations, but I
could be wrong. The other problem could be pointer misalignment, but if so
we would surely have seen it from straight casts by now - I'd be more
worried about operation reordering than misalignment, but maybe this would
need to be tested on a platform with heavier alignment restrictions than any
machine I have (a SPARC, say?).

If you don't want to do this we could turn off strict-aliasing. You might
take a performance hit, though - see
http://www.freetype.org/pipermail/devel/2003-June/009452.html for info on
what the FreeType people found.

There are more fundamental ways of addressing this, but they are far more
intrusive to the code, and presumably we don't want that at this stage in
the cycle. Incidentally, I understand that other compilers than gcc are
starting to implment this part of the standard, so even if we turn it off
for gcc we'll have to deal with it eventually.

cheers

andrew

cheers

andrew

---------------------------(end of broadcast)---------------------------
TIP 8: explain analyze is your friend

-- 
  Bruce Momjian                        |  http://candle.pha.pa.us
  pgman@candle.pha.pa.us               |  (610) 359-1001
  +  If your life is a hard drive,     |  13 Roberts Road
  +  Christ can be your backup.        |  Newtown Square, Pennsylvania 19073

On Sat, Oct 11, 2003 at 12:31:35PM -0400, Bruce Momjian wrote:

Agreed. Patch applied. I was confused because the original posted URL
mentioned unions, which was a different alignment issue from just
structure pointer compatibility.

In the article Andrew mentioned, it is said that the unions thingie is a
GCC extension for solving the problem, thus unportable.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"If it wasn't for my companion, I believe I'd be having
the time of my life" (John Dunbar)

On Sat, Oct 11, 2003 at 12:17:42PM -0400, Andrew Dunstan wrote:

If you don't want to do this we could turn off strict-aliasing. You might
take a performance hit, though - see
http://www.freetype.org/pipermail/devel/2003-June/009452.html for info on
what the FreeType people found.

See the followup. That was actually a mistake in the metodology that
turned -O2 off. The real performance decrease is said to be 1-2%, not
the 100% that the original articly says.

--
Alvaro Herrera (<alvherre[a]dcc.uchile.cl>)
"En las profundidades de nuestro inconsciente hay una obsesiva necesidad
de un universo l�gico y coherente. Pero el universo real se halla siempre
un paso m�s all� de la l�gica" (Irulan)

It has just been pointed out to me that the Freetype guy misconfigured his
test settings, so the performance gain was not great.

See http://www.freetype.org/pipermail/devel/2003-June/009453.html

the other points are valid, though. I think we should be proud of the fact
we can do this :-) - other projects have just given up on it and
use -fno-strict-aliasing.

cheers

andrew

----- Original Message -----
From: "Andrew Dunstan" <andrew@dunslane.net>
To: "PostgreSQL Hackers Mailing List" <pgsql-hackers@postgresql.org>
Sent: Saturday, October 11, 2003 12:17 PM
Subject: Re: [HACKERS] compile warning

----- Original Message -----
From: "Bruce Momjian" <pgman@candle.pha.pa.us>
To: "Andrew Dunstan" <andrew@dunslane.net>
Cc: "PostgreSQL Hackers Mailing List" <pgsql-hackers@postgresql.org>
Sent: Saturday, October 11, 2003 11:26 AM
Subject: Re: [HACKERS] compile warning

Andrew Dunstan wrote:

I have a fix for this which I will post to patches - essentially you

cast

the pointers to (void *) and the compiler doesn't complain. It would

be

a

pity to turn off strict aliasing altogether, as it is known to improve
performance in some cases.

Tested on Cygwin/GCC 3.3.1

I am not sure about the patch. I know it fixes it, but is the compiler
actually reporting a valid concern, or is it broken? Is it complaining
about passing a struct pointer of one type to another? Don't we do that
all over the place?

I hate to add a patch just to fix a buggy version of a compiler. If we
do apply this patch, I think we should cast to (void *), then to the
valid type, and add a comment in each instance about its purpose.

This is not a compiler bug. The compiler is behaving perfectly correctly.
See http://www.gnu.org/software/gcc/bugs.html#nonbugs_c.

See also http://mail-index.netbsd.org/tech-kern/2003/08/11/0001.html for
more info.

Actually, the fact that we get so few warnings on this is a monument to

how

clean our code is, although the warnings are not guaranteed to catch every
instance of illegal type-punning.

If you look through the code you will see that we are casting to void *

all

over the place. (I count 772 instances of the string "void *" in the

code).

AFAIK this patch will inhibit the compiler from making type alias
assumptions which could result in nasty reordering of operations, but I
could be wrong. The other problem could be pointer misalignment, but if so
we would surely have seen it from straight casts by now - I'd be more
worried about operation reordering than misalignment, but maybe this would
need to be tested on a platform with heavier alignment restrictions than

any