Обсуждение: Pointer subtraction with a null pointer
Several of Andres' buildfarm animals have recently started to whine
that "performing pointer subtraction with a null pointer has undefined
behavior" for assorted places in freepage.c.
From a mathematical standpoint, this astonishes me: "x - 0 = x" is a
tautology. So I'm a bit inclined to say "you're full of it" and disable
-Wnull-pointer-subtraction. On the other hand, all of the occurrences
are in calls of relptr_store with a constant-NULL third argument.
So we could silence them without too much pain by adjusting that macro
to special-case NULL. Or maybe we should change these call sites to do
something different, because this is surely abusing the intent of
relptr_store.
Thoughts?
regards, tom lane
I wrote: > Several of Andres' buildfarm animals have recently started to whine > that "performing pointer subtraction with a null pointer has undefined > behavior" for assorted places in freepage.c. Ah, sorry, I meant to include a link: https://buildfarm.postgresql.org/cgi-bin/show_stage_log.pl?nm=mylodon&dt=2022-03-26%2000%3A02%3A10&stg=make 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. kestrel and olingo are reporting it too, but they're new. regards, tom lane
Hi, On 2022-03-26 12:04:54 -0400, Tom Lane wrote: > Several of Andres' buildfarm animals have recently started to whine > that "performing pointer subtraction with a null pointer has undefined > behavior" for assorted places in freepage.c. > > From a mathematical standpoint, this astonishes me: "x - 0 = x" is a > tautology. I don't think that's quite what the warning is warning about. The C standard doesn't allow pointer arithmetic between arbitrary pointers, they have to be to the same "object" (plus a trailing array element). http://www.open-std.org/jtc1/sc22/wg14/www/docs/n1548.pdf 6.5.6 Additive operators, 8/9 When two pointers are subtracted, both shall point to elements of the same array object, or one past the last element of the array object; the result is the difference of the subscripts of the two array elements. NULL can never be part of the same "array object" or one past past the last element as the pointer it is subtracted from. Hence the undefined beaviour. > Or maybe we should change these call sites to do something different, > because this is surely abusing the intent of relptr_store. I think a relptr_zero(), relptr_setnull() or such would make sense. That'd get rid of the need for the cast as well. Greetings, Andres Freund
On Sat, 26 Mar 2022 at 12:24, Andres Freund <andres@anarazel.de> wrote:
NULL can never be part of the same "array object" or one past past the last
element as the pointer it is subtracted from. Hence the undefined beaviour.
Even more fundamentally, NULL is not 0 in any ordinary mathematical sense, even though it can be written 0 in source code and is often (but not always) represented in memory as an all-0s bit pattern. I'm not at all surprised to learn that arithmetic involving NULL is undefined.
Hi, 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. I'll update it to 14 soon, now that that's released. It still has that warning, so it's not going to help us avoid the warning. Greetings, Andres Freund
Andres Freund <andres@anarazel.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.
After more thought I agree that replacing these relptr_store calls
with something else would be the better solution. I'll prepare a
patch.
regards, tom lane
I wrote:
> Andres Freund <andres@anarazel.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. It's hard
to see how that isn't a flat-out compiler bug.
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().
regards, tom lane
Hi,
On 2022-03-26 13:23:34 -0400, Tom Lane wrote:
> I wrote:
> > Andres Freund <andres@anarazel.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
I wrote:
> clang is complaining about the subtraction despite it being inside
> a conditional arm that cannot be reached when val is null. It's hard
> to see how that isn't a flat-out compiler bug.
> 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().
I've confirmed that the attached silences the warning with clang
13.0.0 (on Fedora 35). The store_null notation is not awful, perhaps;
it makes those lines shorter and more readable.
I'm a bit less enthused about removing the conditional in relptr_store,
as that forces re-introducing it at a couple of call sites. Perhaps
we should leave relptr_store alone ... but then the reason for
relptr_store_null is hard to explain except as a workaround for a
broken compiler.
I changed the comment suggesting that you could use relptrs with the
"process address space" as a base, because that would presumably mean
base == NULL which is going to draw the same warning.
regards, tom lane
diff --git a/src/backend/utils/mmgr/freepage.c b/src/backend/utils/mmgr/freepage.c
index b26a295a4e..7af8ec2283 100644
--- a/src/backend/utils/mmgr/freepage.c
+++ b/src/backend/utils/mmgr/freepage.c
@@ -185,8 +185,8 @@ FreePageManagerInitialize(FreePageManager *fpm, char *base)
Size f;
relptr_store(base, fpm->self, fpm);
- relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
- relptr_store(base, fpm->btree_recycle, (FreePageSpanLeader *) NULL);
+ relptr_store_null(base, fpm->btree_root);
+ relptr_store_null(base, fpm->btree_recycle);
fpm->btree_depth = 0;
fpm->btree_recycle_count = 0;
fpm->singleton_first_page = 0;
@@ -198,7 +198,7 @@ FreePageManagerInitialize(FreePageManager *fpm, char *base)
#endif
for (f = 0; f < FPM_NUM_FREELISTS; f++)
- relptr_store(base, fpm->freelist[f], (FreePageSpanLeader *) NULL);
+ relptr_store_null(base, fpm->freelist[f]);
}
/*
@@ -596,7 +596,7 @@ FreePageBtreeCleanup(FreePageManager *fpm)
if (root->hdr.magic == FREE_PAGE_LEAF_MAGIC)
{
/* If root is a leaf, convert only entry to singleton range. */
- relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
+ relptr_store_null(base, fpm->btree_root);
fpm->singleton_first_page = root->u.leaf_key[0].first_page;
fpm->singleton_npages = root->u.leaf_key[0].npages;
}
@@ -608,7 +608,7 @@ FreePageBtreeCleanup(FreePageManager *fpm)
Assert(root->hdr.magic == FREE_PAGE_INTERNAL_MAGIC);
relptr_copy(fpm->btree_root, root->u.internal_key[0].child);
newroot = relptr_access(base, fpm->btree_root);
- relptr_store(base, newroot->hdr.parent, (FreePageBtree *) NULL);
+ relptr_store_null(base, newroot->hdr.parent);
}
FreePageBtreeRecycle(fpm, fpm_pointer_to_page(base, root));
}
@@ -634,8 +634,7 @@ FreePageBtreeCleanup(FreePageManager *fpm)
fpm->singleton_npages = root->u.leaf_key[0].npages +
root->u.leaf_key[1].npages + 1;
fpm->btree_depth = 0;
- relptr_store(base, fpm->btree_root,
- (FreePageBtree *) NULL);
+ relptr_store_null(base, fpm->btree_root);
FreePagePushSpanLeader(fpm, fpm->singleton_first_page,
fpm->singleton_npages);
Assert(max_contiguous_pages == 0);
@@ -886,8 +885,12 @@ FreePageBtreeGetRecycled(FreePageManager *fpm)
Assert(victim != NULL);
newhead = relptr_access(base, victim->next);
if (newhead != NULL)
+ {
relptr_copy(newhead->prev, victim->prev);
- relptr_store(base, fpm->btree_recycle, newhead);
+ relptr_store(base, fpm->btree_recycle, newhead);
+ }
+ else
+ relptr_store_null(base, fpm->btree_recycle);
Assert(fpm_pointer_is_page_aligned(base, victim));
fpm->btree_recycle_count--;
return (FreePageBtree *) victim;
@@ -940,8 +943,11 @@ FreePageBtreeRecycle(FreePageManager *fpm, Size pageno)
span = (FreePageSpanLeader *) fpm_page_to_pointer(base, pageno);
span->magic = FREE_PAGE_SPAN_LEADER_MAGIC;
span->npages = 1;
- relptr_store(base, span->next, head);
- relptr_store(base, span->prev, (FreePageSpanLeader *) NULL);
+ if (head != NULL)
+ relptr_store(base, span->next, head);
+ else
+ relptr_store_null(base, span->next);
+ relptr_store_null(base, span->prev);
if (head != NULL)
relptr_store(base, head->prev, span);
relptr_store(base, fpm->btree_recycle, span);
@@ -998,7 +1004,7 @@ FreePageBtreeRemovePage(FreePageManager *fpm, FreePageBtree *btp)
if (parent == NULL)
{
/* We are removing the root page. */
- relptr_store(base, fpm->btree_root, (FreePageBtree *) NULL);
+ relptr_store_null(base, fpm->btree_root);
fpm->btree_depth = 0;
Assert(fpm->singleton_first_page == 0);
Assert(fpm->singleton_npages == 0);
@@ -1537,7 +1543,7 @@ FreePageManagerPutInternal(FreePageManager *fpm, Size first_page, Size npages,
/* Create the btree and move the preexisting range into it. */
root->hdr.magic = FREE_PAGE_LEAF_MAGIC;
root->hdr.nused = 1;
- relptr_store(base, root->hdr.parent, (FreePageBtree *) NULL);
+ relptr_store_null(base, root->hdr.parent);
root->u.leaf_key[0].first_page = fpm->singleton_first_page;
root->u.leaf_key[0].npages = fpm->singleton_npages;
relptr_store(base, fpm->btree_root, root);
@@ -1773,8 +1779,7 @@ FreePageManagerPutInternal(FreePageManager *fpm, Size first_page, Size npages,
newroot = FreePageBtreeGetRecycled(fpm);
newroot->hdr.magic = FREE_PAGE_INTERNAL_MAGIC;
newroot->hdr.nused = 2;
- relptr_store(base, newroot->hdr.parent,
- (FreePageBtree *) NULL);
+ relptr_store_null(base, newroot->hdr.parent);
newroot->u.internal_key[0].first_page =
FreePageBtreeFirstKey(split_target);
relptr_store(base, newroot->u.internal_key[0].child,
@@ -1878,8 +1883,11 @@ FreePagePushSpanLeader(FreePageManager *fpm, Size first_page, Size npages)
span = (FreePageSpanLeader *) fpm_page_to_pointer(base, first_page);
span->magic = FREE_PAGE_SPAN_LEADER_MAGIC;
span->npages = npages;
- relptr_store(base, span->next, head);
- relptr_store(base, span->prev, (FreePageSpanLeader *) NULL);
+ if (head != NULL)
+ relptr_store(base, span->next, head);
+ else
+ relptr_store_null(base, span->next);
+ relptr_store_null(base, span->prev);
if (head != NULL)
relptr_store(base, head->prev, span);
relptr_store(base, fpm->freelist[f], span);
diff --git a/src/include/utils/relptr.h b/src/include/utils/relptr.h
index fdc2124d2c..15e47467a7 100644
--- a/src/include/utils/relptr.h
+++ b/src/include/utils/relptr.h
@@ -15,13 +15,16 @@
#define RELPTR_H
/*
- * Relative pointers are intended to be used when storing an address that may
- * be relative either to the base of the process's address space or some
- * dynamic shared memory segment mapped therein.
+ * Relative pointers are intended to be used when storing an address that
+ * is relative to the start of a dynamic memory segment, so that it can
+ * be interpreted by different processes that have the DSM mapped at
+ * different places in their address space.
*
* The idea here is that you declare a relative pointer as relptr(type)
- * and then use relptr_access to dereference it and relptr_store to change
- * it. The use of a union here is a hack, because what's stored in the
+ * and then use relptr_access to dereference it and relptr_store or
+ * relptr_store_null to change it.
+ *
+ * The use of a union here is a hack, because what's stored in the
* relptr is always a Size, never an actual pointer. But including a pointer
* in the union allows us to use stupid macro tricks to provide some measure
* of type-safety.
@@ -56,11 +59,16 @@
#define relptr_is_null(rp) \
((rp).relptr_off == 0)
+#define relptr_store_null(base, rp) \
+ (AssertVariableIsOfTypeMacro(base, char *), \
+ (rp).relptr_off = 0)
+
#ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P
#define relptr_store(base, rp, val) \
(AssertVariableIsOfTypeMacro(base, char *), \
AssertVariableIsOfTypeMacro(val, __typeof__((rp).relptr_type)), \
- (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
+ AssertMacro((val) != NULL), \
+ (rp).relptr_off = ((char *) (val)) - (base))
#else
/*
* If we don't have __builtin_types_compatible_p, assume we might not have
@@ -68,7 +76,8 @@
*/
#define relptr_store(base, rp, val) \
(AssertVariableIsOfTypeMacro(base, char *), \
- (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
+ AssertMacro((val) != NULL), \
+ (rp).relptr_off = ((char *) (val)) - (base))
#endif
#define relptr_copy(rp1, rp2) \
Hi,
On 2022-03-26 10:49:53 -0700, Andres Freund wrote:
> > 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.
Reported as https://github.com/llvm/llvm-project/issues/54570
Greetings,
Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Wonder if we should try to get rid of the problem by also fixing the double
> evaluation of val? I think something like
Good idea. The attached also silences the warning, and getting rid
of the double-eval hazard seems like a net win.
> Might also be worth adding an assertion that base < val.
Did that too. On the whole I like this better.
regards, tom lane
diff --git a/src/include/utils/relptr.h b/src/include/utils/relptr.h
index fdc2124d2c..cc80a7200d 100644
--- a/src/include/utils/relptr.h
+++ b/src/include/utils/relptr.h
@@ -56,11 +56,24 @@
#define relptr_is_null(rp) \
((rp).relptr_off == 0)
+/* We use this inline to avoid double eval of "val" in relptr_store */
+static inline Size
+relptr_store_eval(char *base, char *val)
+{
+ if (val == NULL)
+ return 0;
+ else
+ {
+ Assert(val > base);
+ return val - base;
+ }
+}
+
#ifdef HAVE__BUILTIN_TYPES_COMPATIBLE_P
#define relptr_store(base, rp, val) \
(AssertVariableIsOfTypeMacro(base, char *), \
AssertVariableIsOfTypeMacro(val, __typeof__((rp).relptr_type)), \
- (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
+ (rp).relptr_off = relptr_store_eval(base, (char *) (val)))
#else
/*
* If we don't have __builtin_types_compatible_p, assume we might not have
@@ -68,7 +81,7 @@
*/
#define relptr_store(base, rp, val) \
(AssertVariableIsOfTypeMacro(base, char *), \
- (rp).relptr_off = ((val) == NULL ? 0 : ((char *) (val)) - (base)))
+ (rp).relptr_off = relptr_store_eval(base, (char *) (val)))
#endif
#define relptr_copy(rp1, rp2) \
Hi, On 2022-03-26 14:13:56 -0400, Tom Lane wrote: > The attached also silences the warning, and getting rid of the double-eval > hazard seems like a net win. Looks good wrt relptr_store. Maybe we should fix the double-eval hazard in relptr_access too, think that'd be only one left over... > > Might also be worth adding an assertion that base < val. > > Did that too. On the whole I like this better. Better than the relptr_store_null() approach I assume? Agreed, if so. Greetings, Andres Freund
Andres Freund <andres@anarazel.de> writes:
> Looks good wrt relptr_store. Maybe we should fix the double-eval hazard in
> relptr_access too, think that'd be only one left over...
Hm. Probably not worth the trouble, because it's hard to envision
a situation where rp is not a plain lvalue.
regards, tom lane
On 2022-03-26 11:08:59 -0700, Andres Freund wrote:
> On 2022-03-26 10:49:53 -0700, Andres Freund wrote:
> > > 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.
>
> Reported as https://github.com/llvm/llvm-project/issues/54570
And it now got fixed. Will obviously be a bit till it reaches a compiler near
you...