unconstify equivalent for volatile
I propose to add an equivalent to unconstify() for volatile. When
working on this, I picked the name unvolatize() mostly as a joke, but it
appears it's a real word. Other ideas?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From e2275c87f644b30b92434020adc8e2f7892a8e9c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 18 Feb 2019 16:08:41 +0100
Subject: [PATCH] Add macro to cast away volatile without allowing changes to
underlying type
This adds unvolatize(), which works just like unconstify() but for volatile.
---
contrib/dblink/dblink.c | 2 +-
contrib/hstore_plpython/hstore_plpython.c | 6 +++---
contrib/jsonb_plpython/jsonb_plpython.c | 4 ++--
src/backend/postmaster/pgstat.c | 2 +-
src/backend/storage/ipc/latch.c | 2 +-
src/backend/storage/ipc/pmsignal.c| 2 +-
src/include/c.h | 8 +++-
7 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index d95e6bfa71..402a692191 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -988,7 +988,7 @@ materializeQueryResult(FunctionCallInfo fcinfo,
Assert(rsinfo->returnMode == SFRM_Materialize);
/* initialize storeInfo to empty */
- memset((void *) &sinfo, 0, sizeof(sinfo));
+ memset(unvolatize(storeInfo *, &sinfo), 0, sizeof(sinfo));
sinfo.fcinfo = fcinfo;
PG_TRY();
diff --git a/contrib/hstore_plpython/hstore_plpython.c
b/contrib/hstore_plpython/hstore_plpython.c
index 2f24090ff3..beb201f635 100644
--- a/contrib/hstore_plpython/hstore_plpython.c
+++ b/contrib/hstore_plpython/hstore_plpython.c
@@ -146,7 +146,7 @@ plpython_to_hstore(PG_FUNCTION_ARGS)
int32 buflen;
int32 i;
Pairs *pairs;
- PyObject *items = (PyObject *) items_v;
+ PyObject *items = unvolatize(PyObject *, items_v);
pairs = palloc(pcount * sizeof(*pairs));
@@ -177,14 +177,14 @@ plpython_to_hstore(PG_FUNCTION_ARGS)
pairs[i].isnull = false;
}
}
- Py_DECREF(items_v);
+ Py_DECREF(items);
pcount = hstoreUniquePairs(pairs, pcount, &buflen);
out = hstorePairs(pairs, pcount, buflen);
}
PG_CATCH();
{
- Py_DECREF(items_v);
+ Py_DECREF(unvolatize(PyObject *, items_v));
PG_RE_THROW();
}
PG_END_TRY();
diff --git a/contrib/jsonb_plpython/jsonb_plpython.c
b/contrib/jsonb_plpython/jsonb_plpython.c
index f44d364c97..f860e7bc76 100644
--- a/contrib/jsonb_plpython/jsonb_plpython.c
+++ b/contrib/jsonb_plpython/jsonb_plpython.c
@@ -247,7 +247,7 @@ PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState
**jsonb_state)
Py_ssize_t i;
PyObject *items;
- items = (PyObject *) items_v;
+ items = unvolatize(PyObject *, items_v);
pushJsonbValue(jsonb_state, WJB_BEGIN_OBJECT, NULL);
@@ -279,7 +279,7 @@ PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState
**jsonb_state)
}
PG_CATCH();
{
- Py_DECREF(items_v);
+ Py_DECREF(unvolatize(PyObject *, items_v));
PG_RE_THROW();
}
PG_END_TRY();
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 81c6499251..84ebe03284 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3275,7 +3275,7 @@ pgstat_read_current_status(void)
localentry->backendStatus.st_procpid =
beentry->st_procpid;
if (localentry->backendStatus.st_procpid > 0)
{
- memcpy(&localentry->backendStatus, (char *)
beentry, sizeof(PgBackendStatus));
+ memcpy(&localentry->backendStatus,
unvolatize(PgBackendStatus *, beentry), sizeof(PgBackendStatus));
/*
* strcpy is safe even if the string is
modified concurrently,
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 7da337d11f..fa7d72ef76 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -381,7 +381,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents,
pgsocket sock,
if (wakeEvents & WL_LATCH_SET)
AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
- (Latch *) latch, NULL);
+ unvolatize(Latch *, latch),
NULL);
/* Postmaster-managed callers must handle postmaster death somehow. */
Assert(!IsUnderP
unconstify equivalent for volatile
I propose to add an equivalent to unconstify() for volatile. When
working on this, I picked the name unvolatize() mostly as a joke, but it
appears it's a real word. Other ideas?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From e2275c87f644b30b92434020adc8e2f7892a8e9c Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 18 Feb 2019 16:08:41 +0100
Subject: [PATCH] Add macro to cast away volatile without allowing changes to
underlying type
This adds unvolatize(), which works just like unconstify() but for volatile.
---
contrib/dblink/dblink.c | 2 +-
contrib/hstore_plpython/hstore_plpython.c | 6 +++---
contrib/jsonb_plpython/jsonb_plpython.c | 4 ++--
src/backend/postmaster/pgstat.c | 2 +-
src/backend/storage/ipc/latch.c | 2 +-
src/backend/storage/ipc/pmsignal.c| 2 +-
src/include/c.h | 8 +++-
7 files changed, 16 insertions(+), 10 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index d95e6bfa71..402a692191 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -988,7 +988,7 @@ materializeQueryResult(FunctionCallInfo fcinfo,
Assert(rsinfo->returnMode == SFRM_Materialize);
/* initialize storeInfo to empty */
- memset((void *) &sinfo, 0, sizeof(sinfo));
+ memset(unvolatize(storeInfo *, &sinfo), 0, sizeof(sinfo));
sinfo.fcinfo = fcinfo;
PG_TRY();
diff --git a/contrib/hstore_plpython/hstore_plpython.c
b/contrib/hstore_plpython/hstore_plpython.c
index 2f24090ff3..beb201f635 100644
--- a/contrib/hstore_plpython/hstore_plpython.c
+++ b/contrib/hstore_plpython/hstore_plpython.c
@@ -146,7 +146,7 @@ plpython_to_hstore(PG_FUNCTION_ARGS)
int32 buflen;
int32 i;
Pairs *pairs;
- PyObject *items = (PyObject *) items_v;
+ PyObject *items = unvolatize(PyObject *, items_v);
pairs = palloc(pcount * sizeof(*pairs));
@@ -177,14 +177,14 @@ plpython_to_hstore(PG_FUNCTION_ARGS)
pairs[i].isnull = false;
}
}
- Py_DECREF(items_v);
+ Py_DECREF(items);
pcount = hstoreUniquePairs(pairs, pcount, &buflen);
out = hstorePairs(pairs, pcount, buflen);
}
PG_CATCH();
{
- Py_DECREF(items_v);
+ Py_DECREF(unvolatize(PyObject *, items_v));
PG_RE_THROW();
}
PG_END_TRY();
diff --git a/contrib/jsonb_plpython/jsonb_plpython.c
b/contrib/jsonb_plpython/jsonb_plpython.c
index f44d364c97..f860e7bc76 100644
--- a/contrib/jsonb_plpython/jsonb_plpython.c
+++ b/contrib/jsonb_plpython/jsonb_plpython.c
@@ -247,7 +247,7 @@ PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState
**jsonb_state)
Py_ssize_t i;
PyObject *items;
- items = (PyObject *) items_v;
+ items = unvolatize(PyObject *, items_v);
pushJsonbValue(jsonb_state, WJB_BEGIN_OBJECT, NULL);
@@ -279,7 +279,7 @@ PLyMapping_ToJsonbValue(PyObject *obj, JsonbParseState
**jsonb_state)
}
PG_CATCH();
{
- Py_DECREF(items_v);
+ Py_DECREF(unvolatize(PyObject *, items_v));
PG_RE_THROW();
}
PG_END_TRY();
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 81c6499251..84ebe03284 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3275,7 +3275,7 @@ pgstat_read_current_status(void)
localentry->backendStatus.st_procpid =
beentry->st_procpid;
if (localentry->backendStatus.st_procpid > 0)
{
- memcpy(&localentry->backendStatus, (char *)
beentry, sizeof(PgBackendStatus));
+ memcpy(&localentry->backendStatus,
unvolatize(PgBackendStatus *, beentry), sizeof(PgBackendStatus));
/*
* strcpy is safe even if the string is
modified concurrently,
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 7da337d11f..fa7d72ef76 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -381,7 +381,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents,
pgsocket sock,
if (wakeEvents & WL_LATCH_SET)
AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
- (Latch *) latch, NULL);
+ unvolatize(Latch *, latch),
NULL);
/* Postmaster-managed callers must handle postmaster death somehow. */
Assert(!IsUnderP
Re: unconstify equivalent for volatile
On 2019-02-25 09:29, Peter Eisentraut wrote: > On 2019-02-22 21:31, Andres Freund wrote: >> On 2019-02-22 12:38:35 +0100, Peter Eisentraut wrote: >>> On 2019-02-19 18:02, Andres Freund wrote: But even if we were to decide we'd want to keep a volatile in SetLatch() - which I think really would only serve to hide bugs - that'd not mean it's a good idea to keep it on all the other functions in latch.c. > >> Right. But we should ever look/write into the contents of a latch >> outside of latch.c, so I don't think that'd really be a problem, even if >> we relied on volatiles. > > So how about this patch? committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: unconstify equivalent for volatile
On 2019-02-18 16:42, Andres Freund wrote:
> On February 18, 2019 7:39:25 AM PST, Peter Eisentraut
> wrote:
>> I propose to add an equivalent to unconstify() for volatile. When
>> working on this, I picked the name unvolatize() mostly as a joke, but
>> it
>> appears it's a real word. Other ideas?
>
> Shouldn't we just remove just about all those use of volatile? Basically the
> only valid use these days is on sigsetjmp call sites.
So, after some recent cleanups and another one attached here, we're now
down to 1.5 uses of this. (0.5 because the hunk in pmsignal.c doesn't
have a cast right now, but it would need one if s/MemSet/memset/.)
Those seem legitimate.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 89aada5bdc9ef5c0b0125a72eb9d5494bf360282 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Fri, 15 Mar 2019 08:46:06 +0100
Subject: [PATCH v2 01/10] Initialize structure at declaration
Avoids extra memset call and cast.
---
contrib/dblink/dblink.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index d95e6bfa71..d35e5ba3d8 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -982,13 +982,11 @@ materializeQueryResult(FunctionCallInfo fcinfo,
{
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
PGresult *volatile res = NULL;
- volatile storeInfo sinfo;
+ volatile storeInfo sinfo = {0};
/* prepTuplestoreResult must have been called previously */
Assert(rsinfo->returnMode == SFRM_Materialize);
- /* initialize storeInfo to empty */
- memset((void *) &sinfo, 0, sizeof(sinfo));
sinfo.fcinfo = fcinfo;
PG_TRY();
base-commit: 53680c116ce8c501e4081332d32ba0e93aa1aaa2
--
2.21.0
From 81a4e16fd06e8571d08b1d564e0c6ba1cb647476 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 18 Feb 2019 16:08:41 +0100
Subject: [PATCH v2 02/10] Add macro to cast away volatile without allowing
changes to underlying type
This adds unvolatize(), which works just like unconstify() but for volatile.
---
src/backend/postmaster/pgstat.c| 2 +-
src/backend/storage/ipc/pmsignal.c | 2 +-
src/include/c.h| 8 +++-
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 2fbfadd9f0..2a8472b91a 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3311,7 +3311,7 @@ pgstat_read_current_status(void)
localentry->backendStatus.st_procpid =
beentry->st_procpid;
if (localentry->backendStatus.st_procpid > 0)
{
- memcpy(&localentry->backendStatus, (char *)
beentry, sizeof(PgBackendStatus));
+ memcpy(&localentry->backendStatus,
unvolatize(PgBackendStatus *, beentry), sizeof(PgBackendStatus));
/*
* strcpy is safe even if the string is
modified concurrently,
diff --git a/src/backend/storage/ipc/pmsignal.c
b/src/backend/storage/ipc/pmsignal.c
index d707993bf6..48f4311464 100644
--- a/src/backend/storage/ipc/pmsignal.c
+++ b/src/backend/storage/ipc/pmsignal.c
@@ -134,7 +134,7 @@ PMSignalShmemInit(void)
if (!found)
{
- MemSet(PMSignalState, 0, PMSignalShmemSize());
+ MemSet(unvolatize(PMSignalData *, PMSignalState), 0,
PMSignalShmemSize());
PMSignalState->num_child_flags = MaxLivePostmasterChildren();
}
}
diff --git a/src/include/c.h b/src/include/c.h
index 658be50e0d..33c9518195 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -1122,7 +1122,7 @@ typedef union PGAlignedXLogBlock
#endif
/*
- * Macro that allows to cast constness away from an expression, but doesn't
+ * Macro that allows to cast constness and volatile away from an expression,
but doesn't
* allow changing the underlying type. Enforcement of the latter
* currently only works for gcc like compilers.
*
@@ -1141,9 +1141,15 @@ typedef union PGAlignedXLogBlock
(StaticAssertExpr(__builtin_types_compatible_p(__typeof(expr), const
underlying_type), \
"wrong cast"), \
(underlying_type) (expr))
+#define unvolatize(underlying_type, expr) \
+ (StaticAssertExpr(__builtin_types_compatible_p(__typeof(expr), volatile
underlying_type), \
+ "wrong cast"), \
+(underlying_type) (expr))
#else
#define unconstify(underlying_type, expr) \
((underlying_type) (expr))
+#define unvolatize(underlying_type, expr) \
+ ((underlying_type) (expr))
#endif
/*
--
2.21.0
Re: unconstify equivalent for volatile
Hi, On February 18, 2019 7:39:25 AM PST, Peter Eisentraut wrote: >I propose to add an equivalent to unconstify() for volatile. When >working on this, I picked the name unvolatize() mostly as a joke, but >it >appears it's a real word. Other ideas? Shouldn't we just remove just about all those use of volatile? Basically the only valid use these days is on sigsetjmp call sites. Andres Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: unconstify equivalent for volatile
Peter Eisentraut writes: > I propose to add an equivalent to unconstify() for volatile. When > working on this, I picked the name unvolatize() mostly as a joke, but it > appears it's a real word. Other ideas? Umm ... wouldn't this amount to papering over actual bugs? I can think of legitimate reasons to cast away const, but casting away volatile seems pretty dangerous, and not something to encourage by making it notationally easy. regards, tom lane
Re: unconstify equivalent for volatile
Hi, On 2019-02-18 10:43:50 -0500, Tom Lane wrote: > Peter Eisentraut writes: > > I propose to add an equivalent to unconstify() for volatile. When > > working on this, I picked the name unvolatize() mostly as a joke, but it > > appears it's a real word. Other ideas? > > Umm ... wouldn't this amount to papering over actual bugs? I can > think of legitimate reasons to cast away const, but casting away > volatile seems pretty dangerous, and not something to encourage > by making it notationally easy. Most of those seem to be cases where volatile was just to make sigsetjmp safe. There's plently legitimate cases where we need to cast volatile away in those, e.g. because the variable needs to be passed to memcpy. Greetings, Andres Freund
Re: unconstify equivalent for volatile
On 18/02/2019 16:43, Tom Lane wrote: > Peter Eisentraut writes: >> I propose to add an equivalent to unconstify() for volatile. When >> working on this, I picked the name unvolatize() mostly as a joke, but it >> appears it's a real word. Other ideas? > > Umm ... wouldn't this amount to papering over actual bugs? I can > think of legitimate reasons to cast away const, but casting away > volatile seems pretty dangerous, and not something to encourage > by making it notationally easy. > I'd argue that it's not making it easier to do but rather easier to spot (vs normal type casting) which is IMO a good thing from patch review perspective. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: unconstify equivalent for volatile
Petr Jelinek writes: > On 18/02/2019 16:43, Tom Lane wrote: >> Umm ... wouldn't this amount to papering over actual bugs? > I'd argue that it's not making it easier to do but rather easier to spot > (vs normal type casting) which is IMO a good thing from patch review > perspective. Yeah, fair point. As Peter noted about unconstify, these could be viewed as TODO markers. regards, tom lane
Re: unconstify equivalent for volatile
Hi,
On 2019-02-18 16:39:25 +0100, Peter Eisentraut wrote:
> diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
> index 7da337d11f..fa7d72ef76 100644
> --- a/src/backend/storage/ipc/latch.c
> +++ b/src/backend/storage/ipc/latch.c
> @@ -381,7 +381,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents,
> pgsocket sock,
>
> if (wakeEvents & WL_LATCH_SET)
> AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
> - (Latch *) latch, NULL);
> + unvolatize(Latch *, latch),
> NULL);
>
> /* Postmaster-managed callers must handle postmaster death somehow. */
> Assert(!IsUnderPostmaster ||
ISTM this one should rather be solved by removing all volatiles from
latch.[ch]. As that's a cross-process concern we can't rely on it
anyway (and have placed barriers a few years back to allay concerns /
bugs due to reordering).
> diff --git a/src/backend/storage/ipc/pmsignal.c
> b/src/backend/storage/ipc/pmsignal.c
> index d707993bf6..48f4311464 100644
> --- a/src/backend/storage/ipc/pmsignal.c
> +++ b/src/backend/storage/ipc/pmsignal.c
> @@ -134,7 +134,7 @@ PMSignalShmemInit(void)
>
> if (!found)
> {
> - MemSet(PMSignalState, 0, PMSignalShmemSize());
> + MemSet(unvolatize(PMSignalData *, PMSignalState), 0,
> PMSignalShmemSize());
> PMSignalState->num_child_flags = MaxLivePostmasterChildren();
> }
> }
Same. Did you put an type assertion into MemSet(), or how did you
discover this one as needing to be changed?
.oO(We really ought to remove MemSet()).
Re: unconstify equivalent for volatile
On 2019-02-18 21:25, Andres Freund wrote:
> ISTM this one should rather be solved by removing all volatiles from
> latch.[ch]. As that's a cross-process concern we can't rely on it
> anyway (and have placed barriers a few years back to allay concerns /
> bugs due to reordering).
Aren't the volatiles there so that Latch variables can be set from
signal handlers?
>> diff --git a/src/backend/storage/ipc/pmsignal.c
>> b/src/backend/storage/ipc/pmsignal.c
>> index d707993bf6..48f4311464 100644
>> --- a/src/backend/storage/ipc/pmsignal.c
>> +++ b/src/backend/storage/ipc/pmsignal.c
>> @@ -134,7 +134,7 @@ PMSignalShmemInit(void)
>>
>> if (!found)
>> {
>> -MemSet(PMSignalState, 0, PMSignalShmemSize());
>> +MemSet(unvolatize(PMSignalData *, PMSignalState), 0,
>> PMSignalShmemSize());
>> PMSignalState->num_child_flags = MaxLivePostmasterChildren();
>> }
>> }
>
> Same. Did you put an type assertion into MemSet(), or how did you
> discover this one as needing to be changed?
Build with -Wcast-qual, which warns for this because MemSet() does a
(void *) cast.
> .oO(We really ought to remove MemSet()).
yeah
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: unconstify equivalent for volatile
Hi, On February 19, 2019 7:00:58 AM PST, Peter Eisentraut wrote: >On 2019-02-18 21:25, Andres Freund wrote: >> ISTM this one should rather be solved by removing all volatiles from >> latch.[ch]. As that's a cross-process concern we can't rely on it >> anyway (and have placed barriers a few years back to allay concerns / >> bugs due to reordering). > >Aren't the volatiles there so that Latch variables can be set from >signal handlers? Why would they be required, given we have an explicit compiler & memory barrier? That forces the compiler to spill the writes to memory already, even in a signal handler. And conversely the reading side is similarly forced - if not we have a bug in multi core systems - to read the variable from memory after a barrier. The real reason why variables commonly need to be volatile when used in signal handlers is not the signal handler side, but the normal code flow side. Without using explicit care the variable's value can be "cached"in a register, and a change not noticed. But as volatiles aren't sufficient for cross process safety, latches need more anyway. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: unconstify equivalent for volatile
Andres Freund writes: > The real reason why variables commonly need to be volatile when used in > signal handlers is not the signal handler side, but the normal code flow > side. Yeah, exactly. You have not explained why it'd be safe to ignore that. regards, tom lane
Re: unconstify equivalent for volatile
Hi, On 2019-02-19 11:48:16 -0500, Tom Lane wrote: > Andres Freund writes: > > The real reason why variables commonly need to be volatile when used in > > signal handlers is not the signal handler side, but the normal code flow > > side. > > Yeah, exactly. You have not explained why it'd be safe to ignore that. Because SetLatch() is careful to have a pg_memory_barrier() before touching shared state and conversely so are ResetLatch() (and WaitEventSetWait(), which already has no volatiles). And if we've got this wrong they aren't safe for shared latches, because volatiles don't enforce meaningful ordering on weakly ordered architectures. But even if we were to decide we'd want to keep a volatile in SetLatch() - which I think really would only serve to hide bugs - that'd not mean it's a good idea to keep it on all the other functions in latch.c. Greetings, Andres Freund
Re: unconstify equivalent for volatile
On 2019-02-19 18:02, Andres Freund wrote: > Because SetLatch() is careful to have a pg_memory_barrier() before > touching shared state and conversely so are ResetLatch() (and > WaitEventSetWait(), which already has no volatiles). And if we've got > this wrong they aren't safe for shared latches, because volatiles don't > enforce meaningful ordering on weakly ordered architectures. That makes sense. > But even if we were to decide we'd want to keep a volatile in SetLatch() > - which I think really would only serve to hide bugs - that'd not mean > it's a good idea to keep it on all the other functions in latch.c. What is even the meaning of having a volatile Latch * argument on a function when the actual latch variable (MyLatch) isn't volatile? That would just enforce certain constraints on the compiler inside that function but not on the overall program, right? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: unconstify equivalent for volatile
Hi, On 2019-02-22 12:38:35 +0100, Peter Eisentraut wrote: > On 2019-02-19 18:02, Andres Freund wrote: > > But even if we were to decide we'd want to keep a volatile in SetLatch() > > - which I think really would only serve to hide bugs - that'd not mean > > it's a good idea to keep it on all the other functions in latch.c. > > What is even the meaning of having a volatile Latch * argument on a > function when the actual latch variable (MyLatch) isn't volatile? That > would just enforce certain constraints on the compiler inside that > function but not on the overall program, right? Right. But we should ever look/write into the contents of a latch outside of latch.c, so I don't think that'd really be a problem, even if we relied on volatiles. Greetings, Andres Freund
Re: unconstify equivalent for volatile
On 2019-02-22 21:31, Andres Freund wrote:
> On 2019-02-22 12:38:35 +0100, Peter Eisentraut wrote:
>> On 2019-02-19 18:02, Andres Freund wrote:
>>> But even if we were to decide we'd want to keep a volatile in SetLatch()
>>> - which I think really would only serve to hide bugs - that'd not mean
>>> it's a good idea to keep it on all the other functions in latch.c.
> Right. But we should ever look/write into the contents of a latch
> outside of latch.c, so I don't think that'd really be a problem, even if
> we relied on volatiles.
So how about this patch?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 8afb8bc4e01e32be532fefccfd39f4d7294d2f32 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 25 Feb 2019 09:24:15 +0100
Subject: [PATCH] Remove volatile from latch API
This was no longer useful since the latch functions use memory
barriers already, which are also compiler barriers, and volatile does
not help with cross-process access.
Discussion:
https://www.postgresql.org/message-id/flat/20190218202511.qsfpuj5sy4dbezcw%40alap3.anarazel.de#18783c27d73e9e40009c82f6e0df0974
---
src/backend/storage/ipc/latch.c | 18 +-
src/include/storage/latch.h | 16
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 7da337d11f..59fa917ae0 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -225,7 +225,7 @@ InitializeLatchSupport(void)
* Initialize a process-local latch.
*/
void
-InitLatch(volatile Latch *latch)
+InitLatch(Latch *latch)
{
latch->is_set = false;
latch->owner_pid = MyProcPid;
@@ -257,7 +257,7 @@ InitLatch(volatile Latch *latch)
* process references to postmaster-private latches or WaitEventSets.
*/
void
-InitSharedLatch(volatile Latch *latch)
+InitSharedLatch(Latch *latch)
{
#ifdef WIN32
SECURITY_ATTRIBUTES sa;
@@ -293,7 +293,7 @@ InitSharedLatch(volatile Latch *latch)
* as shared latches use SIGUSR1 for inter-process communication.
*/
void
-OwnLatch(volatile Latch *latch)
+OwnLatch(Latch *latch)
{
/* Sanity checks */
Assert(latch->is_shared);
@@ -313,7 +313,7 @@ OwnLatch(volatile Latch *latch)
* Disown a shared latch currently owned by the current process.
*/
void
-DisownLatch(volatile Latch *latch)
+DisownLatch(Latch *latch)
{
Assert(latch->is_shared);
Assert(latch->owner_pid == MyProcPid);
@@ -341,7 +341,7 @@ DisownLatch(volatile Latch *latch)
* we return all of them in one call, but we will return at least one.
*/
int
-WaitLatch(volatile Latch *latch, int wakeEvents, long timeout,
+WaitLatch(Latch *latch, int wakeEvents, long timeout,
uint32 wait_event_info)
{
return WaitLatchOrSocket(latch, wakeEvents, PGINVALID_SOCKET, timeout,
@@ -366,7 +366,7 @@ WaitLatch(volatile Latch *latch, int wakeEvents, long
timeout,
* WaitEventSet instead; that's more efficient.
*/
int
-WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
+WaitLatchOrSocket(Latch *latch, int wakeEvents, pgsocket sock,
long timeout, uint32 wait_event_info)
{
int ret = 0;
@@ -381,7 +381,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents,
pgsocket sock,
if (wakeEvents & WL_LATCH_SET)
AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
- (Latch *) latch, NULL);
+ latch, NULL);
/* Postmaster-managed callers must handle postmaster death somehow. */
Assert(!IsUnderPostmaster ||
@@ -433,7 +433,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents,
pgsocket sock,
* throwing an error is not a good idea.
*/
void
-SetLatch(volatile Latch *latch)
+SetLatch(Latch *latch)
{
#ifndef WIN32
pid_t owner_pid;
@@ -516,7 +516,7 @@ SetLatch(volatile Latch *latch)
* the latch is set again before the WaitLatch call.
*/
void
-ResetLatch(volatile Latch *latch)
+ResetLatch(Latch *latch)
{
/* Only the owner should reset the latch */
Assert(latch->owner_pid == MyProcPid);
diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
index 5b48709c8a..fc995819d3 100644
--- a/src/include/storage/latch.h
+++ b/src/include/storage/latch.h
@@ -156,12 +156,12 @@ typedef struct WaitEventSet WaitEventSet;
* prototypes for functions in latch.c
*/
extern void InitializeLatchSupport(void);
-extern void InitLatch(volatile Latch *latch);
-extern void InitSharedLatch(volatile Latch *latch);
-extern void OwnLatch(volatile Latch *latch);
-extern void DisownLatch(volatile Latch *latch);
-extern void SetLatch(volatile Latch *latch);
-extern void ResetLatch(volatile Latch *latch);
+extern void InitLatch(Latch *latch);
+exte
Re: unconstify equivalent for volatile
On 2019-02-19 18:02, Andres Freund wrote: > Because SetLatch() is careful to have a pg_memory_barrier() before > touching shared state and conversely so are ResetLatch() (and > WaitEventSetWait(), which already has no volatiles). And if we've got > this wrong they aren't safe for shared latches, because volatiles don't > enforce meaningful ordering on weakly ordered architectures. That makes sense. > But even if we were to decide we'd want to keep a volatile in SetLatch() > - which I think really would only serve to hide bugs - that'd not mean > it's a good idea to keep it on all the other functions in latch.c. What is even the meaning of having a volatile Latch * argument on a function when the actual latch variable (MyLatch) isn't volatile? That would just enforce certain constraints on the compiler inside that function but not on the overall program, right? -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: unconstify equivalent for volatile
Hi, On 2019-02-22 12:38:35 +0100, Peter Eisentraut wrote: > On 2019-02-19 18:02, Andres Freund wrote: > > But even if we were to decide we'd want to keep a volatile in SetLatch() > > - which I think really would only serve to hide bugs - that'd not mean > > it's a good idea to keep it on all the other functions in latch.c. > > What is even the meaning of having a volatile Latch * argument on a > function when the actual latch variable (MyLatch) isn't volatile? That > would just enforce certain constraints on the compiler inside that > function but not on the overall program, right? Right. But we should ever look/write into the contents of a latch outside of latch.c, so I don't think that'd really be a problem, even if we relied on volatiles. Greetings, Andres Freund
Re: unconstify equivalent for volatile
On 2019-02-22 21:31, Andres Freund wrote:
> On 2019-02-22 12:38:35 +0100, Peter Eisentraut wrote:
>> On 2019-02-19 18:02, Andres Freund wrote:
>>> But even if we were to decide we'd want to keep a volatile in SetLatch()
>>> - which I think really would only serve to hide bugs - that'd not mean
>>> it's a good idea to keep it on all the other functions in latch.c.
> Right. But we should ever look/write into the contents of a latch
> outside of latch.c, so I don't think that'd really be a problem, even if
> we relied on volatiles.
So how about this patch?
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 8afb8bc4e01e32be532fefccfd39f4d7294d2f32 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 25 Feb 2019 09:24:15 +0100
Subject: [PATCH] Remove volatile from latch API
This was no longer useful since the latch functions use memory
barriers already, which are also compiler barriers, and volatile does
not help with cross-process access.
Discussion:
https://www.postgresql.org/message-id/flat/20190218202511.qsfpuj5sy4dbezcw%40alap3.anarazel.de#18783c27d73e9e40009c82f6e0df0974
---
src/backend/storage/ipc/latch.c | 18 +-
src/include/storage/latch.h | 16
2 files changed, 17 insertions(+), 17 deletions(-)
diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
index 7da337d11f..59fa917ae0 100644
--- a/src/backend/storage/ipc/latch.c
+++ b/src/backend/storage/ipc/latch.c
@@ -225,7 +225,7 @@ InitializeLatchSupport(void)
* Initialize a process-local latch.
*/
void
-InitLatch(volatile Latch *latch)
+InitLatch(Latch *latch)
{
latch->is_set = false;
latch->owner_pid = MyProcPid;
@@ -257,7 +257,7 @@ InitLatch(volatile Latch *latch)
* process references to postmaster-private latches or WaitEventSets.
*/
void
-InitSharedLatch(volatile Latch *latch)
+InitSharedLatch(Latch *latch)
{
#ifdef WIN32
SECURITY_ATTRIBUTES sa;
@@ -293,7 +293,7 @@ InitSharedLatch(volatile Latch *latch)
* as shared latches use SIGUSR1 for inter-process communication.
*/
void
-OwnLatch(volatile Latch *latch)
+OwnLatch(Latch *latch)
{
/* Sanity checks */
Assert(latch->is_shared);
@@ -313,7 +313,7 @@ OwnLatch(volatile Latch *latch)
* Disown a shared latch currently owned by the current process.
*/
void
-DisownLatch(volatile Latch *latch)
+DisownLatch(Latch *latch)
{
Assert(latch->is_shared);
Assert(latch->owner_pid == MyProcPid);
@@ -341,7 +341,7 @@ DisownLatch(volatile Latch *latch)
* we return all of them in one call, but we will return at least one.
*/
int
-WaitLatch(volatile Latch *latch, int wakeEvents, long timeout,
+WaitLatch(Latch *latch, int wakeEvents, long timeout,
uint32 wait_event_info)
{
return WaitLatchOrSocket(latch, wakeEvents, PGINVALID_SOCKET, timeout,
@@ -366,7 +366,7 @@ WaitLatch(volatile Latch *latch, int wakeEvents, long
timeout,
* WaitEventSet instead; that's more efficient.
*/
int
-WaitLatchOrSocket(volatile Latch *latch, int wakeEvents, pgsocket sock,
+WaitLatchOrSocket(Latch *latch, int wakeEvents, pgsocket sock,
long timeout, uint32 wait_event_info)
{
int ret = 0;
@@ -381,7 +381,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents,
pgsocket sock,
if (wakeEvents & WL_LATCH_SET)
AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
- (Latch *) latch, NULL);
+ latch, NULL);
/* Postmaster-managed callers must handle postmaster death somehow. */
Assert(!IsUnderPostmaster ||
@@ -433,7 +433,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents,
pgsocket sock,
* throwing an error is not a good idea.
*/
void
-SetLatch(volatile Latch *latch)
+SetLatch(Latch *latch)
{
#ifndef WIN32
pid_t owner_pid;
@@ -516,7 +516,7 @@ SetLatch(volatile Latch *latch)
* the latch is set again before the WaitLatch call.
*/
void
-ResetLatch(volatile Latch *latch)
+ResetLatch(Latch *latch)
{
/* Only the owner should reset the latch */
Assert(latch->owner_pid == MyProcPid);
diff --git a/src/include/storage/latch.h b/src/include/storage/latch.h
index 5b48709c8a..fc995819d3 100644
--- a/src/include/storage/latch.h
+++ b/src/include/storage/latch.h
@@ -156,12 +156,12 @@ typedef struct WaitEventSet WaitEventSet;
* prototypes for functions in latch.c
*/
extern void InitializeLatchSupport(void);
-extern void InitLatch(volatile Latch *latch);
-extern void InitSharedLatch(volatile Latch *latch);
-extern void OwnLatch(volatile Latch *latch);
-extern void DisownLatch(volatile Latch *latch);
-extern void SetLatch(volatile Latch *latch);
-extern void ResetLatch(volatile Latch *latch);
+extern void InitLatch(Latch *latch);
+exte
Re: unconstify equivalent for volatile
On 2019-02-25 09:29, Peter Eisentraut wrote: > On 2019-02-22 21:31, Andres Freund wrote: >> On 2019-02-22 12:38:35 +0100, Peter Eisentraut wrote: >>> On 2019-02-19 18:02, Andres Freund wrote: But even if we were to decide we'd want to keep a volatile in SetLatch() - which I think really would only serve to hide bugs - that'd not mean it's a good idea to keep it on all the other functions in latch.c. > >> Right. But we should ever look/write into the contents of a latch >> outside of latch.c, so I don't think that'd really be a problem, even if >> we relied on volatiles. > > So how about this patch? committed -- Peter Eisentraut http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: unconstify equivalent for volatile
On 2019-02-18 16:42, Andres Freund wrote:
> On February 18, 2019 7:39:25 AM PST, Peter Eisentraut
> wrote:
>> I propose to add an equivalent to unconstify() for volatile. When
>> working on this, I picked the name unvolatize() mostly as a joke, but
>> it
>> appears it's a real word. Other ideas?
>
> Shouldn't we just remove just about all those use of volatile? Basically the
> only valid use these days is on sigsetjmp call sites.
So, after some recent cleanups and another one attached here, we're now
down to 1.5 uses of this. (0.5 because the hunk in pmsignal.c doesn't
have a cast right now, but it would need one if s/MemSet/memset/.)
Those seem legitimate.
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
From 89aada5bdc9ef5c0b0125a72eb9d5494bf360282 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Fri, 15 Mar 2019 08:46:06 +0100
Subject: [PATCH v2 01/10] Initialize structure at declaration
Avoids extra memset call and cast.
---
contrib/dblink/dblink.c | 4 +---
1 file changed, 1 insertion(+), 3 deletions(-)
diff --git a/contrib/dblink/dblink.c b/contrib/dblink/dblink.c
index d95e6bfa71..d35e5ba3d8 100644
--- a/contrib/dblink/dblink.c
+++ b/contrib/dblink/dblink.c
@@ -982,13 +982,11 @@ materializeQueryResult(FunctionCallInfo fcinfo,
{
ReturnSetInfo *rsinfo = (ReturnSetInfo *) fcinfo->resultinfo;
PGresult *volatile res = NULL;
- volatile storeInfo sinfo;
+ volatile storeInfo sinfo = {0};
/* prepTuplestoreResult must have been called previously */
Assert(rsinfo->returnMode == SFRM_Materialize);
- /* initialize storeInfo to empty */
- memset((void *) &sinfo, 0, sizeof(sinfo));
sinfo.fcinfo = fcinfo;
PG_TRY();
base-commit: 53680c116ce8c501e4081332d32ba0e93aa1aaa2
--
2.21.0
From 81a4e16fd06e8571d08b1d564e0c6ba1cb647476 Mon Sep 17 00:00:00 2001
From: Peter Eisentraut
Date: Mon, 18 Feb 2019 16:08:41 +0100
Subject: [PATCH v2 02/10] Add macro to cast away volatile without allowing
changes to underlying type
This adds unvolatize(), which works just like unconstify() but for volatile.
---
src/backend/postmaster/pgstat.c| 2 +-
src/backend/storage/ipc/pmsignal.c | 2 +-
src/include/c.h| 8 +++-
3 files changed, 9 insertions(+), 3 deletions(-)
diff --git a/src/backend/postmaster/pgstat.c b/src/backend/postmaster/pgstat.c
index 2fbfadd9f0..2a8472b91a 100644
--- a/src/backend/postmaster/pgstat.c
+++ b/src/backend/postmaster/pgstat.c
@@ -3311,7 +3311,7 @@ pgstat_read_current_status(void)
localentry->backendStatus.st_procpid =
beentry->st_procpid;
if (localentry->backendStatus.st_procpid > 0)
{
- memcpy(&localentry->backendStatus, (char *)
beentry, sizeof(PgBackendStatus));
+ memcpy(&localentry->backendStatus,
unvolatize(PgBackendStatus *, beentry), sizeof(PgBackendStatus));
/*
* strcpy is safe even if the string is
modified concurrently,
diff --git a/src/backend/storage/ipc/pmsignal.c
b/src/backend/storage/ipc/pmsignal.c
index d707993bf6..48f4311464 100644
--- a/src/backend/storage/ipc/pmsignal.c
+++ b/src/backend/storage/ipc/pmsignal.c
@@ -134,7 +134,7 @@ PMSignalShmemInit(void)
if (!found)
{
- MemSet(PMSignalState, 0, PMSignalShmemSize());
+ MemSet(unvolatize(PMSignalData *, PMSignalState), 0,
PMSignalShmemSize());
PMSignalState->num_child_flags = MaxLivePostmasterChildren();
}
}
diff --git a/src/include/c.h b/src/include/c.h
index 658be50e0d..33c9518195 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -1122,7 +1122,7 @@ typedef union PGAlignedXLogBlock
#endif
/*
- * Macro that allows to cast constness away from an expression, but doesn't
+ * Macro that allows to cast constness and volatile away from an expression,
but doesn't
* allow changing the underlying type. Enforcement of the latter
* currently only works for gcc like compilers.
*
@@ -1141,9 +1141,15 @@ typedef union PGAlignedXLogBlock
(StaticAssertExpr(__builtin_types_compatible_p(__typeof(expr), const
underlying_type), \
"wrong cast"), \
(underlying_type) (expr))
+#define unvolatize(underlying_type, expr) \
+ (StaticAssertExpr(__builtin_types_compatible_p(__typeof(expr), volatile
underlying_type), \
+ "wrong cast"), \
+(underlying_type) (expr))
#else
#define unconstify(underlying_type, expr) \
((underlying_type) (expr))
+#define unvolatize(underlying_type, expr) \
+ ((underlying_type) (expr))
#endif
/*
--
2.21.0
Re: unconstify equivalent for volatile
Hi, On February 18, 2019 7:39:25 AM PST, Peter Eisentraut wrote: >I propose to add an equivalent to unconstify() for volatile. When >working on this, I picked the name unvolatize() mostly as a joke, but >it >appears it's a real word. Other ideas? Shouldn't we just remove just about all those use of volatile? Basically the only valid use these days is on sigsetjmp call sites. Andres Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: unconstify equivalent for volatile
Peter Eisentraut writes: > I propose to add an equivalent to unconstify() for volatile. When > working on this, I picked the name unvolatize() mostly as a joke, but it > appears it's a real word. Other ideas? Umm ... wouldn't this amount to papering over actual bugs? I can think of legitimate reasons to cast away const, but casting away volatile seems pretty dangerous, and not something to encourage by making it notationally easy. regards, tom lane
Re: unconstify equivalent for volatile
Hi, On 2019-02-18 10:43:50 -0500, Tom Lane wrote: > Peter Eisentraut writes: > > I propose to add an equivalent to unconstify() for volatile. When > > working on this, I picked the name unvolatize() mostly as a joke, but it > > appears it's a real word. Other ideas? > > Umm ... wouldn't this amount to papering over actual bugs? I can > think of legitimate reasons to cast away const, but casting away > volatile seems pretty dangerous, and not something to encourage > by making it notationally easy. Most of those seem to be cases where volatile was just to make sigsetjmp safe. There's plently legitimate cases where we need to cast volatile away in those, e.g. because the variable needs to be passed to memcpy. Greetings, Andres Freund
Re: unconstify equivalent for volatile
On 18/02/2019 16:43, Tom Lane wrote: > Peter Eisentraut writes: >> I propose to add an equivalent to unconstify() for volatile. When >> working on this, I picked the name unvolatize() mostly as a joke, but it >> appears it's a real word. Other ideas? > > Umm ... wouldn't this amount to papering over actual bugs? I can > think of legitimate reasons to cast away const, but casting away > volatile seems pretty dangerous, and not something to encourage > by making it notationally easy. > I'd argue that it's not making it easier to do but rather easier to spot (vs normal type casting) which is IMO a good thing from patch review perspective. -- Petr Jelinek http://www.2ndQuadrant.com/ PostgreSQL Development, 24x7 Support, Training & Services
Re: unconstify equivalent for volatile
Petr Jelinek writes: > On 18/02/2019 16:43, Tom Lane wrote: >> Umm ... wouldn't this amount to papering over actual bugs? > I'd argue that it's not making it easier to do but rather easier to spot > (vs normal type casting) which is IMO a good thing from patch review > perspective. Yeah, fair point. As Peter noted about unconstify, these could be viewed as TODO markers. regards, tom lane
Re: unconstify equivalent for volatile
Hi,
On 2019-02-18 16:39:25 +0100, Peter Eisentraut wrote:
> diff --git a/src/backend/storage/ipc/latch.c b/src/backend/storage/ipc/latch.c
> index 7da337d11f..fa7d72ef76 100644
> --- a/src/backend/storage/ipc/latch.c
> +++ b/src/backend/storage/ipc/latch.c
> @@ -381,7 +381,7 @@ WaitLatchOrSocket(volatile Latch *latch, int wakeEvents,
> pgsocket sock,
>
> if (wakeEvents & WL_LATCH_SET)
> AddWaitEventToSet(set, WL_LATCH_SET, PGINVALID_SOCKET,
> - (Latch *) latch, NULL);
> + unvolatize(Latch *, latch),
> NULL);
>
> /* Postmaster-managed callers must handle postmaster death somehow. */
> Assert(!IsUnderPostmaster ||
ISTM this one should rather be solved by removing all volatiles from
latch.[ch]. As that's a cross-process concern we can't rely on it
anyway (and have placed barriers a few years back to allay concerns /
bugs due to reordering).
> diff --git a/src/backend/storage/ipc/pmsignal.c
> b/src/backend/storage/ipc/pmsignal.c
> index d707993bf6..48f4311464 100644
> --- a/src/backend/storage/ipc/pmsignal.c
> +++ b/src/backend/storage/ipc/pmsignal.c
> @@ -134,7 +134,7 @@ PMSignalShmemInit(void)
>
> if (!found)
> {
> - MemSet(PMSignalState, 0, PMSignalShmemSize());
> + MemSet(unvolatize(PMSignalData *, PMSignalState), 0,
> PMSignalShmemSize());
> PMSignalState->num_child_flags = MaxLivePostmasterChildren();
> }
> }
Same. Did you put an type assertion into MemSet(), or how did you
discover this one as needing to be changed?
.oO(We really ought to remove MemSet()).
Re: unconstify equivalent for volatile
On 2019-02-18 21:25, Andres Freund wrote:
> ISTM this one should rather be solved by removing all volatiles from
> latch.[ch]. As that's a cross-process concern we can't rely on it
> anyway (and have placed barriers a few years back to allay concerns /
> bugs due to reordering).
Aren't the volatiles there so that Latch variables can be set from
signal handlers?
>> diff --git a/src/backend/storage/ipc/pmsignal.c
>> b/src/backend/storage/ipc/pmsignal.c
>> index d707993bf6..48f4311464 100644
>> --- a/src/backend/storage/ipc/pmsignal.c
>> +++ b/src/backend/storage/ipc/pmsignal.c
>> @@ -134,7 +134,7 @@ PMSignalShmemInit(void)
>>
>> if (!found)
>> {
>> -MemSet(PMSignalState, 0, PMSignalShmemSize());
>> +MemSet(unvolatize(PMSignalData *, PMSignalState), 0,
>> PMSignalShmemSize());
>> PMSignalState->num_child_flags = MaxLivePostmasterChildren();
>> }
>> }
>
> Same. Did you put an type assertion into MemSet(), or how did you
> discover this one as needing to be changed?
Build with -Wcast-qual, which warns for this because MemSet() does a
(void *) cast.
> .oO(We really ought to remove MemSet()).
yeah
--
Peter Eisentraut http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services
Re: unconstify equivalent for volatile
Hi, On February 19, 2019 7:00:58 AM PST, Peter Eisentraut wrote: >On 2019-02-18 21:25, Andres Freund wrote: >> ISTM this one should rather be solved by removing all volatiles from >> latch.[ch]. As that's a cross-process concern we can't rely on it >> anyway (and have placed barriers a few years back to allay concerns / >> bugs due to reordering). > >Aren't the volatiles there so that Latch variables can be set from >signal handlers? Why would they be required, given we have an explicit compiler & memory barrier? That forces the compiler to spill the writes to memory already, even in a signal handler. And conversely the reading side is similarly forced - if not we have a bug in multi core systems - to read the variable from memory after a barrier. The real reason why variables commonly need to be volatile when used in signal handlers is not the signal handler side, but the normal code flow side. Without using explicit care the variable's value can be "cached"in a register, and a change not noticed. But as volatiles aren't sufficient for cross process safety, latches need more anyway. Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Re: unconstify equivalent for volatile
Andres Freund writes: > The real reason why variables commonly need to be volatile when used in > signal handlers is not the signal handler side, but the normal code flow > side. Yeah, exactly. You have not explained why it'd be safe to ignore that. regards, tom lane
Re: unconstify equivalent for volatile
Hi, On 2019-02-19 11:48:16 -0500, Tom Lane wrote: > Andres Freund writes: > > The real reason why variables commonly need to be volatile when used in > > signal handlers is not the signal handler side, but the normal code flow > > side. > > Yeah, exactly. You have not explained why it'd be safe to ignore that. Because SetLatch() is careful to have a pg_memory_barrier() before touching shared state and conversely so are ResetLatch() (and WaitEventSetWait(), which already has no volatiles). And if we've got this wrong they aren't safe for shared latches, because volatiles don't enforce meaningful ordering on weakly ordered architectures. But even if we were to decide we'd want to keep a volatile in SetLatch() - which I think really would only serve to hide bugs - that'd not mean it's a good idea to keep it on all the other functions in latch.c. Greetings, Andres Freund
