Warning -Wclobbered in PG_TRY(...)
Hi, hackers!
While building pg_duckdb extension with PostgreSQL 17.5 we found
-Wclobbered warning from gcc with PG_TRY():
g++ -Wall -Wpointer-arith -Wendif-labels -Wmissing-format-attribute
-Wimplicit-fallthrough=3 -Wcast-function-type -Wshadow=compatible-local
-Wformat-security -fno-strict-aliasing -fwrapv -g -ggdb -Og -g3
-fno-omit-frame-pointer -DDEBUG -fvisibility=default -std=c++17
-Wno-sign-compare -Wshadow -Wswitch -Wunused-parameter
-Wunreachable-code -Wno-unknown-pragmas -Wall -Wextra -Wno-register
-fPIC -fvisibility=hidden -fvisibility-inlines-hidden -Iinclude -isystem
third_party/duckdb/src/include -isystem
third_party/duckdb/third_party/re2 -isystem
/home/user/data/pgddb/bins/include/postgresql/server -Wno-sign-compare
-Wshadow -Wswitch -Wunused-parameter -Wunreachable-code
-Wno-unknown-pragmas -Wall -Wextra -I. -I./
-I/home/user/data/pgddb/bins/include/postgresql/server
-I/home/user/data/pgddb/bins/include/postgresql/internal -D_GNU_SOURCE
-I/usr/include/libxml2 -c -o src/catalog/pgduckdb_storage.o
src/catalog/pgduckdb_storage.cpp -MMD -MP -MF .deps/pgduckdb_storage.Po
In file included from
/home/user/data/pgddb/bins/include/postgresql/server/postgres.h:46,
from include/pgduckdb/utility/cpp_wrapper.hpp:6,
from src/pgduckdb_background_worker.cpp:19:
src/pgduckdb_background_worker.cpp: In function ‘Datum
force_motherduck_sync(FunctionCallInfo)’:
src/pgduckdb_background_worker.cpp:289:9: warning: variable
‘_do_rethrow’ might be clobbered by ‘longjmp’ or ‘vfork’ [-Wclobbered]
289 | PG_TRY();
| ^~~~~~
Looking into a thread
/messages/by-id/2eda015b-7dff-47fd-d5e2-f1a9899b90a6@postgrespro.ru
we changed
bool _do_rethrow##__VA_ARGS__ = false; \
to
volatile bool _do_rethrow##__VA_ARGS__ = false; \
and the warning disappeared.
But what is not clear here - why does this warning happens?
According to sigsetjmp manual:
"
The compiler may optimize variables into registers, and longjmp() may
restore the values of other registers in addition to the stack pointer
and program counter. Consequently, the values of automatic variables
are unspecified after a call to longjmp() if they meet
all the following criteria:
• they are local to the function that made the corresponding setjmp()
call;
• their values are changed between the calls to setjmp() and longjmp();
and
• they are not declared as volatile.
"
In our case the first and the third statements hold. But it is not
obvious here - why, where and how the local variable _do_rethrow can be
changed?
And what the -Wclobbered warning really means here?
Does it makes sense to add volatile attribute to the _do_rethrow or
should we just ignore that -Wclobbered warning?
Any thoughts and advices are very welcome.
Versions:
PostgreSQL: branch REL_17_STABLE
commit a3c6d92f3cb3e49bde59e52268e2d74db05d7789
Author: Michael Paquier <michael@paquier.xyz>
Date: Wed May 28 09:43:45 2025 +0900
pg_duckdb:
commit aaedec1cf7d7fed1018ff3767d0bd4f85ea4f89a
Author: Jelte Fennema-Nio <jelte@motherduck.com>
Date: Wed May 28 14:54:02 2025 +0200
gcc:
gcc (Ubuntu 11.4.0-1ubuntu1~22.04) 11.4.0
PostgreSQL config:
../configure --enable-debug --enable-cassert --enable-depend
--enable-tap-tests --enable-nls --with-perl --with-icu --with-libxml
--with-libxslt --with-gssapi --with-openssl --with-zstd --with-lz4
--with-ldap --with-python --prefix=$PGINSTDIR --with-pgport=$PGPORT
Kind regards,
MIkhail Litsarev
m.litsarev@postgrespro.ru writes:
Does it makes sense to add volatile attribute to the _do_rethrow or
should we just ignore that -Wclobbered warning?
-Wclobbered would be really useful if it worked --- but sadly,
on practically all gcc and clang versions, those warnings have
nearly nothing to do with reality. We typically ignore them.
See eg:
/messages/by-id/4001633.1722625252@sss.pgh.pa.us
regards, tom lane
On Thu, 29 May 2025 at 14:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
-Wclobbered would be really useful if it worked --- but sadly,
on practically all gcc and clang versions, those warnings have
nearly nothing to do with reality. We typically ignore them.
Yes, in the vast majority of cases. But I believe this is not one of them.
Actually, we do
not adhere to the C language standard for a setjump/logjump. Notice how the
non-volatile local variable "bool dorethrow##" utilized in the
setjump/logjump block.
This topic reminds me of [0]/messages/by-id/2eda015b-7dff-47fd-d5e2-f1a9899b90a6@postgrespro.ru.
In fact, we do not receive a warning for a C-lang build because the
compiler does not
appear to be "smart" enough to put this variable in a register. I ran a
synthetic test
equivalent to PG_TRY using godbolt.org, and the final code was the same
whether
the volatile qualifier was used or not. This is not the case with the C++
build. If the
volatile qualifier is not used, the compiler optimizes this line and puts
it in a register,
and we get a warning. And to get rid of this warning, you need to either
turn it off via
the pragma directive, write your own version of PG_TRY with a volatile type
qualifier,
or patch Postgres. None of these solutions sound good to me. You may be
wondering
who will write Postgres extensions in C++. Having two different runtimes is
absolute
misery, and I completely agree. However, such extensions already exist; for
example,
pg_duckdb. AFAICS, it is quite popular in our area.
So, to be safe, we should follow the standard and include a volatile
qualifier here. It
really costs us nothing; the resulting code will be identical for C, and
we'll avoid any
future issues. In the case of C++, it will also resolve the warning.
Don't consider me as something of a formalist or someone who wants to make
mountains out of molehills. I just want to make things better. PFA trivial
patch.
[0]: /messages/by-id/2eda015b-7dff-47fd-d5e2-f1a9899b90a6@postgrespro.ru
/messages/by-id/2eda015b-7dff-47fd-d5e2-f1a9899b90a6@postgrespro.ru
--
Best regards,
Maxim Orlov.
Attachments:
v1-0001-Use-volatile-to-avoid-unspecified-behavior-in-PG_.patchapplication/octet-stream; name=v1-0001-Use-volatile-to-avoid-unspecified-behavior-in-PG_.patchDownload
From cc0dae3abd10bc73c2a1fbf3dc86e1e4dd8be922 Mon Sep 17 00:00:00 2001
From: Maxim Orlov <orlovmg@gmail.com>
Date: Thu, 29 May 2025 17:19:09 +0300
Subject: [PATCH v1] Use volatile to avoid unspecified behavior in PG_TRY
---
src/include/utils/elog.h | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 5eac0e16970..5d0e3fd4aa4 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -387,7 +387,7 @@ extern PGDLLIMPORT ErrorContextCallback *error_context_stack;
sigjmp_buf *_save_exception_stack##__VA_ARGS__ = PG_exception_stack; \
ErrorContextCallback *_save_context_stack##__VA_ARGS__ = error_context_stack; \
sigjmp_buf _local_sigjmp_buf##__VA_ARGS__; \
- bool _do_rethrow##__VA_ARGS__ = false; \
+ volatile bool _do_rethrow##__VA_ARGS__ = false; \
if (sigsetjmp(_local_sigjmp_buf##__VA_ARGS__, 0) == 0) \
{ \
PG_exception_stack = &_local_sigjmp_buf##__VA_ARGS__
--
2.49.0
Maxim Orlov <orlovmg@gmail.com> writes:
On Thu, 29 May 2025 at 14:14, Tom Lane <tgl@sss.pgh.pa.us> wrote:
-Wclobbered would be really useful if it worked --- but sadly,
on practically all gcc and clang versions, those warnings have
nearly nothing to do with reality. We typically ignore them.
Yes, in the vast majority of cases. But I believe this is not one of them.
Actually, we do
not adhere to the C language standard for a setjump/logjump.
Really? I don't have a current C standard at hand, but POSIX-2024 [1]
claims to be aligned with the C standard for this, and what it says
under "longjmp" is
All accessible objects have values, and all other components of
the abstract machine have state (for example, floating-point
status flags and open files), as of the time longjmp() was called,
except that the values of objects of automatic storage duration
are unspecified if they meet all the following conditions:
They are local to the function containing the corresponding
setjmp() invocation.
They do not have volatile-qualified type.
They are changed between the setjmp() invocation and longjmp() call.
That is pretty black-and-white, and the PG_TRY macros do not
change _do_rethrow between setjmp() and longjmp(). PG_FINALLY
will change it after return from longjmp(), but at that point
it doesn't matter. So if we need a volatile qualifier here,
it means the implementation fails to adhere to the C standard.
I'm not clear what conditions provoke the -Wclobbered warning,
but many cases seem to amount to "the value of the variable
gets changed before reaching PG_TRY". That again is not a
case that the implementation is allowed to call undefined.
regards, tom lane
I wrote:
Really? I don't have a current C standard at hand, but POSIX-2024 [1]
claims to be aligned with the C standard for this,
Ooops, forgot to attach the URL:
[1]: https://pubs.opengroup.org/onlinepubs/9799919799/
Go there and use the "keyword search" for "longjmp" --- they
don't seem to make it possible to bookmark individual pages.
regards, tom lane
On Thu, 29 May 2025 at 18:26, Tom Lane <tgl@sss.pgh.pa.us> wrote:
Go there and use the "keyword search" for "longjmp" --- they
don't seem to make it possible to bookmark individual pages.Indeed, you are fully correct. I didn't analyse the code well enough. The
equivalent code
for PG_TRY/PG_FINALLY/PG_END_TRY will appear like the following:
do {
jmp_buf *_save_exception_stack = PG_exception_stack;
jmp_buf _local_sigjmp_buf;
bool _do_rethrow = false;
if (setjmp(_local_sigjmp_buf) == 0)
{
PG_exception_stack = &_local_sigjmp_buf;
/* try here */
longjmp(_local_sigjmp_buf, 1); /* elog ERROR */
}
else
_do_rethrow = true;
{
PG_exception_stack = _save_exception_stack;
/* catch */
}
if (_do_rethrow)
pg_re_throw();
PG_exception_stack = _save_exception_stack;
/* finally */
} while (0);
There is no problem here. We only change _do_rethrow after longjmp.
Apparently, we're
dealing with a false positive warning here. Not sure, but it could be
something like [0]https://gcc.gnu.org/bugzilla/show_bug.cgi?id=21161. The
correct solution in this case would be to disable clobbered warning.
[0]: https://gcc.gnu.org/bugzilla/show_bug.cgi?id=21161
--
Best regards,
Maxim Orlov.