Обсуждение: 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
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
Andres Freund <andres@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
Andres Freund <andres@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
On 2015-01-10 17:58:10 -0500, Tom Lane wrote: > Andres Freund <andres@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
On 2015-01-11 00:06:41 +0100, Andres Freund wrote: > On 2015-01-10 17:58:10 -0500, Tom Lane wrote: > > Andres Freund <andres@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'dbetter 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
Andres Freund <andres@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
On 2015-01-10 18:40:58 -0500, Tom Lane wrote: > Andres Freund <andres@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
Andres Freund <andres@2ndquadrant.com> writes:
> On 2015-01-10 18:40:58 -0500, Tom Lane wrote:
>> Andres Freund <andres@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
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
On Mon, Jan 12, 2015 at 12:57 PM, Andres Freund <andres@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
On 2015-01-14 12:27:42 -0500, Robert Haas wrote: > On Mon, Jan 12, 2015 at 12:57 PM, Andres Freund <andres@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
Andres Freund <andres@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
On 2015-01-14 19:31:18 -0500, Tom Lane wrote: > Andres Freund <andres@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... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
andres@anarazel.de (Andres Freund) writes:
> On 2015-01-14 19:31:18 -0500, Tom Lane wrote:
>> Andres Freund <andres@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
I wrote:
> I've launched a run now, expect results from gcc HEAD in an hour and
> a half or so.
... and it's happy. Thanks!
BTW, the reason I went to the trouble of cranking up the buildfarm scripts
on that machine (and it was painful :-() is that I don't believe any other
buildfarm members are running compilers old enough to complain about some
of the things these will. In particular:
* I've got gaur configured so it will throw "array subscript of type char"
complaints whenever somebody forgets to cast a <ctype.h> function argument
to unsigned char.
* pademelon will complain about // comments, variable-sized local arrays,
flexible array syntax, non-static function definition after static
declaration, and probably some other C89 violations that I am not
remembering right now.
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.
regards, tom lane
On 2015-01-15 10:57:10 -0500, Tom Lane wrote: > * I've got gaur configured so it will throw "array subscript of type char" > complaints whenever somebody forgets to cast a <ctype.h> function argument > to unsigned char. But, but. That would never happen to anyone (hides). > 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. Yea, agreed. I also don't think we want to adopt all of C99 at once, but rather do it piecemal. Feature by feature. 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. I think Robert has committed most of the fixes since, and I now actually could do the rest one of these days... Greetings, Andres Freund -- Andres Freund http://www.2ndQuadrant.com/PostgreSQL Development, 24x7 Support, Training & Services
Andres Freund <andres@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
On 2015-01-15 11:56:24 -0500, Tom Lane wrote: > Andres Freund <andres@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
On 2015-01-15 17:59:40 +0100, Andres Freund wrote: > On 2015-01-15 11:56:24 -0500, Tom Lane wrote: > > Andres Freund <andres@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