Re: [HACKERS] New s_lock.h fails on HPUX with gcc

Sorry not to reply sooner, the people who pay me wanted me to work on their
stuff... sigh.

> ... because the conditional structure assumes that pgsql will only be
> built with non-gcc compilers on HPUX.

My fault. But, see below.

> This is an entirely bogus assumption not only for HPUX, but for any
> other architecture that has gcc available.

Not quite. Only for architectures whose S_LOCK code is identical under
both GCC and non GCC. Solaris for example has different code for the GCC
case vs the non GCC case.

> To be able to compile, I just duplicated the "#if defined(__hpux)"
> block into the "#if defined(__GNUC__)" part of the file, but that's
> a pretty grotty hack. I think that the right way to structure the
> file is just this:
>
> #if defined(HAS_TEST_AND_SET)
>
> #if defined(somearchitecture)
>
> #if defined(__GNUC__)
> // inline definition of tas here
> #else
> // non-inline definition of tas here, if default isn't adequate
> #endif
>
> // machine-dependent-but-compiler-independent definitions here
>
> #endif /* somearchitecture */
>
> // ... repeat above structure for each architecture supported ...
>
> On architectures where we don't have any special inline code for GCC,
> the inner "#if defined(__GNUC__)" can just be omitted in that
> architecture's block.
>
> The existing arrangement with an outer "#if defined(__GNUC__)" doesn't
> have any obvious benefit, and it encourages missed cases like this one.

I see your point and apologize for my oversight in the cases where the GCC
implementation is identical to the non gcc implementation.

However, I do think the current "outer" __GNUC__ block has some advantages
that as you say may not be obvious. It also, as you found, has some problems
that I did not notice.

- It works fine on platforms that don't have GCC.

- It works fine on platforms that have only GCC.

- It works fine on platforms that have both GCC and non-GCC but
have _different_ implementations of S_LOCK (eg Solaris).

- It requires duplicating a code block to make it work on platforms that
have both GCC and non-GCC and also have identical implementations of
S_LOCK for both compilers (eg HPUX). It might be

The main advantage of using _GCC_ as the outer condition is to unify
all the X86 unix flavors (bar SCO and Solaris) and eliminate a bunch of
the previously almost but not quite identical x86 gcc asm blocks in *BSD
and linux specific segments. These could also perhaps have been written:

#if defined(ix86) && (defined(linux) || defined(FreeBSD) || defined(NetBSD) || defined(BSDI) || defined(BeOS) ...)

GCC specific ix86 asm inline here

#endif /* defined(ix86) && (defined(linux) || defined(FreeBSD) || defined(NetBSD) || defined(BSDI) || defined(BeOS) ...) */

But I really hate complex platform conditionals as they are a fine source
of error.

Secondly, by testing for __GNUC__ it makes the unifying feature of
the various platforms explicit.

We also have a couple of platforms that potentially could have other
compilers than GCC (eg alpha, VMS), but our current S_LOCK code was clearly
written for GCC, so I stuck them in the GCC block.

Perhaps a look at the original unpatched 6.3.2 code will help explain
what I was trying to accomplish.

Still, your point about making it easy to miss some possibilities is well
taken. On the other hand, the #if block per platform gets pretty clumsy
when you have a half dozen major platforms that use the same compiler.

Perhaps something this would be better:

#if defined(__GNUC)
#if defined(x86)
x86 gcc stuff
#elsif defined(sparc)
sparc gcc stuff
#endif
#elsif defined(GCC_same_as_other_platform)
stuff that works for both
#elsif defined(some_doesn't_even_have_gcc_platform)
stuff that only works for proprietary C
#elsif defined(non_gcc_is_different_than_gcc_platform)
stuff that only works for proprietary C
#endif

I have worked a lot harder on this silly spinlock thing than I ever intended.
First there was a merge problem. Then I got sidetracked into checking out
the performance issue Bruce was concerned about, which was interesting
but time consuming, and now this. At this point I really want to do
"the right thing", but I also really want to move on.

So if you have a better idea than I outlined just above, or an objection,
I am very happy to hear it and try to make it right. But, let me know soon
otherwise I will put together a patch using the above method this weekend.

-dg

David Gould dg(at)illustra(dot)com 510.628.3783 or 510.305.9468
Informix Software (No, really) 300 Lakeside Drive Oakland, CA 94612
Q: Someone please enlighten me, but what are they packing into NT5 to
make it twice the size of NT4/EE? A: A whole chorus line of dancing
paperclips. -- mcgredo(at)otter(dot)mbay(dot)net