[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.
Attachments:
0001-PATCH-1-1-Porting-OpenBSD-internal-changes.patchapplication/octet-stream; name=0001-PATCH-1-1-Porting-OpenBSD-internal-changes.patchDownload
From 513f0446486318bc5118f719834c5960ee2a1f8e Mon Sep 17 00:00:00 2001
From: David Carlier <devnexen@gmail.com>
Date: Sun, 19 Nov 2017 08:52:56 +0000
Subject: [PATCH] [PATCH 1/1] Porting OpenBSD internal changes.
Porting to the 88k architecture the locking mechanism.
---
src/backend/storage/lmgr/s_lock.c | 2 +-
src/include/storage/s_lock.h | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 40d8156331..b3e2e34e7e 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\
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 99e109853f..c5ca66b9ec 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -693,6 +693,29 @@ typedef unsigned char slock_t;
do { __asm__ __volatile__("" : : : "memory"); *(lock) = 0; } while (0)
#endif
+#if defined(__m88k__) /* Motorola 88k */
+#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)
+ "0" (_res), "r"(lock)
+ "memory");
+ return (int) _res;
+}
+
+#endif /* __m88k__ */
+
+
#endif /* defined(__GNUC__) || defined(__INTEL_COMPILER) */
--
2.15.0
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.
Show quoted text
regards, tom lane
Attachments:
0001-PATCH-1-1-Porting-OpenBSD-internal-changes.patchapplication/octet-stream; name=0001-PATCH-1-1-Porting-OpenBSD-internal-changes.patchDownload
From 513f0446486318bc5118f719834c5960ee2a1f8e Mon Sep 17 00:00:00 2001
From: David Carlier <devnexen@gmail.com>
Date: Sun, 19 Nov 2017 08:52:56 +0000
Subject: [PATCH] [PATCH 1/1] Porting OpenBSD internal changes.
Porting to the 88k architecture the locking mechanism.
---
src/backend/storage/lmgr/s_lock.c | 2 +-
src/include/storage/s_lock.h | 23 +++++++++++++++++++++++
2 files changed, 24 insertions(+), 1 deletion(-)
diff --git a/src/backend/storage/lmgr/s_lock.c b/src/backend/storage/lmgr/s_lock.c
index 40d8156331..b3e2e34e7e 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\
diff --git a/src/include/storage/s_lock.h b/src/include/storage/s_lock.h
index 99e109853f..c5ca66b9ec 100644
--- a/src/include/storage/s_lock.h
+++ b/src/include/storage/s_lock.h
@@ -693,6 +693,29 @@ typedef unsigned char slock_t;
do { __asm__ __volatile__("" : : : "memory"); *(lock) = 0; } while (0)
#endif
+#if defined(__m88k__) /* Motorola 88k */
+#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)
+: "0" (_res), "r"(lock)
+: "memory");
+ return (int) _res;
+}
+
+#endif /* __m88k__ */
+
+
#endif /* defined(__GNUC__) || defined(__INTEL_COMPILER) */
--
2.15.0
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
Attachments:
absorb-openbsd-fixes-v3.patchtext/x-diff; charset=us-ascii; name=absorb-openbsd-fixes-v3.patchDownload
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:
Show quoted text
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