Thread

New s_lock.h fails on HPUX with gcc

Tom Lane <tgl@sss.pgh.pa.us> — 1998-07-06T20:02:25Z

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

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

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


#if !defined(S_LOCK)
// default definition of S_LOCK
#endif

// default definitions of other macros done in the same way

#endif /* HAS_TEST_AND_SET */


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.


BTW, I'd suggest making the definition of clear_lock for HPUX be

static const slock_t clear_lock =
{{-1, -1, -1, -1}};

The extra braces are needed to suppress warnings from gcc, and declaring
it const just seems like good practice.

			regards, tom lane

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

David Gould <dg@illustra.com> — 1998-07-09T07:07:37Z

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

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

Bruce Momjian <maillist@candle.pha.pa.us> — 1998-07-18T14:50:54Z

> ... because the conditional structure assumes that pgsql will only be
> built with non-gcc compilers on HPUX.
> 
> This is an entirely bogus assumption not only for HPUX, but for any
> other architecture that has gcc available.
> 
> 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 ...
> 
> 
> #if !defined(S_LOCK)
> // default definition of S_LOCK
> #endif
> 
> // default definitions of other macros done in the same way
> 
> #endif /* HAS_TEST_AND_SET */
> 
> 
> 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.
> 
> 
> BTW, I'd suggest making the definition of clear_lock for HPUX be
> 
> static const slock_t clear_lock =
> {{-1, -1, -1, -1}};
> 
> The extra braces are needed to suppress warnings from gcc, and declaring
> it const just seems like good practice.
> 
> 			regards, tom lane
> 
> 

Patch applied.  I just moved hpux out of the gcc/nogcc ifdef, so it
always gets hit.  Also changed the clear_lock stuff.

-- 
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)

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

Bruce Momjian <maillist@candle.pha.pa.us> — 1998-07-18T14:59:10Z

> ... because the conditional structure assumes that pgsql will only be
> built with non-gcc compilers on HPUX.
> 
> This is an entirely bogus assumption not only for HPUX, but for any
> other architecture that has gcc available.
> 
> 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:

I have moved platforms that have have common code for gcc and non-gcc to
their own section of s_lock.h.  Should make things easier.

-- 
Bruce Momjian                          |  830 Blythe Avenue
maillist@candle.pha.pa.us              |  Drexel Hill, Pennsylvania 19026
  +  If your life is a hard drive,     |  (610) 353-9879(w)
  +  Christ can be your backup.        |  (610) 853-3000(h)