Re: suspicious valgrind reports about radixtree/tidstore on arm64 - Mailing list pgsql-hackers

From Tom Lane
Subject Re: suspicious valgrind reports about radixtree/tidstore on arm64
Date
Msg-id 248275.1718841112@sss.pgh.pa.us
Whole thread Raw
In response to Re: suspicious valgrind reports about radixtree/tidstore on arm64  (Tom Lane <tgl@sss.pgh.pa.us>)
Responses Re: suspicious valgrind reports about radixtree/tidstore on arm64
List pgsql-hackers
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

==00:00:00:32.759 247596== Conditional jump or move depends on uninitialised value(s)
==00:00:00:32.759 247596==    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

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 chunk)
     /* convert comparison to a bitfield */
     bitfield = vector8_highbit_mask(cmp1) | (vector8_highbit_mask(cmp2) << sizeof(Vector8));

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

     Assert(slot_simd == slot);
     return slot_simd;

pgsql-hackers by date:

Previous
From: Tom Lane
Date:
Subject: Re: suspicious valgrind reports about radixtree/tidstore on arm64
Next
From: Michael Paquier
Date:
Subject: Re: Pluggable cumulative statistics