Received: from malur.postgresql.org ([217.196.149.56]) by arkaria.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nYAZH-0008UY-SY for pgsql-hackers@arkaria.postgresql.org; Sat, 26 Mar 2022 17:51:16 +0000 Received: from localhost ([127.0.0.1] helo=malur.postgresql.org) by malur.postgresql.org with esmtp (Exim 4.92) (envelope-from ) id 1nYAZG-0004i3-KI for pgsql-hackers@arkaria.postgresql.org; Sat, 26 Mar 2022 17:51:14 +0000 Received: from makus.postgresql.org ([2001:4800:3e1:1::229]) by malur.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nYAZG-0004hu-6w for pgsql-hackers@lists.postgresql.org; Sat, 26 Mar 2022 17:51:14 +0000 Received: from sss.pgh.pa.us ([66.207.139.130]) by makus.postgresql.org with esmtps (TLS1.3:ECDHE_RSA_AES_256_GCM_SHA384:256) (Exim 4.92) (envelope-from ) id 1nYAZD-0006a7-I9 for pgsql-hackers@lists.postgresql.org; Sat, 26 Mar 2022 17:51:13 +0000 Received: from sss1.sss.pgh.pa.us (localhost [127.0.0.1]) by sss.pgh.pa.us (8.15.2/8.15.2) with ESMTP id 22QHp7v6088127; Sat, 26 Mar 2022 13:51:07 -0400 From: Tom Lane To: Andres Freund cc: pgsql-hackers@lists.postgresql.org, Robert Haas , Thomas Munro Subject: Re: Pointer subtraction with a null pointer 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> Comments: In-reply-to Tom Lane message dated "Sat, 26 Mar 2022 13:23:34 -0400" MIME-Version: 1.0 Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0" Content-ID: <87947.1648316942.0@sss.pgh.pa.us> Date: Sat, 26 Mar 2022 13:51:07 -0400 Message-ID: <88126.1648317067@sss.pgh.pa.us> List-Id: List-Help: List-Subscribe: List-Post: List-Owner: List-Archive: Archived-At: Precedence: bulk ------- =_aaaaaaaaaa0 Content-Type: text/plain; charset="us-ascii" Content-ID: <87947.1648316942.1@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 ------- =_aaaaaaaaaa0 Content-Type: text/x-diff; name="silence-null-subtraction-warning-1.patch"; charset="us-ascii" Content-ID: <87947.1648316942.2@sss.pgh.pa.us> Content-Description: silence-null-subtraction-warning-1.patch Content-Transfer-Encoding: quoted-printable diff --git a/src/backend/utils/mmgr/freepage.c b/src/backend/utils/mmgr/fr= eepage.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 =3D 0; fpm->btree_recycle_count =3D 0; fpm->singleton_first_page =3D 0; @@ -198,7 +198,7 @@ FreePageManagerInitialize(FreePageManager *fpm, char *= base) #endif = for (f =3D 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 =3D=3D 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 =3D root->u.leaf_key[0].first_page; fpm->singleton_npages =3D root->u.leaf_key[0].npages; } @@ -608,7 +608,7 @@ FreePageBtreeCleanup(FreePageManager *fpm) Assert(root->hdr.magic =3D=3D FREE_PAGE_INTERNAL_MAGIC); relptr_copy(fpm->btree_root, root->u.internal_key[0].child); newroot =3D 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 =3D root->u.leaf_key[0].npages + root->u.leaf_key[1].npages + 1; fpm->btree_depth =3D 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 =3D=3D 0); @@ -886,8 +885,12 @@ FreePageBtreeGetRecycled(FreePageManager *fpm) Assert(victim !=3D NULL); newhead =3D relptr_access(base, victim->next); if (newhead !=3D 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 pagen= o) span =3D (FreePageSpanLeader *) fpm_page_to_pointer(base, pageno); span->magic =3D FREE_PAGE_SPAN_LEADER_MAGIC; span->npages =3D 1; - relptr_store(base, span->next, head); - relptr_store(base, span->prev, (FreePageSpanLeader *) NULL); + if (head !=3D NULL) + relptr_store(base, span->next, head); + else + relptr_store_null(base, span->next); + relptr_store_null(base, span->prev); if (head !=3D NULL) relptr_store(base, head->prev, span); relptr_store(base, fpm->btree_recycle, span); @@ -998,7 +1004,7 @@ FreePageBtreeRemovePage(FreePageManager *fpm, FreePag= eBtree *btp) if (parent =3D=3D 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 =3D 0; Assert(fpm->singleton_first_page =3D=3D 0); Assert(fpm->singleton_npages =3D=3D 0); @@ -1537,7 +1543,7 @@ FreePageManagerPutInternal(FreePageManager *fpm, Siz= e first_page, Size npages, /* Create the btree and move the preexisting range into it. */ root->hdr.magic =3D FREE_PAGE_LEAF_MAGIC; root->hdr.nused =3D 1; - relptr_store(base, root->hdr.parent, (FreePageBtree *) NULL); + relptr_store_null(base, root->hdr.parent); root->u.leaf_key[0].first_page =3D fpm->singleton_first_page; root->u.leaf_key[0].npages =3D fpm->singleton_npages; relptr_store(base, fpm->btree_root, root); @@ -1773,8 +1779,7 @@ FreePageManagerPutInternal(FreePageManager *fpm, Siz= e first_page, Size npages, newroot =3D FreePageBtreeGetRecycled(fpm); newroot->hdr.magic =3D FREE_PAGE_INTERNAL_MAGIC; newroot->hdr.nused =3D 2; - relptr_store(base, newroot->hdr.parent, - (FreePageBtree *) NULL); + relptr_store_null(base, newroot->hdr.parent); newroot->u.internal_key[0].first_page =3D FreePageBtreeFirstKey(split_target); relptr_store(base, newroot->u.internal_key[0].child, @@ -1878,8 +1883,11 @@ FreePagePushSpanLeader(FreePageManager *fpm, Size f= irst_page, Size npages) span =3D (FreePageSpanLeader *) fpm_page_to_pointer(base, first_page); span->magic =3D FREE_PAGE_SPAN_LEADER_MAGIC; span->npages =3D npages; - relptr_store(base, span->next, head); - relptr_store(base, span->prev, (FreePageSpanLeader *) NULL); + if (head !=3D NULL) + relptr_store(base, span->next, head); + else + relptr_store_null(base, span->next); + relptr_store_null(base, span->prev); if (head !=3D 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 chang= e - * 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 poi= nter * in the union allows us to use stupid macro tricks to provide some meas= ure * of type-safety. @@ -56,11 +59,16 @@ #define relptr_is_null(rp) \ ((rp).relptr_off =3D=3D 0) = +#define relptr_store_null(base, rp) \ + (AssertVariableIsOfTypeMacro(base, char *), \ + (rp).relptr_off =3D 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 =3D ((val) =3D=3D NULL ? 0 : ((char *) (val)) - (base))= ) + AssertMacro((val) !=3D NULL), \ + (rp).relptr_off =3D ((char *) (val)) - (base)) #else /* * If we don't have __builtin_types_compatible_p, assume we might not hav= e @@ -68,7 +76,8 @@ */ #define relptr_store(base, rp, val) \ (AssertVariableIsOfTypeMacro(base, char *), \ - (rp).relptr_off =3D ((val) =3D=3D NULL ? 0 : ((char *) (val)) - (base))= ) + AssertMacro((val) !=3D NULL), \ + (rp).relptr_off =3D ((char *) (val)) - (base)) #endif = #define relptr_copy(rp1, rp2) \ ------- =_aaaaaaaaaa0--