Thread: New s_lock.h fails on HPUX with gcc
... 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
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
dg@illustra.com (David Gould) writes:
>> 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.
Quite true. But it seems to me that the default assumption should be
that an architecture has the same code for GCC and other compilers;
first you get it going, then maybe you think about using asm inline
to optimize under GCC. With the existing structure of s_lock.h, the
easiest path is to miss out one case or the other completely.
Your example seems to be that all the x86/GCC platforms can be
consolidated, but AFAICS that's true whether you make the outer
conditional be GCC or platform.
> 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.
But cutting&pasting to start adding support for a new platform is pretty
simple and straightforward if there is only one block of code to look at.
There might be a tad more duplicated code after a while, but it'd be
easy to understand and hence easy to modify. I think the direction you
are headed reduces code duplication at a very high price in confusion
and fragility.
> 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.
Since I'm not doing the work, I'll shut up now and let you do what you
think best ;-)
regards, tom lane
> ... 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)
> ... 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)