[m-rev.] for post-commit review: Fix sparse_bitset.divide_by_set.

Peter Wang novalazy at gmail.com
Thu Oct 13 17:34:22 AEDT 2016


The helper predicate `divide_bits_by_set' compares two bit vectors
(words) at the same offset, but it was incorrectly passed an absolute
offset instead of the offset *within* the two words it compares.
That led a call to unchecked_left_shift:

        OffsetBit = unchecked_left_shift(1, Offset)

which compiles to C:

        OffsetBit = 1 << Offset;

whose behaviour is undefined when Offset >= bits_per_int.

The correct expression should be

        OffsetBit = 1 << OffsetInWord;

(strictly speaking, this still relies on undefined behaviour because we
may shift into the sign bit)

It so happens that the incorrect code produces expected value of OffsetBit
on x86 and x86_64 because the left shift instructions mask the shift count
to the lower 5 or 6 bits (for 32-bit and 64-bit words, respectively),
and because the base offset of each bitset_elem is always a multiple
of bits_per_int.

The incorrect code also happens to work when targeting Java and C#
because those languages define their left shift operators to mask the
shift count in the same way.

library/sparse_bitset.m:
    As above.
---
 library/sparse_bitset.m | 12 ++++++------
 1 file changed, 6 insertions(+), 6 deletions(-)

diff --git a/library/sparse_bitset.m b/library/sparse_bitset.m
index 5a8edb0..f381816 100644
--- a/library/sparse_bitset.m
+++ b/library/sparse_bitset.m
@@ -1487,7 +1487,7 @@ divide_nodes_by_set([DivideByNode | DivideByNodes], [Node | Nodes],
         OutNodes = [Node | OutNodesTail]
     else
         divide_nodes_by_set(DivideByNodes, Nodes, InNodesTail, OutNodesTail),
-        divide_bits_by_set(DivideByBits, Offset, Bits, bits_per_int,
+        divide_bits_by_set(DivideByBits, 0, Bits, bits_per_int,
             0, In, 0, Out),
         ( if In = 0 then
             InNodes = InNodesTail
@@ -1506,11 +1506,11 @@ divide_nodes_by_set([DivideByNode | DivideByNodes], [Node | Nodes],
 :- pred divide_bits_by_set(int::in,
     int::in, int::in, int::in, int::in, int::out, int::in, int::out) is det.
 
-divide_bits_by_set(DivideByBits, Offset, Bits, Size, !In, !Out) :-
+divide_bits_by_set(DivideByBits, OffsetInWord, Bits, Size, !In, !Out) :-
     ( if Bits = 0 then
         true
     else if Size = 1 then
-        OffsetBit = unchecked_left_shift(1, Offset),
+        OffsetBit = unchecked_left_shift(1, OffsetInWord),
         ( if DivideByBits /\ OffsetBit = 0 then
             !:Out = !.Out \/ OffsetBit
         else
@@ -1526,10 +1526,10 @@ divide_bits_by_set(DivideByBits, Offset, Bits, Size, !In, !Out) :-
         % Extract the high-order half of the bits.
         HighBits = Mask /\ unchecked_right_shift(Bits, HalfSize),
 
-        divide_bits_by_set(DivideByBits, Offset, LowBits, HalfSize,
+        divide_bits_by_set(DivideByBits, OffsetInWord, LowBits, HalfSize,
             !In, !Out),
-        divide_bits_by_set(DivideByBits, Offset + HalfSize, HighBits, HalfSize,
-            !In, !Out)
+        divide_bits_by_set(DivideByBits, OffsetInWord + HalfSize, HighBits,
+            HalfSize, !In, !Out)
     ).
 
 %---------------------------------------------------------------------------%
-- 
2.9.0



More information about the reviews mailing list