Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Undefined behaviour on 32-bit platforms #412

Closed
sjakobi opened this issue Apr 14, 2022 · 3 comments · Fixed by #413
Closed

Undefined behaviour on 32-bit platforms #412

sjakobi opened this issue Apr 14, 2022 · 3 comments · Fixed by #413
Labels

Comments

@sjakobi
Copy link
Member

sjakobi commented Apr 14, 2022

-- | A bitmask with the 'bitsPerSubkey' least significant bits set.
fullNodeMask :: Bitmap
fullNodeMask = complement (complement 0 `unsafeShiftL` maxChildren)
{-# INLINE fullNodeMask #-}

The docs for unsafeShiftL say:

Shift the argument left by the specified number of bits. The
result is undefined for negative shift amounts and shift amounts
greater or equal to the 'bitSize'.

With maxChildren being 32 since #317, the value of fullNodeMask seems to be subject to UB on 32-bit platforms.

@sjakobi
Copy link
Member Author

sjakobi commented Apr 14, 2022

I haven't found any other UB related to unsafeShift{L,R}.

For index, we can document the bounds on the Shift argument though.

-- | Mask out the 'bitsPerSubkey' bits used for indexing at this level
-- of the tree.
index :: Hash -> Shift -> Int
index w s = fromIntegral $ unsafeShiftR w s .&. subkeyMask
{-# INLINE index #-}

@sjakobi sjakobi added the bug label Apr 14, 2022
@treeowl
Copy link
Collaborator

treeowl commented Apr 14, 2022

Do things work out if we manually set the value to -1, or do other bitwise operations go screwy for other reasons? If a branching factor of 32 is problematic on 32-bit architectures, can't we just set it back to 16 for them?

@sjakobi
Copy link
Member Author

sjakobi commented Apr 14, 2022

Do things work out if we manually set the value to -1, or do other bitwise operations go screwy for other reasons?

I think some operations expect the "empty bitmap" to be 0, for example the loop in unionArrayBy.

To fix this issue, I'd try using shiftL instead of unsafeShiftL.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging a pull request may close this issue.

2 participants