On Wed Dec 27, 2023 at 6:42 AM CST, Peter Eisentraut wrote:
On 19.12.23 21:43, Tristan Partin wrote:
> Here is a patch which adds support for the returns_nonnull attribute
> alongside all the other attributes we optionally support.
>
> I recently wound up in a situation where I was checking for NULL return
> values of a function that couldn't ever return NULL because the
> inability to allocate memory was always elog(ERROR)ed (aborted).
>
> I didn't go through and mark anything, but I feel like it could be
> useful for people going forward, including myself.
I think it would be useful if this patch series contained a patch that
added some initial uses of this. That way we can check that the
proposed definition actually works, and we can observe what it does,
with respect to warnings, static analysis, etc.
Good point. Patch attached.
I tried to find some ways to prove some value, but I couldn't. Take this
example for instance:
static const char word[] = { 'h', 'e', 'l', 'l', 'o' };
const char * __attribute__((returns_nonnull))
hello()
{
return word;
}
int
main(void)
{
const char *r;
r = hello();
if (r == NULL)
return 1;
return 0;
}
I would have thought I could get gcc or clang to warn on a wasteful NULL
check, but alas. I also didn't see any code generation improvements, but
I am assuming that the example is too contrived. I couldn't find any
good things online that had examples of when such an annotation forced
the compiler to warn or create more optimized code.
If you return NULL from the hello() function, clang will warn that the
attribute doesn't match reality.
--
Tristan Partin
Neon (https://neon.tech)
From fe4916093d0ce036e7b70595b39351b4ecf93798 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tris...@neon.tech>
Date: Tue, 19 Dec 2023 14:39:03 -0600
Subject: [PATCH v2 1/2] Add support for __attribute__((returns_nonnull))
Allows for marking functions that can't possibly return NULL, like those
that always elog(ERROR) for instance in the case of failures.
While not only being good documentation, annotating functions which
can't return NULL can lead to better code generation since the optimizer
can more easily analyze a function. Quoting the LLVM documentation:
> The returns_nonnull attribute implies that returning a null pointer is
> undefined behavior, which the optimizer may take advantage of.
The more we mark, the better the analysis can become.
---
src/include/c.h | 12 ++++++++++++
1 file changed, 12 insertions(+)
diff --git a/src/include/c.h b/src/include/c.h
index 26bf7ec16e..e3a127f954 100644
--- a/src/include/c.h
+++ b/src/include/c.h
@@ -285,6 +285,18 @@
#define pg_unreachable() abort()
#endif
+/*
+ * Place on functions which return a pointer but can't return NULL. When used,
+ * it can allow the compiler to warn if a NULL check occurs in the parent
+ * function because that NULL check would always fail. It is also an opportunity
+ * to help the compiler with optimizations.
+ */
+#if __has_attribute (returns_nonnull)
+#define pg_returns_nonnull __attribute__((returns_nonnull))
+#else
+#define pg_returns_nonnull
+#endif
+
/*
* Hints to the compiler about the likelihood of a branch. Both likely() and
* unlikely() return the boolean value of the contained expression.
--
Tristan Partin
Neon (https://neon.tech)
From 1dc3544f887c857f55cc5579df240179496f72b6 Mon Sep 17 00:00:00 2001
From: Tristan Partin <tris...@neon.tech>
Date: Wed, 27 Dec 2023 11:21:11 -0600
Subject: [PATCH v2 2/2] Mark some initial candidates as returns_nonnull
---
src/include/common/fe_memutils.h | 22 ++++++++++++----------
src/include/utils/palloc.h | 28 ++++++++++++++--------------
src/include/utils/pg_locale.h | 2 +-
3 files changed, 27 insertions(+), 25 deletions(-)
diff --git a/src/include/common/fe_memutils.h b/src/include/common/fe_memutils.h
index 89601cc778..0f026dbd18 100644
--- a/src/include/common/fe_memutils.h
+++ b/src/include/common/fe_memutils.h
@@ -9,6 +9,8 @@
#ifndef FE_MEMUTILS_H
#define FE_MEMUTILS_H
+#include "c.h"
+
/*
* Flags for pg_malloc_extended and palloc_extended, deliberately named
* the same as the backend flags.
@@ -22,11 +24,11 @@
* "Safe" memory allocation functions --- these exit(1) on failure
* (except pg_malloc_extended with MCXT_ALLOC_NO_OOM)
*/
-extern char *pg_strdup(const char *in);
-extern void *pg_malloc(size_t size);
-extern void *pg_malloc0(size_t size);
+extern pg_returns_nonnull char *pg_strdup(const char *in);
+extern pg_returns_nonnull void *pg_malloc(size_t size);
+extern pg_returns_nonnull void *pg_malloc0(size_t size);
extern void *pg_malloc_extended(size_t size, int flags);
-extern void *pg_realloc(void *ptr, size_t size);
+extern pg_returns_nonnull void *pg_realloc(void *ptr, size_t size);
extern void pg_free(void *ptr);
/*
@@ -52,12 +54,12 @@ extern void pg_free(void *ptr);
#define pg_realloc_array(pointer, type, count) ((type *) pg_realloc(pointer, sizeof(type) * (count)))
/* Equivalent functions, deliberately named the same as backend functions */
-extern char *pstrdup(const char *in);
-extern char *pnstrdup(const char *in, Size size);
-extern void *palloc(Size size);
-extern void *palloc0(Size size);
+extern pg_returns_nonnull char *pstrdup(const char *in);
+extern pg_returns_nonnull char *pnstrdup(const char *in, Size size);
+extern pg_returns_nonnull void *palloc(Size size);
+extern pg_returns_nonnull void *palloc0(Size size);
extern void *palloc_extended(Size size, int flags);
-extern void *repalloc(void *pointer, Size size);
+extern pg_returns_nonnull void *repalloc(void *pointer, Size size);
extern void pfree(void *pointer);
#define palloc_object(type) ((type *) palloc(sizeof(type)))
@@ -67,7 +69,7 @@ extern void pfree(void *pointer);
#define repalloc_array(pointer, type, count) ((type *) repalloc(pointer, sizeof(type) * (count)))
/* sprintf into a palloc'd buffer --- these are in psprintf.c */
-extern char *psprintf(const char *fmt,...) pg_attribute_printf(1, 2);
+extern pg_returns_nonnull char *psprintf(const char *fmt,...) pg_attribute_printf(1, 2);
extern size_t pvsnprintf(char *buf, size_t len, const char *fmt, va_list args) pg_attribute_printf(3, 0);
#endif /* FE_MEMUTILS_H */
diff --git a/src/include/utils/palloc.h b/src/include/utils/palloc.h
index cf98ddc0ec..6f5a9bf259 100644
--- a/src/include/utils/palloc.h
+++ b/src/include/utils/palloc.h
@@ -68,21 +68,21 @@ extern PGDLLIMPORT MemoryContext CurrentMemoryContext;
/*
* Fundamental memory-allocation operations (more are in utils/memutils.h)
*/
-extern void *MemoryContextAlloc(MemoryContext context, Size size);
-extern void *MemoryContextAllocZero(MemoryContext context, Size size);
+extern pg_returns_nonnull void *MemoryContextAlloc(MemoryContext context, Size size);
+extern pg_returns_nonnull void *MemoryContextAllocZero(MemoryContext context, Size size);
extern void *MemoryContextAllocExtended(MemoryContext context,
Size size, int flags);
extern void *MemoryContextAllocAligned(MemoryContext context,
Size size, Size alignto, int flags);
-extern void *palloc(Size size);
-extern void *palloc0(Size size);
+extern pg_returns_nonnull void *palloc(Size size);
+extern pg_returns_nonnull void *palloc0(Size size);
extern void *palloc_extended(Size size, int flags);
extern void *palloc_aligned(Size size, Size alignto, int flags);
-extern pg_nodiscard void *repalloc(void *pointer, Size size);
+extern pg_nodiscard pg_returns_nonnull void *repalloc(void *pointer, Size size);
extern pg_nodiscard void *repalloc_extended(void *pointer,
Size size, int flags);
-extern pg_nodiscard void *repalloc0(void *pointer, Size oldsize, Size size);
+extern pg_nodiscard pg_returns_nonnull void *repalloc0(void *pointer, Size oldsize, Size size);
extern void pfree(void *pointer);
/*
@@ -109,8 +109,8 @@ extern void pfree(void *pointer);
#define repalloc0_array(pointer, type, oldcount, count) ((type *) repalloc0(pointer, sizeof(type) * (oldcount), sizeof(type) * (count)))
/* Higher-limit allocators. */
-extern void *MemoryContextAllocHuge(MemoryContext context, Size size);
-extern pg_nodiscard void *repalloc_huge(void *pointer, Size size);
+extern pg_returns_nonnull void *MemoryContextAllocHuge(MemoryContext context, Size size);
+extern pg_nodiscard pg_returns_nonnull void *repalloc_huge(void *pointer, Size size);
/*
* Although this header file is nominally backend-only, certain frontend
@@ -120,7 +120,7 @@ extern pg_nodiscard void *repalloc_huge(void *pointer, Size size);
*/
#ifndef FRONTEND
-static inline MemoryContext
+static inline MemoryContext pg_returns_nonnull
MemoryContextSwitchTo(MemoryContext context)
{
MemoryContext old = CurrentMemoryContext;
@@ -138,14 +138,14 @@ extern void MemoryContextRegisterResetCallback(MemoryContext context,
* These are like standard strdup() except the copied string is
* allocated in a context, not with malloc().
*/
-extern char *MemoryContextStrdup(MemoryContext context, const char *string);
-extern char *pstrdup(const char *in);
-extern char *pnstrdup(const char *in, Size len);
+extern pg_returns_nonnull char *MemoryContextStrdup(MemoryContext context, const char *string);
+extern pg_returns_nonnull char *pstrdup(const char *in);
+extern pg_returns_nonnull char *pnstrdup(const char *in, Size len);
-extern char *pchomp(const char *in);
+extern pg_returns_nonnull char *pchomp(const char *in);
/* sprintf into a palloc'd buffer --- these are in psprintf.c */
-extern char *psprintf(const char *fmt,...) pg_attribute_printf(1, 2);
+extern char *psprintf(const char *fmt,...) pg_attribute_printf(1, 2) pg_returns_nonnull;
extern size_t pvsnprintf(char *buf, size_t len, const char *fmt, va_list args) pg_attribute_printf(3, 0);
#endif /* PALLOC_H */
diff --git a/src/include/utils/pg_locale.h b/src/include/utils/pg_locale.h
index 6447bea8e0..61692a47fe 100644
--- a/src/include/utils/pg_locale.h
+++ b/src/include/utils/pg_locale.h
@@ -61,7 +61,7 @@ extern bool lc_ctype_is_c(Oid collation);
* Return the POSIX lconv struct (contains number/money formatting
* information) with locale information for all categories.
*/
-extern struct lconv *PGLC_localeconv(void);
+extern pg_returns_nonnull struct lconv *PGLC_localeconv(void);
extern void cache_locale_time(void);
--
Tristan Partin
Neon (https://neon.tech)
