From: Tom Lane <tgl@sss.pgh.pa.us>
To: Tomas Vondra <tomas.vondra@enterprisedb.com>
cc: PostgreSQL Hackers <pgsql-hackers@lists.postgresql.org>,
        Masahiko Sawada <sawada.mshk@gmail.com>,
        John Naylor <johncnaylorls@gmail.com>
Subject: Re: suspicious valgrind reports about radixtree/tidstore on arm64
In-reply-to: <225667.1718837686@sss.pgh.pa.us>
References: <120c63ad-3d12-415f-a7bf-3da451c31bf6@enterprisedb.com> <115286.1718809897@sss.pgh.pa.us> <c87432b8-7643-4654-b26b-74398108c7ca@enterprisedb.com> <162580.1718831471@sss.pgh.pa.us> <225667.1718837686@sss.pgh.pa.us>
Comments: In-reply-to Tom Lane <tgl@sss.pgh.pa.us>
	message dated "Wed, 19 Jun 2024 18:54:46 -0400"
MIME-Version: 1.0
Content-Type: multipart/mixed; boundary="----- =_aaaaaaaaaa0"
Content-ID: <248229.1718841080.0@sss.pgh.pa.us>
Date: Wed, 19 Jun 2024 19:51:52 -0400
Message-ID: <248275.1718841112@sss.pgh.pa.us>
Archived-At: <https://www.postgresql.org/message-id/248275.1718841112%40sss.pgh.pa.us>
Precedence: bulk

------- =_aaaaaaaaaa0
Content-Type: text/plain; charset="us-ascii"
Content-ID: <248229.1718841080.1@sss.pgh.pa.us>
Content-Transfer-Encoding: quoted-printable

I wrote:
> I hypothesize that the reason we're not seeing equivalent failures
> on x86_64 is one of

> 1. x86_64 valgrind is stupider than aarch64, and fails to track that
> the contents of the SIMD registers are only partially defined.

> 2. x86_64 valgrind is smarter than aarch64, and is able to see
> that the "mask off invalid entries" step removes all the
> potentially-uninitialized bits.

Hah: it's the second case.  If I patch radixtree.h as attached,
then x86_64 valgrind complains about =


=3D=3D00:00:00:32.759 247596=3D=3D Conditional jump or move depends on uni=
nitialised value(s)
=3D=3D00:00:00:32.759 247596=3D=3D    at 0x52F668: local_ts_node_16_search=
_eq (radixtree.h:1018)

showing that it knows that the result of vector8_highbit_mask is
only partly defined.  Kind of odd though that aarch64 valgrind
is getting the hard part right and not the easy(?) part.

			regards, tom lane


------- =_aaaaaaaaaa0
Content-Type: text/x-diff;
	name="make-radixtree-failure-visible-on-x86_64.patch";
	charset="us-ascii"
Content-ID: <248229.1718841080.2@sss.pgh.pa.us>
Content-Description: make-radixtree-failure-visible-on-x86_64.patch
Content-Transfer-Encoding: quoted-printable

diff --git a/src/include/lib/radixtree.h b/src/include/lib/radixtree.h
index 338e1d741d..267ec6de03 100644
--- a/src/include/lib/radixtree.h
+++ b/src/include/lib/radixtree.h
@@ -1015,12 +1015,15 @@ RT_NODE_16_SEARCH_EQ(RT_NODE_16 * node, uint8 chun=
k)
 	/* convert comparison to a bitfield */
 	bitfield =3D vector8_highbit_mask(cmp1) | (vector8_highbit_mask(cmp2) <<=
 sizeof(Vector8));
 =

-	/* mask off invalid entries */
-	bitfield &=3D ((UINT64CONST(1) << count) - 1);
-
-	/* convert bitfield to index by counting trailing zeros */
 	if (bitfield)
-		slot_simd =3D &node->children[pg_rightmost_one_pos32(bitfield)];
+	{
+		/* mask off invalid entries */
+		bitfield &=3D ((UINT64CONST(1) << count) - 1);
+
+		/* convert bitfield to index by counting trailing zeros */
+		if (bitfield)
+			slot_simd =3D &node->children[pg_rightmost_one_pos32(bitfield)];
+	}
 =

 	Assert(slot_simd =3D=3D slot);
 	return slot_simd;

------- =_aaaaaaaaaa0--