pg.ddx.io pgsql-committers@postgresql.org mailing list archive
help / color / mirror / Atom feedpgsql: Fix up misuse of "volatile" in contrib/xml2.
6+ messages / 3 participants
[nested] [flat]
* pgsql: Fix up misuse of "volatile" in contrib/xml2.
@ 2025-07-08 21:00 Tom Lane <tgl@sss.pgh.pa.us>
2025-07-08 23:32 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2025-07-08 21:00 UTC (permalink / raw)
To: pgsql-committers@lists.postgresql.org
Fix up misuse of "volatile" in contrib/xml2.
What we want in these places is "xmlChar *volatile ptr",
not "volatile xmlChar *ptr". The former means that the
pointer variable itself needs to be treated as volatile,
while the latter says that what it points to is volatile.
Since the point here is to ensure that the pointer variables
don't go crazy after a longjmp, it's the former semantics
that we need. The misplacement of "volatile" also led
to needing to cast away volatile in some places.
Also fix a number of places where variables that are assigned to
within a PG_TRY and then used after it were not initialized or
not marked as volatile. (A few buildfarm members were issuing
"may be used uninitialized" warnings about some of these variables,
which is what drew my attention to this area.) In most cases
these variables were being set as the last step within the PG_TRY
block, which might mean that we could get away without the "volatile"
marking. But doing that seems unsafe and is definitely not per our
coding conventions.
These problems seem to have come in with 732061150, so no need
for back-patch.
Branch
------
master
Details
-------
https://git.postgresql.org/pg/commitdiff/93001888d85c21a5b9ab1fe8dabfecb673fc007c
Modified Files
--------------
contrib/xml2/xpath.c | 49 ++++++++++++++++++++++++------------------------
contrib/xml2/xslt_proc.c | 10 +++++-----
2 files changed, 29 insertions(+), 30 deletions(-)
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: Fix up misuse of "volatile" in contrib/xml2.
2025-07-08 21:00 pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
@ 2025-07-08 23:32 ` Michael Paquier <michael@paquier.xyz>
2025-07-09 15:49 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2025-07-08 23:32 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers@lists.postgresql.org
On Tue, Jul 08, 2025 at 09:00:39PM +0000, Tom Lane wrote:
> Fix up misuse of "volatile" in contrib/xml2.
>
> Also fix a number of places where variables that are assigned to
> within a PG_TRY and then used after it were not initialized or
> not marked as volatile. (A few buildfarm members were issuing
> "may be used uninitialized" warnings about some of these variables,
> which is what drew my attention to this area.) In most cases
> these variables were being set as the last step within the PG_TRY
> block, which might mean that we could get away without the "volatile"
> marking. But doing that seems unsafe and is definitely not per our
> coding conventions.
>
> These problems seem to have come in with 732061150, so no need
> for back-patch.
Oops, thanks. I was not aware of these reports, and the buildfarm was
not showing any red, the CI looked fine and my machine did not
complain with a rather new gcc. What were the buildfarm members
impacted? Did these use a switch and/or a specific compiler that
helped in detecting these problems?
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: Fix up misuse of "volatile" in contrib/xml2.
2025-07-08 21:00 pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
2025-07-08 23:32 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Michael Paquier <michael@paquier.xyz>
@ 2025-07-09 15:49 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-07-10 03:55 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Michael Paquier <michael@paquier.xyz>
0 siblings, 1 reply; 6+ messages in thread
From: Tom Lane @ 2025-07-09 15:49 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: pgsql-committers@lists.postgresql.org
Michael Paquier <michael@paquier.xyz> writes:
> On Tue, Jul 08, 2025 at 09:00:39PM +0000, Tom Lane wrote:
>> Also fix a number of places where variables that are assigned to
>> within a PG_TRY and then used after it were not initialized or
>> not marked as volatile. (A few buildfarm members were issuing
>> "may be used uninitialized" warnings about some of these variables,
>> which is what drew my attention to this area.)
> Oops, thanks. I was not aware of these reports, and the buildfarm was
> not showing any red, the CI looked fine and my machine did not
> complain with a rather new gcc. What were the buildfarm members
> impacted? Did these use a switch and/or a specific compiler that
> helped in detecting these problems?
Not sure. Yesterday I saw such warnings from arowana,
boa, dhole, rhinoceros, and shelduck, eg
arowana | 2025-07-08 04:54:18 | xpath.c:274:6: warning: 'workspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
arowana | 2025-07-08 04:54:18 | xpath.c:319:6: warning: 'workspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
arowana | 2025-07-08 04:54:18 | xpath.c:374:6: warning: 'workspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
arowana | 2025-07-08 04:54:18 | ../../src/include/postgres.h:329:2: warning: 'result' may be used uninitialized in this function [-Wmaybe-uninitialized]
Didn't look to try to figure out what the common factor
among these machines is, but I think all of them are somewhat
dated, which is depressing. You'd hope that newer compilers
are more likely to find such issues, not less likely.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: Fix up misuse of "volatile" in contrib/xml2.
2025-07-08 21:00 pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
2025-07-08 23:32 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Michael Paquier <michael@paquier.xyz>
2025-07-09 15:49 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
@ 2025-07-10 03:55 ` Michael Paquier <michael@paquier.xyz>
2025-07-10 17:21 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Jacob Champion <jacob.champion@enterprisedb.com>
0 siblings, 1 reply; 6+ messages in thread
From: Michael Paquier @ 2025-07-10 03:55 UTC (permalink / raw)
To: Tom Lane <tgl@sss.pgh.pa.us>; +Cc: pgsql-committers@lists.postgresql.org
On Wed, Jul 09, 2025 at 11:49:55AM -0400, Tom Lane wrote:
> Not sure. Yesterday I saw such warnings from arowana,
> boa, dhole, rhinoceros, and shelduck, eg
>
> arowana | 2025-07-08 04:54:18 | xpath.c:274:6: warning: 'workspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
> arowana | 2025-07-08 04:54:18 | xpath.c:319:6: warning: 'workspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
> arowana | 2025-07-08 04:54:18 | xpath.c:374:6: warning: 'workspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
> arowana | 2025-07-08 04:54:18 | ../../src/include/postgres.h:329:2: warning: 'result' may be used uninitialized in this function [-Wmaybe-uninitialized]
>
> Didn't look to try to figure out what the common factor
> among these machines is, but I think all of them are somewhat
> dated, which is depressing. You'd hope that newer compilers
> are more likely to find such issues, not less likely.
They are all using some gcc 4.X flavor, most with -O2 but not all.
And -Wmaybe-uninitialized is included in my default switches with a
gcc 14, and there is nothing with several levels of optimizations
applied, up to -O3. :(
--
Michael
Attachments:
signature.asc (application/pgp-signature, 832B) download
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: Fix up misuse of "volatile" in contrib/xml2.
2025-07-08 21:00 pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
2025-07-08 23:32 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Michael Paquier <michael@paquier.xyz>
2025-07-09 15:49 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
2025-07-10 03:55 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Michael Paquier <michael@paquier.xyz>
@ 2025-07-10 17:21 ` Jacob Champion <jacob.champion@enterprisedb.com>
2025-07-10 17:27 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 1 reply; 6+ messages in thread
From: Jacob Champion @ 2025-07-10 17:21 UTC (permalink / raw)
To: Michael Paquier <michael@paquier.xyz>; +Cc: Tom Lane <tgl@sss.pgh.pa.us>; pgsql-committers@lists.postgresql.org
On Wed, Jul 9, 2025 at 8:55 PM Michael Paquier <michael@paquier.xyz> wrote:
>
> On Wed, Jul 09, 2025 at 11:49:55AM -0400, Tom Lane wrote:
> > Not sure. Yesterday I saw such warnings from arowana,
> > boa, dhole, rhinoceros, and shelduck, eg
> >
> > arowana | 2025-07-08 04:54:18 | xpath.c:274:6: warning: 'workspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > arowana | 2025-07-08 04:54:18 | xpath.c:319:6: warning: 'workspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > arowana | 2025-07-08 04:54:18 | xpath.c:374:6: warning: 'workspace' may be used uninitialized in this function [-Wmaybe-uninitialized]
> > arowana | 2025-07-08 04:54:18 | ../../src/include/postgres.h:329:2: warning: 'result' may be used uninitialized in this function [-Wmaybe-uninitialized]
> >
> > Didn't look to try to figure out what the common factor
> > among these machines is, but I think all of them are somewhat
> > dated, which is depressing. You'd hope that newer compilers
> > are more likely to find such issues, not less likely.
>
> They are all using some gcc 4.X flavor, most with -O2 but not all.
>
> And -Wmaybe-uninitialized is included in my default switches with a
> gcc 14, and there is nothing with several levels of optimizations
> applied, up to -O3. :(
I think I remember that GCC has had historical problems with tuning
the false-positive:false-negative rates for `-Wmaybe-uninitialized`.
It's not super surprising to me that later versions aren't always
better at seeing specific problems, especially if users were
complaining that an earlier version was too sensitive...
--Jacob
^ permalink raw reply [nested|flat] 6+ messages in thread
* Re: pgsql: Fix up misuse of "volatile" in contrib/xml2.
2025-07-08 21:00 pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
2025-07-08 23:32 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Michael Paquier <michael@paquier.xyz>
2025-07-09 15:49 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
2025-07-10 03:55 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Michael Paquier <michael@paquier.xyz>
2025-07-10 17:21 ` Re: pgsql: Fix up misuse of "volatile" in contrib/xml2. Jacob Champion <jacob.champion@enterprisedb.com>
@ 2025-07-10 17:27 ` Tom Lane <tgl@sss.pgh.pa.us>
0 siblings, 0 replies; 6+ messages in thread
From: Tom Lane @ 2025-07-10 17:27 UTC (permalink / raw)
To: Jacob Champion <jacob.champion@enterprisedb.com>; +Cc: Michael Paquier <michael@paquier.xyz>; pgsql-committers@lists.postgresql.org
Jacob Champion <jacob.champion@enterprisedb.com> writes:
> On Wed, Jul 9, 2025 at 8:55 PM Michael Paquier <michael@paquier.xyz> wrote:
>> On Wed, Jul 09, 2025 at 11:49:55AM -0400, Tom Lane wrote:
>>> Not sure. Yesterday I saw such warnings from arowana,
>>> boa, dhole, rhinoceros, and shelduck, eg
>> They are all using some gcc 4.X flavor, most with -O2 but not all.
> I think I remember that GCC has had historical problems with tuning
> the false-positive:false-negative rates for `-Wmaybe-uninitialized`.
> It's not super surprising to me that later versions aren't always
> better at seeing specific problems, especially if users were
> complaining that an earlier version was too sensitive...
Yeah. Worth noting also is that even the machines that were
complaining were warning about just a subset of the xpath.c functions
that had this problem :-(. So there's definitely some heuristics
involved, which seems odd for what feels like it ought to be a
pretty black-and-white condition.
regards, tom lane
^ permalink raw reply [nested|flat] 6+ messages in thread
end of thread, other threads:[~2025-07-10 17:27 UTC | newest]
Thread overview: 6+ messages (download: mbox mbox.gz follow: Atom feed)
-- links below jump to the message on this page --
2025-07-08 21:00 pgsql: Fix up misuse of "volatile" in contrib/xml2. Tom Lane <tgl@sss.pgh.pa.us>
2025-07-08 23:32 ` Michael Paquier <michael@paquier.xyz>
2025-07-09 15:49 ` Tom Lane <tgl@sss.pgh.pa.us>
2025-07-10 03:55 ` Michael Paquier <michael@paquier.xyz>
2025-07-10 17:21 ` Jacob Champion <jacob.champion@enterprisedb.com>
2025-07-10 17:27 ` Tom Lane <tgl@sss.pgh.pa.us>
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox