Hi,
On 2023-01-12 10:44:33 -0800, Nathan Bossart wrote:
> On Thu, Jan 12, 2023 at 11:19:29PM +0800, Xing Guo wrote:
> > @@ -690,12 +690,12 @@ PLy_trigger_build_args(FunctionCallInfo fcinfo,
> > PLyProcedure *proc, HeapTuple *r
> > PyObject *volatile pltdata = NULL;
> > char *stroid;
> >
> > + pltdata = PyDict_New();
> > + if (!pltdata)
> > + return NULL;
> > +
> > PG_TRY();
> > {
> > - pltdata = PyDict_New();
> > - if (!pltdata)
> > - return NULL;
> > -
> > pltname = PLyUnicode_FromString(tdata->tg_trigger->tgname);
> > PyDict_SetItemString(pltdata, "name", pltname);
> > Py_DECREF(pltname);
>
> There's another "return" later on in this PG_TRY block. I wonder if it's
> possible to detect this sort of thing at compile time.
Clang provides some annotations that allow to detect this kind of thing. I
hacked up a test for this, and it finds quite a bit of prolematic
code. plpython is, uh, not being good? But also in plperl, pltcl.
Example complaints:
[776/1239 42 62%] Compiling C object
src/pl/plpython/plpython3.so.p/plpy_exec.c.o
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:472:1:
warning: no_returns_in_pg_try 'no_returns_handle' is not held on every path
through here [-Wthread-safety-analysis]
}
^
../../../../home/andres/src/postgresql/src/pl/plpython/plpy_exec.c:417:2: note:
no_returns_in_pg_try acquired here
PG_TRY();
^
../../../../home/andres/src/postgresql/src/include/utils/elog.h:424:7: note:
expanded from macro 'PG_TRY'
no_returns_start(no_returns_handle##__VA_ARGS__)
^
...
[785/1239 42 63%] Compiling C object src/pl/tcl/pltcl.so.p/pltcl.c.o
../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1830:1: warning:
no_returns_in_pg_try 'no_returns_handle' is not held on every path through here
[-Wthread-safety-analysis]
}
^
../../../../home/andres/src/postgresql/src/pl/tcl/pltcl.c:1809:2: note:
no_returns_in_pg_try acquired here
PG_CATCH();
^
../../../../home/andres/src/postgresql/src/include/utils/elog.h:433:7: note:
expanded from macro 'PG_CATCH'
no_returns_start(no_returns_handle##__VA_ARGS__)
^
Not perfect digestible, but also not too bad. I pushed the
no_returns_start()/no_returns_stop() calls into all the PG_TRY related macros,
because that causes the warning to point to block that the problem is
in. E.g. above the first warning points to PG_TRY, the second to
PG_CATCH. It'd work to just put it into PG_TRY and PG_END_TRY.
Clearly this would need a bunch more work, but it seems promising? I think
there'd be other uses than this.
I briefly tried to use it for spinlocks. Mostly works and detects things like
returning with a spinlock held. But it does not like dynahash's habit of
conditionally acquiring and releasing spinlocks.
Greetings,
Andres Freund
>From d1c99e9d12ba01adb21c5f17c792be44cfeef20f Mon Sep 17 00:00:00 2001
From: Andres Freund <[email protected]>
Date: Thu, 12 Jan 2023 21:18:55 -0800
Subject: [PATCH v1] wip: use clang anotations to warn if code in
PG_TRY/CATCH/FINALLY returns
Only hooked up to meson right now.
---
meson.build | 1 +
src/include/utils/elog.h | 43 +++++++++++++++++++++++++++++++++++++---
2 files changed, 41 insertions(+), 3 deletions(-)
diff --git a/meson.build b/meson.build
index 45fb9dd616e..66a40e728f4 100644
--- a/meson.build
+++ b/meson.build
@@ -1741,6 +1741,7 @@ common_warning_flags = [
'-Wimplicit-fallthrough=3',
'-Wcast-function-type',
'-Wshadow=compatible-local',
+ '-Wthread-safety',
# This was included in -Wall/-Wformat in older GCC versions
'-Wformat-security',
]
diff --git a/src/include/utils/elog.h b/src/include/utils/elog.h
index 4a9562fdaae..b211e08322a 100644
--- a/src/include/utils/elog.h
+++ b/src/include/utils/elog.h
@@ -381,32 +381,69 @@ extern PGDLLIMPORT ErrorContextCallback *error_context_stack;
* same within each component macro of the given PG_TRY() statement.
*----------
*/
+
+
+/*
+ * Annotations for detecting returns inside a PG_TRY(), using clang's thread
+ * safety annotations.
+ *
+ * The "lock" implementations need no_thread_safety_analysis as clang can't
+ * understand how a lock is implemented. We wouldn't want an implementation
+ * anyway, since there's no real lock here.
+ */
+#ifdef __clang__
+
+typedef int __attribute__((capability("no_returns_in_pg_try"))) no_returns_handle_t;
+
+static inline void no_returns_start(no_returns_handle_t l)
+ __attribute__((acquire_capability(l)))
+ __attribute__((no_thread_safety_analysis))
+{
+}
+
+static inline void no_returns_stop(no_returns_handle_t l)
+ __attribute__((release_capability(l)))
+ __attribute__((no_thread_safety_analysis))
+{}
+#else
+typedef int pg_attribute_unused() no_returns_handle_t;
+#define no_returns_start(t) (void)0
+#define no_returns_stop(t) (void)0
+#endif
+
#define PG_TRY(...) \
do { \
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; \
+ no_returns_handle_t no_returns_handle##__VA_ARGS__ = 0; \
if (sigsetjmp(_local_sigjmp_buf##__VA_ARGS__, 0) == 0) \
{ \
- PG_exception_stack = &_local_sigjmp_buf##__VA_ARGS__
+ PG_exception_stack = &_local_sigjmp_buf##__VA_ARGS__; \
+ no_returns_start(no_returns_handle##__VA_ARGS__)
#define PG_CATCH(...) \
+ no_returns_stop(no_returns_handle##__VA_ARGS__); \
} \
else \
{ \
PG_exception_stack = _save_exception_stack##__VA_ARGS__; \
- error_context_stack = _save_context_stack##__VA_ARGS__
+ error_context_stack = _save_context_stack##__VA_ARGS__; \
+ no_returns_start(no_returns_handle##__VA_ARGS__)
#define PG_FINALLY(...) \
+ no_returns_stop(no_returns_handle##__VA_ARGS__); \
} \
else \
_do_rethrow##__VA_ARGS__ = true; \
{ \
PG_exception_stack = _save_exception_stack##__VA_ARGS__; \
- error_context_stack = _save_context_stack##__VA_ARGS__
+ error_context_stack = _save_context_stack##__VA_ARGS__; \
+ no_returns_start(no_returns_handle##__VA_ARGS__)
#define PG_END_TRY(...) \
+ no_returns_stop(no_returns_handle##__VA_ARGS__); \
} \
if (_do_rethrow##__VA_ARGS__) \
PG_RE_THROW(); \
--
2.38.0
