Re: Pointer subtraction with a null pointer

Hi,

On 2022-03-26 13:23:34 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres(at)anarazel(dot)de> writes:
> >> On 2022-03-26 12:13:12 -0400, Tom Lane wrote:
> >>> This code is old, but mylodon wasn't doing that a week ago, so
> >>> Andres must've updated the compiler and/or changed its options.
>
> >> Yep, updated it to clang 13. It's a warning present in 13, but not in 12.
>
> > OK, that answers that.
>
> ... Actually, after looking closer, I misread what our code is doing.
> These call sites are trying to set the relptr value to "null" (zero),
> and AFAICS it should be allowed:
>
> freepage.c:188:2: warning: performing pointer subtraction with a null pointer has undefined behavior [-Wnull-pointer-subtraction]
> relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
> ^~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~~
> ../../../../src/include/utils/relptr.h:63:59: note: expanded from macro 'relptr_store'
> (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
> ~~~~~~~~~~~~~~~~ ^
>
> clang is complaining about the subtraction despite it being inside
> a conditional arm that cannot be reached when val is null.

Huh, yea. I somehow read the conditional branch as guarding against a an
uninitialized base pointer or such.

> It's hard to see how that isn't a flat-out compiler bug.

It only happens if the NULL is directly passed as an argument to the macro,
not if there's an intermediary variable. Argh.

#include <stddef.h>

#define relptr_store(base, rp, val) \
((rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))

typedef union { struct foo *relptr_type; size_t relptr_off; } relptr;

void
problem_not_present(relptr *rp, char *base)
{
struct foo *val = NULL;

relptr_store(base, *rp, val);
}

void
problem_present(relptr *rp, char *base)
{
relptr_store(base, *rp, NULL);
}

Looks like that warning is uttered whenever there's a subtraction from a
pointer with NULL, even if the code isn't reachable. Which I guess makes
*some* sense, outside of macros it's not something that'd ever be reasonable.

Wonder if we should try to get rid of the problem by also fixing the double
evaluation of val? I think something like

static inline void
relptr_store_internal(size_t *off, char *base, char *val)
{
if (val == NULL)
*off = 0;
else
*off = val - base;
}

#ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P
#define relptr_store(base, rp, val) \
(AssertVariableIsOfTypeMacro(base, char *), \
AssertVariableIsOfTypeMacro(val, __typeof__((rp).relptr_type)), \
relptr_store_internal(&(rp).relptr_off, base, (char *) val))
#else
...

should do the trick?

Might also be worth adding an assertion that base < val.

> However, granting that it isn't going to get fixed right away,
> we could replace these call sites with "relptr_store_null()",
> and maybe get rid of the conditional in relptr_store().

Also would be good with that.

Greetings,

Andres Freund