Re: Pointer subtraction with a null pointer - Mailing list pgsql-hackers

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