Hi, On 2025-04-25 15:58:29 -0400, Andres Freund wrote: > On 2025-04-25 13:37:15 -0400, Tom Lane wrote: > > Buildfarm member serinus has been producing the identical warning for > > some time. I'd been ignoring that because it runs "experimental gcc", > > but I guess the experiment has leaked out to production distros. > > > > What seems to be happening here is that after inlining > > assign_simple_var into exec_set_found, the compiler decides that > > "newvalue" might be zero (since it's a BoolGetDatum result), > > and then it warns -- in a rather strange way -- about the > > potential null dereference. > > I don't think it actually is complaining about a null dereference - it thinks > we're interpreting a boolean as a pointer (for which it obviously is not wide > enough) > > > > The dereference is not reachable > > because of the preceding "var->datatype->typlen == -1" check, > > but that's not stopping the optimizer from bitching. > > > I experimented with modifying exec_set_found thus: > > > > var = (PLpgSQL_var *) (estate->datums[estate->found_varno]); > > + Assert(var->datatype->typlen == 1); > > assign_simple_var(estate, var, BoolGetDatum(state), false, false); > > > > which should be OK since we're expecting the "found" variable to > > be boolean. That does silence the warning, but of course only > > in --enable-cassert builds. > > One way to address this is outlined here: > > https://postgr.es/m/20230316172818.x6375uvheom3ibt2%40awork3.anarazel.de > https://postgr.es/m/20240207203138.sknifhlppdtgtxnk%40awork3.anarazel.de > > I've been wondering about adding wrapping something like that in a > pg_assume(expr) or such.
I've been once more annoyed by this warning. Here's a prototype for the approach outlined above. Greetings, Andres Freund
>From 147e2058e94338802ebc03bcfa7639d3c6bde75a Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Wed, 4 Jun 2025 14:28:19 -0400 Subject: [PATCH v1 1/2] Add pg_assume(expr) macro This macro can be used to avoid compiler warnings, particularly when using -O3 and not using assertions, and to get the compiler to generate better code. A subsequent commit introduces a first user. Discussion: https://postgr.es/m/3prdb6hkep3duglhsujrn52bkvnlkvhc54fzvph2emrsm4vodl@77yy6j4hkemb Discussion: https://postgr.es/m/20230316172818.x6375uvheom3ibt2%40awork3.anarazel.de Discussion: https://postgr.es/m/20240207203138.sknifhlppdtgtxnk%40awork3.anarazel.de --- src/include/c.h | 30 ++++++++++++++++++++++++++++++ 1 file changed, 30 insertions(+) diff --git a/src/include/c.h b/src/include/c.h index 8cdc16a0f4a..318c8ba978f 100644 --- a/src/include/c.h +++ b/src/include/c.h @@ -332,6 +332,36 @@ #define pg_unreachable() abort() #endif +/* + * pg_assume(expr) stats that we assume `expr` to evaluate to true. In assert + * enabled builds pg_assume() is turned into an assertion, in optimized builds + * we try to clue the compiler into the fact that `expr` is true. + * + * This is useful for two purposes: + * + * 1) Avoid compiler warnings by telling the compiler about assumptions the + * code makes. This is particularly useful when building with optimizations + * and w/o assertions. + * + * 2) Help the compiler to generate more efficient code + * + * It is unspecified whether `expr` is evaluated, therefore it better be + * side-effect free. + */ +#if defined(USE_ASSERT_CHECKING) +#define pg_assume(expr) Assert(expr) +#elif defined(HAVE__BUILTIN_UNREACHABLE) +#define pg_assume(expr) \ + do { \ + if (!(expr)) \ + __builtin_unreachable(); \ + } while (0) +#elif defined(_MSC_VER) +#define pg_assume(expr) __assume(expr) +#else +#define pg_assume(expr) ((void) 0) +#endif + /* * Hints to the compiler about the likelihood of a branch. Both likely() and * unlikely() return the boolean value of the contained expression. -- 2.48.1.76.g4e746b1a31.dirty
>From 3fe01158335d43ed133755aaffd41270c5e7af1a Mon Sep 17 00:00:00 2001 From: Andres Freund <and...@anarazel.de> Date: Wed, 4 Jun 2025 14:33:53 -0400 Subject: [PATCH v1 2/2] Use pg_assume() to avoid compiler warning below exec_set_found() The warning, visible when building with -O3 and a recent-ish gcc, is due to gcc not realizing that found is a byvalue type and therefore will never be interpreted as a varlena type. Discussion: https://postgr.es/m/3prdb6hkep3duglhsujrn52bkvnlkvhc54fzvph2emrsm4vodl@77yy6j4hkemb Discussion: https://postgr.es/m/20230316172818.x6375uvheom3ibt2%40awork3.anarazel.de Discussion: https://postgr.es/m/20240207203138.sknifhlppdtgtxnk%40awork3.anarazel.de --- src/pl/plpgsql/src/pl_exec.c | 9 +++++++++ 1 file changed, 9 insertions(+) diff --git a/src/pl/plpgsql/src/pl_exec.c b/src/pl/plpgsql/src/pl_exec.c index bb99781c56e..ecac29e830b 100644 --- a/src/pl/plpgsql/src/pl_exec.c +++ b/src/pl/plpgsql/src/pl_exec.c @@ -8606,6 +8606,15 @@ exec_set_found(PLpgSQL_execstate *estate, bool state) PLpgSQL_var *var; var = (PLpgSQL_var *) (estate->datums[estate->found_varno]); + + /* + * The pg_assume() avoids a spurious warning with some compilers due to + * compiler not realizing the VARATT_IS_EXTERNAL_NON_EXPANDED() branch in + * assign_simple_var() will never be reached, due to "found" being a + * boolean, a byvalue type. + */ + pg_assume(var->datatype->typlen != -1); + assign_simple_var(estate, var, BoolGetDatum(state), false, false); } -- 2.48.1.76.g4e746b1a31.dirty
