Обсуждение: [PATCH] Porting small OpenBSD changes.
Hi,
Compilation pass, make check passes.
Motivations :
- Reducing OpenBSD postfgresql maintainer internal changes bookeeping if those small changes make sense for the PostgreSQL developers.
Hope it is good.
Thanks in advance.
Kind regards.
Вложения
David CARLIER <devnexen@gmail.com> writes: > - Reducing OpenBSD postfgresql maintainer internal changes bookeeping if > those small changes make sense for the PostgreSQL developers. Hm. The s_lock.c change is surely fine if OpenBSD maintainers say it is. Not sure about adding Motorola 88K support to s_lock.h ... is anybody really still interested in that platform? OTOH, we still have M68K and VAX stanzas in that file, so I suppose it's silly to complain about 88K. A bigger issue is that I wonder whether that code has ever been tested: it does not look to me like the __asm__ call is even syntactically correct. There should be colons in it. regards, tom lane
On 20 November 2017 at 18:49, Tom Lane <tgl@sss.pgh.pa.us> wrote:
David CARLIER <devnexen@gmail.com> writes:
> - Reducing OpenBSD postfgresql maintainer internal changes bookeeping if
> those small changes make sense for the PostgreSQL developers.
Hm. The s_lock.c change is surely fine if OpenBSD maintainers say it is.
Not sure about adding Motorola 88K support to s_lock.h ... is anybody
really still interested in that platform?
Yes there is :-)
OTOH, we still have M68K
and VAX stanzas in that file, so I suppose it's silly to complain
about 88K. A bigger issue is that I wonder whether that code has
ever been tested: it does not look to me like the __asm__ call is
even syntactically correct. There should be colons in it.
True :-) corrected. Thanks.
regards, tom lane
Вложения
On Mon, Nov 20, 2017 at 06:57:47PM +0000, David CARLIER wrote: > On 20 November 2017 at 18:49, Tom Lane <tgl@sss.pgh.pa.us> wrote: > > > David CARLIER <devnexen@gmail.com> writes: > > > - Reducing OpenBSD postfgresql maintainer internal changes bookeeping if > > > those small changes make sense for the PostgreSQL developers. > > > > Hm. The s_lock.c change is surely fine if OpenBSD maintainers say it is. > > > > Not sure about adding Motorola 88K support to s_lock.h ... is anybody > > really still interested in that platform? > > > Yes there is :-) Any chance of a buildfarm http://www.pgbuildfarm.org/ member or two with this architecture? Best, David. -- David Fetter <david(at)fetter(dot)org> http://fetter.org/ Phone: +1 415 235 3778 Remember to vote! Consider donating to Postgres: http://www.postgresql.org/about/donate
David CARLIER <devnexen@gmail.com> writes: > On 20 November 2017 at 18:49, Tom Lane <tgl@sss.pgh.pa.us> wrote: >> OTOH, we still have M68K >> and VAX stanzas in that file, so I suppose it's silly to complain >> about 88K. A bigger issue is that I wonder whether that code has >> ever been tested: it does not look to me like the __asm__ call is >> even syntactically correct. There should be colons in it. > True :-) corrected. Thanks. I still dare to doubt whether you've tested this, because AFAICS the operand numbering is wrong. The "r"(lock) operand is number 3 given these operand declarations, not number 2. Our practice elsewhere in s_lock.h is to use a "+" constraint instead of duplicated operands, and I think that's better style because it avoids any question of whether you're supposed to count duplicated operands. Also, per the comment near s_lock.h line 115, it's important to specify "+m"(*lock) as an output operand so that gcc knows that the asm clobbers *lock. It's possible that "memory" makes that redundant, but I'd just as soon maintain consistency with the well-tested other parts of the file. So I propose the attached patch instead. It would still be a good idea to actually test this ;-) regards, tom lane diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c index 40d8156..247d7b8 100644 --- a/src/backend/storage/lmgr/s_lock.c +++ b/src/backend/storage/lmgr/s_lock.c @@ -251,7 +251,7 @@ static void tas_dummy() { __asm__ __volatile__( -#if defined(__NetBSD__) && defined(__ELF__) +#if (defined(__NetBSD__) || defined(__OpenBSD__)) && defined(__ELF__) /* no underscore for label and % for registers */ "\ .global tas \n\ @@ -276,7 +276,7 @@ _tas: \n\ _success: \n\ moveq #0,d0 \n\ rts \n" -#endif /* __NetBSD__ && __ELF__ */ +#endif /* (__NetBSD__ || __OpenBSD__) && __ELF__ */ ); } #endif /* __m68k__ && !__linux__ */ diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h index 99e1098..35088db 100644 --- a/src/include/storage/s_lock.h +++ b/src/include/storage/s_lock.h @@ -543,6 +543,30 @@ tas(volatile slock_t *lock) #endif /* (__mc68000__ || __m68k__) && __linux__ */ +/* Motorola 88k */ +#if defined(__m88k__) +#define HAS_TEST_AND_SET + +typedef unsigned int slock_t; + +#define TAS(lock) tas(lock) + +static __inline__ int +tas(volatile slock_t *lock) +{ + register slock_t _res = 1; + + __asm__ __volatile__( + " xmem %0, %2, %%r0 \n" +: "+r"(_res), "+m"(*lock) +: "r"(lock) +: "memory"); + return (int) _res; +} + +#endif /* __m88k__ */ + + /* * VAXen -- even multiprocessor ones * (thanks to Tom Ivar Helbekkmo)
I wrote: > I still dare to doubt whether you've tested this, because AFAICS > the operand numbering is wrong. The "r"(lock) operand is number 3 > given these operand declarations, not number 2. Oh, my apologies, scratch that. Evidently I put in the "+m"(*lock) operand and confused myself about what was what. I still think the form I proposed is better style though. regards, tom lane
I m not against, I would go with your final version too. Thanks !
On 20 November 2017 at 22:36, Tom Lane <tgl@sss.pgh.pa.us> wrote:
I wrote:
> I still dare to doubt whether you've tested this, because AFAICS
> the operand numbering is wrong. The "r"(lock) operand is number 3
> given these operand declarations, not number 2.
Oh, my apologies, scratch that. Evidently I put in the "+m"(*lock)
operand and confused myself about what was what.
I still think the form I proposed is better style though.
regards, tom lane
David CARLIER <devnexen@gmail.com> writes: > I m not against, I would go with your final version too. Thanks ! Pushed to all supported branches. regards, tom lane