Horton

Thread

Commits

  1. Add macro to cast away volatile without allowing changes to underlying type

  2. Initialize structure at declaration

  1. unconstify equivalent for volatile

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-18T15:39:25Z

    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

  2. Re: unconstify equivalent for volatile

    Andres Freund <andres@anarazel.de> — 2019-02-18T15:42:33Z

    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 call sites.

Andres

Andres
-- 
Sent from my Android device with K-9 Mail. Please excuse my brevity.

  3. Re: unconstify equivalent for volatile

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-18T15:43:50Z

    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

  4. Re: unconstify equivalent for volatile

    Andres Freund <andres@anarazel.de> — 2019-02-18T16:32:00Z

    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

  5. Re: unconstify equivalent for volatile

    Petr Jelinek <petr.jelinek@2ndquadrant.com> — 2019-02-18T20:18:42Z

    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

  6. Re: unconstify equivalent for volatile

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-18T20:20:55Z

    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

  7. Re: unconstify equivalent for volatile

    Andres Freund <andres@anarazel.de> — 2019-02-18T20:25:11Z

    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()).

  8. Re: unconstify equivalent for volatile

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-19T15:00:58Z

    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

  9. Re: unconstify equivalent for volatile

    Andres Freund <andres@anarazel.de> — 2019-02-19T16:09:21Z

    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 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.

  10. Re: unconstify equivalent for volatile

    Tom Lane <tgl@sss.pgh.pa.us> — 2019-02-19T16:48:16Z

    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

  11. Re: unconstify equivalent for volatile

    Andres Freund <andres@anarazel.de> — 2019-02-19T17:02:44Z

    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

  12. Re: unconstify equivalent for volatile

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-22T11:38:35Z

    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

  13. Re: unconstify equivalent for volatile

    Andres Freund <andres@anarazel.de> — 2019-02-22T20:31:39Z

    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

  14. Re: unconstify equivalent for volatile

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-02-25T08:29:36Z

    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

  15. Re: unconstify equivalent for volatile

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-03-04T10:36:48Z

    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

  16. Re: unconstify equivalent for volatile

    Peter Eisentraut <peter.eisentraut@2ndquadrant.com> — 2019-03-19T10:52:47Z

    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 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