From: Tom Lane <tgl@sss.pgh.pa.us>
To: Andres Freund <andres@anarazel.de>
Cc: pgsql-hackers@lists.postgresql.org, Robert Haas <robertmhaas@gmail.com>
Cc: Thomas Munro <thomas.munro@gmail.com>
Subject: Re: Pointer subtraction with a null pointer
Date: Sat, 26 Mar 2022 13:51:07 -0400
Message-ID: <88126.1648317067@sss.pgh.pa.us> (raw)
In-Reply-To: <55128.1648315414@sss.pgh.pa.us>
References: <48826.1648310694@sss.pgh.pa.us>
<49360.1648311192@sss.pgh.pa.us>
<20220326163441.qzpcioycozbzarsf@alap3.anarazel.de>
<51678.1648312664@sss.pgh.pa.us>
<55128.1648315414@sss.pgh.pa.us>
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
Attachments:
[text/x-diff] silence-null-subtraction-warning-1.patch (7.3K, ../88126.1648317067@sss.pgh.pa.us/2-silence-null-subtraction-warning-1.patch)
download | inline diff:diff --git a/src/backend/utils/mmgr/freepage.c b/src/backend/utils/mmgr/freepage.cindex 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.hindex 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) \
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Reply to all the recipients using the --to and --cc options:
reply via email
To: pgsql-hackers@postgresql.org
Cc: tgl@sss.pgh.pa.us, andres@anarazel.de, robertmhaas@gmail.com, thomas.munro@gmail.com
Subject: Re: Pointer subtraction with a null pointer
In-Reply-To: <88126.1648317067@sss.pgh.pa.us>
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
This inbox is served by DDX for PostgreSQL; see mirroring instructions
for how to clone and mirror all data and code used for this inbox