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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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'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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers

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

--
Sent via pgsql-hackers mailing list (pgsql-hackers@postgresql.org)
To make changes to your subscription:
http://www.postgresql.org/mailpref/pgsql-hackers