Fix volatile vs. pointer confusion

classic Classic list List threaded Threaded
6 messages Options
Reply | Threaded
Open this post in threaded view
|

Fix volatile vs. pointer confusion

Peter Eisentraut-6
Variables used after a longjmp() need to be declared volatile.  In
case of a pointer, it's the pointer itself that needs to be declared
volatile, not the pointed-to value.  So we need

    PyObject *volatile items;

instead of

    volatile PyObject *items;  /* wrong */

Attached patch fixes a couple of cases of that.  Most instances were
already correct.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

0001-Fix-volatile-vs.-pointer-confusion.patch (3K) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix volatile vs. pointer confusion

Michael Paquier-2
On Mon, Mar 11, 2019 at 08:23:39AM +0100, Peter Eisentraut wrote:
> Attached patch fixes a couple of cases of that.  Most instances were
> already correct.

It seems to me that you should look at that:
https://www.postgresql.org/message-id/20190308055911.GG4099@...
They treat about the same subject, and a patch has been sent for this
CF.
--
Michael

signature.asc (849 bytes) Download Attachment
Reply | Threaded
Open this post in threaded view
|

Re: Fix volatile vs. pointer confusion

Alvaro Herrera-9
In reply to this post by Peter Eisentraut-6
On 2019-Mar-11, Peter Eisentraut wrote:

> Variables used after a longjmp() need to be declared volatile.  In
> case of a pointer, it's the pointer itself that needs to be declared
> volatile, not the pointed-to value.  So we need
>
>     PyObject *volatile items;
>
> instead of
>
>     volatile PyObject *items;  /* wrong */

Looking at recently committed 2e616dee9e60, we have introduced this:

+       volatile xmlBufferPtr buf = NULL;
+       volatile xmlNodePtr cur_copy = NULL;

where the pointer-ness nature of the object is inside the typedef.  I
*suppose* that this is correct as written.  There are a few occurrences
of this pattern in eg. contrib/xml2.


--
Álvaro Herrera                https://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Fix volatile vs. pointer confusion

Peter Eisentraut-6
In reply to this post by Michael Paquier-2
On 2019-03-11 09:31, Michael Paquier wrote:
> On Mon, Mar 11, 2019 at 08:23:39AM +0100, Peter Eisentraut wrote:
>> Attached patch fixes a couple of cases of that.  Most instances were
>> already correct.
>
> It seems to me that you should look at that:
> https://www.postgresql.org/message-id/20190308055911.GG4099@...
> They treat about the same subject, and a patch has been sent for this
> CF.

I'm aware of that patch and have been looking at it.  But it's not
directly related to this issue.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Fix volatile vs. pointer confusion

Peter Eisentraut-6
In reply to this post by Alvaro Herrera-9
On 2019-03-11 12:57, Alvaro Herrera wrote:
> Looking at recently committed 2e616dee9e60, we have introduced this:
>
> +       volatile xmlBufferPtr buf = NULL;
> +       volatile xmlNodePtr cur_copy = NULL;
>
> where the pointer-ness nature of the object is inside the typedef.  I
> *suppose* that this is correct as written.  There are a few occurrences
> of this pattern in eg. contrib/xml2.

I think this is correct, but I don't want to wreck my sanity trying to
understand the syntax-level details of why.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services

Reply | Threaded
Open this post in threaded view
|

Re: Fix volatile vs. pointer confusion

Peter Eisentraut-6
In reply to this post by Peter Eisentraut-6
On 2019-03-11 08:23, Peter Eisentraut wrote:

> Variables used after a longjmp() need to be declared volatile.  In
> case of a pointer, it's the pointer itself that needs to be declared
> volatile, not the pointed-to value.  So we need
>
>     PyObject *volatile items;
>
> instead of
>
>     volatile PyObject *items;  /* wrong */
>
> Attached patch fixes a couple of cases of that.  Most instances were
> already correct.

Committed.

I'll wait for the build farm to see if there are any new compiler
warnings because of this, then backpatch.

--
Peter Eisentraut              http://www.2ndQuadrant.com/
PostgreSQL Development, 24x7 Support, Remote DBA, Training & Services