Re: [HACKERS] s_lock.h default definitions are rather confused
On 2015-01-15 17:59:40 +0100, Andres Freund wrote: On 2015-01-15 11:56:24 -0500, Tom Lane wrote: Andres Freund and...@anarazel.de writes: On 2015-01-15 10:57:10 -0500, Tom Lane wrote: While I'll not cry too hard when we decide to break C89 compatibility, I don't want it to happen accidentally; so having a pretty old-school compiler in the farm seems important to me. I'd worked on setting up a modern gcc (or was it clang?) with the appropriate flags to warn about !C89 stuff some time back, but failed because of configure bugs. My recollection is that there isn't any reasonable way to get gcc to warn about C89 violations as such. -ansi -pedantic is not very fit for the purpose. It was clang, which has -Wc99-extensions/-Wc11-extensions. gcc-5 now has: * A new command-line option -Wc90-c99-compat has been added to warn about features not present in ISO C90, but present in ISO C99. * A new command-line option -Wc99-c11-compat has been added to warn about features not present in ISO C99, but present in ISO C11. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
On 2015-01-15 11:56:24 -0500, Tom Lane wrote: Andres Freund and...@anarazel.de writes: On 2015-01-15 10:57:10 -0500, Tom Lane wrote: While I'll not cry too hard when we decide to break C89 compatibility, I don't want it to happen accidentally; so having a pretty old-school compiler in the farm seems important to me. I'd worked on setting up a modern gcc (or was it clang?) with the appropriate flags to warn about !C89 stuff some time back, but failed because of configure bugs. My recollection is that there isn't any reasonable way to get gcc to warn about C89 violations as such. -ansi -pedantic is not very fit for the purpose. It was clang, which has -Wc99-extensions/-Wc11-extensions. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
Andres Freund and...@anarazel.de writes: On 2015-01-15 10:57:10 -0500, Tom Lane wrote: While I'll not cry too hard when we decide to break C89 compatibility, I don't want it to happen accidentally; so having a pretty old-school compiler in the farm seems important to me. I'd worked on setting up a modern gcc (or was it clang?) with the appropriate flags to warn about !C89 stuff some time back, but failed because of configure bugs. My recollection is that there isn't any reasonable way to get gcc to warn about C89 violations as such. -ansi -pedantic is not very fit for the purpose. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
and...@anarazel.de (Andres Freund) writes: On 2015-01-14 19:31:18 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Right now I think a #ifdef/undef S_UNLOCK in the relevant gcc section sufficient and acceptable. It's after all the HPPA section that doesn't really play by the rules. Works for me. Pushed something like that. Gaur has the note 'Runs infrequently' - I'm not sure whether that means we'll see the results anytime soon... That means it runs when I boot it up and launch a run ;-) ... the machine's old enough (and noisy enough) that I don't want to leave it turned on 24x7. I've launched a run now, expect results from gcc HEAD in an hour and a half or so. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
On 2015-01-14 12:27:42 -0500, Robert Haas wrote: On Mon, Jan 12, 2015 at 12:57 PM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-10 23:03:36 +0100, Andres Freund wrote: On 2015-01-10 16:09:42 -0500, Tom Lane wrote: I've not tried to build HEAD on my HPPA dinosaur for awhile, but I did just now, and I am presented with boatloads of this: ../../../src/include/storage/s_lock.h:759: warning: `S_UNLOCK' redefined ../../../src/include/storage/s_lock.h:679: warning: this is the location of the previous definition which is not too surprising because the default definition at line 679 precedes the HPPA-specific one at line 759. That's 0709b7ee72e4bc71ad07b7120acd117265ab51d0. Not too surprising that it broke and wasn't noticed without access to hppa - the hppa code uses gcc inline assembly outside of the big defined(__GNUC__) and inside the section headed Platforms that use non-gcc inline assembly. I'm not particularly interested in untangling the effects of the recent hackery in s_lock.h enough to figure out how the overall structure got broken, but I trust one of you will clean up the mess. I think it's easiest solved by moving the gcc inline assembly up to the rest of the gcc inline assembly. That'll require duplicating a couple lines, but seems easier to understand nonetheless. Not pretty. Robert, do you have a better idea? How about hoisting the entire hppa section up to the top of the file, before the __GNUC__ || __INTEL_COMPILER section? We could do that, but I'd rather not see that uglyness at the top everytime I open the file :P Right now I think a #ifdef/undef S_UNLOCK in the relevant gcc section sufficient and acceptable. It's after all the HPPA section that doesn't really play by the rules. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
Andres Freund and...@2ndquadrant.com writes: Right now I think a #ifdef/undef S_UNLOCK in the relevant gcc section sufficient and acceptable. It's after all the HPPA section that doesn't really play by the rules. Works for me. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
On Mon, Jan 12, 2015 at 12:57 PM, Andres Freund and...@2ndquadrant.com wrote: On 2015-01-10 23:03:36 +0100, Andres Freund wrote: On 2015-01-10 16:09:42 -0500, Tom Lane wrote: I've not tried to build HEAD on my HPPA dinosaur for awhile, but I did just now, and I am presented with boatloads of this: ../../../src/include/storage/s_lock.h:759: warning: `S_UNLOCK' redefined ../../../src/include/storage/s_lock.h:679: warning: this is the location of the previous definition which is not too surprising because the default definition at line 679 precedes the HPPA-specific one at line 759. That's 0709b7ee72e4bc71ad07b7120acd117265ab51d0. Not too surprising that it broke and wasn't noticed without access to hppa - the hppa code uses gcc inline assembly outside of the big defined(__GNUC__) and inside the section headed Platforms that use non-gcc inline assembly. I'm not particularly interested in untangling the effects of the recent hackery in s_lock.h enough to figure out how the overall structure got broken, but I trust one of you will clean up the mess. I think it's easiest solved by moving the gcc inline assembly up to the rest of the gcc inline assembly. That'll require duplicating a couple lines, but seems easier to understand nonetheless. Not pretty. Robert, do you have a better idea? How about hoisting the entire hppa section up to the top of the file, before the __GNUC__ || __INTEL_COMPILER section? -- Robert Haas EnterpriseDB: http://www.enterprisedb.com The Enterprise PostgreSQL Company -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
On 2015-01-10 23:03:36 +0100, Andres Freund wrote: On 2015-01-10 16:09:42 -0500, Tom Lane wrote: I've not tried to build HEAD on my HPPA dinosaur for awhile, but I did just now, and I am presented with boatloads of this: ../../../src/include/storage/s_lock.h:759: warning: `S_UNLOCK' redefined ../../../src/include/storage/s_lock.h:679: warning: this is the location of the previous definition which is not too surprising because the default definition at line 679 precedes the HPPA-specific one at line 759. That's 0709b7ee72e4bc71ad07b7120acd117265ab51d0. Not too surprising that it broke and wasn't noticed without access to hppa - the hppa code uses gcc inline assembly outside of the big defined(__GNUC__) and inside the section headed Platforms that use non-gcc inline assembly. I'm not particularly interested in untangling the effects of the recent hackery in s_lock.h enough to figure out how the overall structure got broken, but I trust one of you will clean up the mess. I think it's easiest solved by moving the gcc inline assembly up to the rest of the gcc inline assembly. That'll require duplicating a couple lines, but seems easier to understand nonetheless. Not pretty. Robert, do you have a better idea? Alternatively we could just add a #undef in the hppa gcc section and live with the uglyness. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
Andres Freund and...@2ndquadrant.com writes: On 2015-01-10 18:40:58 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Actually. It looks like I only translated the logic from barrier.h 1:1 and it already was broken there. Hm, it looks like the current code essentially is from 89779bf2c8f9aa480e0ceb8883f93e9d65c43a6e. There's a small difference, which is that the code actually worked as of that commit. Are you sure it actually worked on hppa !gcc? Sure, the s_lock.h gcc breakage is caused by Robert's s_lock.h commit (making spinlock proper barriers), but I don't see how the tree as of 89779bf2c8f9aa48 could work on !gcc hppa? Ah, sorry, I mixed up commit hashes. I can say that the !gcc HPPA build worked as of commit 44cd47c1d49655c5dd9648bde8e267617c3735b4, instead. I don't think I'd tried it since then, until yesterday. Could you check whether that heals that problem? I've verified that it allows me to build with gcc, even if I remove its compiler barrier definition. As of HEAD right now, the !gcc build is fine (well, there are a few warnings that have been there for awhile, but they're uninteresting). The gcc build is still spewing lots of ../../../../src/include/storage/s_lock.h:759: warning: `S_UNLOCK' redefined ../../../../src/include/storage/s_lock.h:679: warning: this is the location of the previous definition regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
[HACKERS] s_lock.h default definitions are rather confused
I've not tried to build HEAD on my HPPA dinosaur for awhile, but I did just now, and I am presented with boatloads of this: ../../../src/include/storage/s_lock.h:759: warning: `S_UNLOCK' redefined ../../../src/include/storage/s_lock.h:679: warning: this is the location of the previous definition which is not too surprising because the default definition at line 679 precedes the HPPA-specific one at line 759. I'm not particularly interested in untangling the effects of the recent hackery in s_lock.h enough to figure out how the overall structure got broken, but I trust one of you will clean up the mess. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
Andres Freund and...@2ndquadrant.com writes: Given you got the error above, you used gcc. Right. Have you used non-gcc compiler on hppa recently? I seem to recall you mentioning that that doesn't work sanely anymore? If so, perhaps we can just remove the !gcc variant? I'll try that shortly ... regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
On 2015-01-10 16:09:42 -0500, Tom Lane wrote: I've not tried to build HEAD on my HPPA dinosaur for awhile, but I did just now, and I am presented with boatloads of this: ../../../src/include/storage/s_lock.h:759: warning: `S_UNLOCK' redefined ../../../src/include/storage/s_lock.h:679: warning: this is the location of the previous definition which is not too surprising because the default definition at line 679 precedes the HPPA-specific one at line 759. That's 0709b7ee72e4bc71ad07b7120acd117265ab51d0. Not too surprising that it broke and wasn't noticed without access to hppa - the hppa code uses gcc inline assembly outside of the big defined(__GNUC__) and inside the section headed Platforms that use non-gcc inline assembly. I'm not particularly interested in untangling the effects of the recent hackery in s_lock.h enough to figure out how the overall structure got broken, but I trust one of you will clean up the mess. I think it's easiest solved by moving the gcc inline assembly up to the rest of the gcc inline assembly. That'll require duplicating a couple lines, but seems easier to understand nonetheless. Not pretty. Given you got the error above, you used gcc. Have you used non-gcc compiler on hppa recently? I seem to recall you mentioning that that doesn't work sanely anymore? If so, perhaps we can just remove the !gcc variant? Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
On 2015-01-10 17:58:10 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Given you got the error above, you used gcc. Have you used non-gcc compiler on hppa recently? I seem to recall you mentioning that that doesn't work sanely anymore? If so, perhaps we can just remove the !gcc variant? It still compiles, modulo some old and uninteresting warnings, but linking the postgres executable fails with usr/ccs/bin/ld: Unsatisfied symbols: pg_compiler_barrier_impl (code) make[2]: *** [postgres] Error 1 Ick, that one is my failure. Curiously, there are no compiler warnings about reference to undeclared function, which is odd because I see nothing that would declare that name as a function or macro for non-gcc HPPA. Yea, that's odd. But in any case, the fallback logic for compiler barriers evidently still needs work. Will fix (or well, try). It broke due to the atomics patch. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
On 2015-01-10 18:40:58 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: On 2015-01-11 00:06:41 +0100, Andres Freund wrote: Ick, that one is my failure. Actually. It looks like I only translated the logic from barrier.h 1:1 and it already was broken there. Hm, it looks like the current code essentially is from 89779bf2c8f9aa480e0ceb8883f93e9d65c43a6e. There's a small difference, which is that the code actually worked as of that commit. Are you sure it actually worked on hppa !gcc? Sure, the s_lock.h gcc breakage is caused by Robert's s_lock.h commit (making spinlock proper barriers), but I don't see how the tree as of 89779bf2c8f9aa48 could work on !gcc hppa? At least bgworker.c already used read and write barriers back then. Those were redefined to a compiler barrier by /* HPPA doesn't do either read or write reordering */ #define pg_memory_barrier() pg_compiler_barrier() but barrier.h only provided (back then) compiler barriers for icc, gcc, ia64 (without a compiler restriction?) and win32. So I don't see how that could have worked. On a reread, you even noted But note this patch only fixes things for gcc, not for builds with HP's compiler. in the commit message.. Anyway, i've pushed a fix for that to master. For one I'm not yet sure if it's actually broken in the backbranches (and if we care), for another the = 9.4 fix will not have a single file in common anyway. Any opinion on that? Could you check whether that heals that problem? I've verified that it allows me to build with gcc, even if I remove its compiler barrier definition. Unless somebody protests I'm going to introduce a generic fallback compiler barrier that's just a extern function. Seems reasonable to me. An empty external function should do the job for any compiler that isn't doing cross-procedural analysis. And I really doubt any of those that do fail to provide a compiler barrier... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
Andres Freund and...@2ndquadrant.com writes: Given you got the error above, you used gcc. Have you used non-gcc compiler on hppa recently? I seem to recall you mentioning that that doesn't work sanely anymore? If so, perhaps we can just remove the !gcc variant? It still compiles, modulo some old and uninteresting warnings, but linking the postgres executable fails with usr/ccs/bin/ld: Unsatisfied symbols: pg_compiler_barrier_impl (code) make[2]: *** [postgres] Error 1 Curiously, there are no compiler warnings about reference to undeclared function, which is odd because I see nothing that would declare that name as a function or macro for non-gcc HPPA. But in any case, the fallback logic for compiler barriers evidently still needs work. ... further on ... Looks like somebody has recently broken pg_dump, too: cc: pg_dump.c, line 319: error 1521: Incorrect initialization. cc: pg_dump.c, line 320: error 1521: Incorrect initialization. cc: pg_dump.c, line 321: error 1521: Incorrect initialization. cc: pg_dump.c, line 322: error 1521: Incorrect initialization. cc: pg_dump.c, line 323: error 1521: Incorrect initialization. cc: pg_dump.c, line 324: error 1521: Incorrect initialization. cc: pg_dump.c, line 326: error 1521: Incorrect initialization. cc: pg_dump.c, line 327: error 1521: Incorrect initialization. cc: pg_dump.c, line 329: error 1521: Incorrect initialization. cc: pg_dump.c, line 333: error 1521: Incorrect initialization. cc: pg_dump.c, line 335: error 1521: Incorrect initialization. cc: pg_dump.c, line 336: error 1521: Incorrect initialization. cc: pg_dump.c, line 337: error 1521: Incorrect initialization. cc: pg_dump.c, line 338: error 1521: Incorrect initialization. make[3]: *** [pg_dump.o] Error 1 This one looks like overoptimistic assumptions about what's legal in an initializer. We could probably fix that by reverting the option variables to statics; I see no benefit to be had from having them as dynamic variables in main(). regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
On 2015-01-11 00:06:41 +0100, Andres Freund wrote: On 2015-01-10 17:58:10 -0500, Tom Lane wrote: Andres Freund and...@2ndquadrant.com writes: Given you got the error above, you used gcc. Have you used non-gcc compiler on hppa recently? I seem to recall you mentioning that that doesn't work sanely anymore? If so, perhaps we can just remove the !gcc variant? It still compiles, modulo some old and uninteresting warnings, but linking the postgres executable fails with usr/ccs/bin/ld: Unsatisfied symbols: pg_compiler_barrier_impl (code) make[2]: *** [postgres] Error 1 Ick, that one is my failure. Actually. It looks like I only translated the logic from barrier.h 1:1 and it already was broken there. Hm, it looks like the current code essentially is from 89779bf2c8f9aa480e0ceb8883f93e9d65c43a6e. That introduced: +#elif defined(__hppa) || defined(__hppa__) /* HP PA-RISC */ + +/* HPPA doesn't do either read or write reordering */ +#define pg_memory_barrier()pg_compiler_barrier() That works well enough for hppa on gcc because there's generic compiler barrier for the latter. But for HPUX on own compiler no compiler barrier is defined, because: /* * If read or write barriers are undefined, we upgrade them to full memory * barriers. * * If a compiler barrier is unavailable, you probably don't want a full * memory barrier instead, so if you have a use case for a compiler barrier, * you'd better use #ifdef. */ #if !defined(pg_read_barrier) #define pg_read_barrier() pg_memory_barrier() #endif #if !defined(pg_write_barrier) #define pg_write_barrier() pg_memory_barrier() #endif Unless somebody protests I'm going to introduce a generic fallback compiler barrier that's just a extern function. Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training Services -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
Re: [HACKERS] s_lock.h default definitions are rather confused
Andres Freund and...@2ndquadrant.com writes: On 2015-01-11 00:06:41 +0100, Andres Freund wrote: Ick, that one is my failure. Actually. It looks like I only translated the logic from barrier.h 1:1 and it already was broken there. Hm, it looks like the current code essentially is from 89779bf2c8f9aa480e0ceb8883f93e9d65c43a6e. There's a small difference, which is that the code actually worked as of that commit. I suspect this got broken by Robert's barrier-hacking of a few months ago. I don't think I've tried the non-gcc build since committing 89779bf2c8f9aa48 :-( Unless somebody protests I'm going to introduce a generic fallback compiler barrier that's just a extern function. Seems reasonable to me. An empty external function should do the job for any compiler that isn't doing cross-procedural analysis. regards, tom lane -- Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org) To make changes to your subscription: http://www.postgresql.org/mailpref/pgsql-hackers
