Обсуждение: compile warning
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.osrc/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' -- Alvaro Herrera (<alvherre[a]dcc.uchile.cl>) "In Europe they call me Niklaus Wirth; in the US they call me Nickel's worth. That's because in Europe they call me by name, and in the US by value!"
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.osrc/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, Pennsylvania19073
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.osrc/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.osrc/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 rulesexecQual.c:749:warning: dereferencing type-punned pointer will break strict-aliasing rulesexecQual.c:995: warning: dereferencingtype-punned pointer will break strict-aliasing rulespg_shmem.c:368: warning: passing arg 1 of `shmdt' from incompatiblepointer typeproc.c:1016: warning: dereferencing type-punned pointer will break strict-aliasing rulesproc.c:1057:warning: dereferencing type-punned pointer will break strict-aliasing rulesproc.c:1123: warning: dereferencingtype-punned pointer will break strict-aliasing rulescommand.c:1283: warning: dereferencing type-punned pointerwill 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, Pennsylvania19073
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 compilermake 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 takea 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 tcphdrhdr; __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 > what we want to do, again probably from an overabundance of caution. > > cheers > > andrew > > > ---------------------------(end of broadcast)--------------------------- > TIP 9: the planner will ignore your desire to choose an index scan if your > joining column's datatypes do not match
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, Pennsylvania19073
----- 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, Pennsylvania19073
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 > 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