Re: [HACKERS] New s_lock.h fails on HPUX with gcc - Mailing list pgsql-hackers

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@illustra.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@otter.mbay.net