Обсуждение: 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
Вложения
Hi, On February 18, 2019 7:39:25 AM PST, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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 callsites. Andres Andres -- Sent from my Android device with K-9 Mail. Please excuse my brevity.
Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
Hi, On 2019-02-18 10:43:50 -0500, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
On 18/02/2019 16:43, Tom Lane wrote: > Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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
Petr Jelinek <petr.jelinek@2ndquadrant.com> 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
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()).
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
Hi, On February 19, 2019 7:00:58 AM PST, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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 writesto memory already, even in a signal handler. And conversely the reading side is similarly forced - if not we have abug 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, butthe normal code flow side. Without using explicit care the variable's value can be "cached"in a register, and a changenot 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.
Andres Freund <andres@anarazel.de> 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
Hi, On 2019-02-19 11:48:16 -0500, Tom Lane wrote: > Andres Freund <andres@anarazel.de> 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
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
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
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
Вложения
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
On 2019-02-18 16:42, Andres Freund wrote: > On February 18, 2019 7:39:25 AM PST, Peter Eisentraut <peter.eisentraut@2ndquadrant.com> 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 sigsetjmpcall 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