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) \
