Fix infinite loop in isSubmapOf[By] / isSubsetOf on 32-bit platforms

[sjakobi]

The internal submapBitmapIndexed function used by these functions could enter an infinite loop while comparing two nodes m1 and m2 where m2 contained a sub-node associated with the partial hash 0b11111. In this case the high bit of the combined bitmap of m1 and m2, b1Orb2, was 1. After checking the submap condition at the high bit, the 32-bit m variable used to iterate over b1Orb2 would be left-shifted and overflow to 0, resulting in the infinite loop.

unordered-containers/Data/HashMap/Internal.hs

Lines 1551 to 1572 in 37eee22

	-- | \(O(\min n m))\) Checks if a bitmap indexed node is a submap of another.
	submapBitmapIndexed :: (HashMap k v1 -> HashMap k v2 -> Bool) -> Bitmap -> A.Array (HashMap k v1) -> Bitmap -> A.Array (HashMap k v2) -> Bool
	submapBitmapIndexed comp !b1 !ary1 !b2 !ary2 = subsetBitmaps && go 0 0 (b1Orb2 .&. negate b1Orb2)
	where
	go :: Int -> Int -> Bitmap -> Bool
	go !i !j !m
	| m > b1Orb2 = True
	-- In case a key is both in ary1 and ary2, check ary1[i] <= ary2[j] and
	-- increment the indices i and j.
	| b1Andb2 .&. m /= 0 = comp (A.index ary1 i) (A.index ary2 j) &&
	go (i+1) (j+1) (m `unsafeShiftL` 1)
	-- In case a key occurs in ary1, but not ary2, only increment index j.
	| b2 .&. m /= 0 = go i (j+1) (m `unsafeShiftL` 1)
	-- In case a key neither occurs in ary1 nor ary2, continue.
	| otherwise = go i j (m `unsafeShiftL` 1)
	b1Andb2 = b1 .&. b2
	b1Orb2 = b1 .|. b2
	subsetBitmaps = b1Orb2 == b2

To fix this bug and any similar issues related to a branching factor of 32, we return to a branching factor of 16 for 32-bit platforms. On 64-bit platforms the branching factors stays at 32.

Fixes #491.

---

TODO:

• Add CI on a 32bit platform
• Add fix for isSubmapOfBy
• Remove -fno-omit-yields option

[treeowl]

Oh dear ... now I understand why we need a rather full audit for 32-bit systems. Or: we could go back to using a smaller branching factor on those systems.

[sjakobi]

Or: we could go back to using a smaller branching factor on those systems.

Yeah, that seems like a good solution to me.

I think the share of users on 32bit platforms is very small these days and will continue to shrink. I wasn't too keen on hunting for issues that would affect only this particular user group.

I will make a patch so bitsPerSubkey is 4 on platforms where the word size is < 64 bits.

[sjakobi]

I think this is ready for review now.

I'll clean up the commit history before merging!

[treeowl]

The fix looks reasonable. But are there tests you wrote that should be put back in?

[sjakobi]

But are there tests you wrote that should be put back in?

Look at tests/Regressions.hs!

For debugging, I had also added some timeouts to the property tests for isSubmapOf, but I felt they they weren't necessary anymore, now that the tests are fixed.