Re: 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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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

Andres Freund <andres(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)com> writes:
> On 2015-01-10 18:40:58 -0500, Tom Lane wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)2ndquadrant(dot)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(at)anarazel(dot)de (Andres Freund) writes:
> On 2015-01-14 19:31:18 -0500, Tom Lane wrote:
>> Andres Freund <andres(at)2ndquadrant(dot)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(at)anarazel(dot)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(at)anarazel(dot)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(at)anarazel(dot)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