[llvm] r247972 - Simplify SmallBitVector::applyMask by consolidating common code for 32-bit and 64-bit builds.

[Aaron Ballman via llvm-commits]

On Fri, Sep 18, 2015 at 2:35 AM, Yaron Keren via llvm-commits
<llvm-commits at lists.llvm.org> wrote:
> Author: yrnkrn
> Date: Fri Sep 18 01:35:12 2015
> New Revision: 247972
>
> URL: http://llvm.org/viewvc/llvm-project?rev=247972&view=rev
> Log:
> Simplify SmallBitVector::applyMask by consolidating common code for 32-bit and 64-bit builds.
> Extend mask value to 64 bits before taking its complement and assert when mask is
> too large to apply in the small case (previously the extra words were silently ignored).
>
> http://reviews.llvm.org/D11890
>
> Patch by James Touton!
>
>
> Modified:
>     llvm/trunk/include/llvm/ADT/SmallBitVector.h
>     llvm/trunk/unittests/ADT/BitVectorTest.cpp
>
> Modified: llvm/trunk/include/llvm/ADT/SmallBitVector.h
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallBitVector.h?rev=247972&r1=247971&r2=247972&view=diff
> ==============================================================================
> --- llvm/trunk/include/llvm/ADT/SmallBitVector.h (original)
> +++ llvm/trunk/include/llvm/ADT/SmallBitVector.h Fri Sep 18 01:35:12 2015
> @@ -553,17 +553,15 @@ public:
>  private:
>    template<bool AddBits, bool InvertMask>
>    void applyMask(const uint32_t *Mask, unsigned MaskWords) {
> -    if (NumBaseBits == 64 && MaskWords >= 2) {
> -      uint64_t M = Mask[0] | (uint64_t(Mask[1]) << 32);
> -      if (InvertMask) M = ~M;
> -      if (AddBits) setSmallBits(getSmallBits() | M);
> -      else         setSmallBits(getSmallBits() & ~M);
> -    } else {
> -      uint32_t M = Mask[0];
> -      if (InvertMask) M = ~M;
> -      if (AddBits) setSmallBits(getSmallBits() | M);
> -      else         setSmallBits(getSmallBits() & ~M);
> +    uintptr_t M = Mask[0];
> +    if (MaskWords != 1) {
> +      assert(NumBaseBits == 64 && MaskWords == 2 &&
> +             "Mask is larger than base!");
> +      M |= uintptr_t(Mask[1]) << 32;

This patch introduces undefined behavior on platforms where uintptr_t
is 32-bits, such as on Windows when compiling with MSVC in 23-bit
mode.

67>E:\llvm\llvm\include\llvm/ADT/SmallBitVector.h(560): warning C4293:
'<<': shift count negative or too big, undefined behavior (compiling
source file E:\llvm\llvm\tools\clang\lib\StaticAnalyzer\Core\FunctionSummary.cpp)
67>  E:\llvm\llvm\include\llvm/ADT/SmallBitVector.h(521): note: see
reference to function template instantiation 'void
llvm::SmallBitVector::applyMask<true,false>(const uint32_t *,unsigned
int)' being compiled (compiling source file
E:\llvm\llvm\tools\clang\lib\StaticAnalyzer\Core\FunctionSummary.cpp)

I am reverting the patch (and r247972) temporarily since this also
generates a lot of warnings (everywhere SmallBitVector.h ends up being
included from) in r247983.

~Aaron

>      }
> +    if (InvertMask) M = ~M;
> +    if (AddBits) setSmallBits(getSmallBits() | M);
> +    else         setSmallBits(getSmallBits() & ~M);
>    }
>  };
>
>
> Modified: llvm/trunk/unittests/ADT/BitVectorTest.cpp
> URL: http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/BitVectorTest.cpp?rev=247972&r1=247971&r2=247972&view=diff
> ==============================================================================
> --- llvm/trunk/unittests/ADT/BitVectorTest.cpp (original)
> +++ llvm/trunk/unittests/ADT/BitVectorTest.cpp Fri Sep 18 01:35:12 2015
> @@ -235,12 +235,12 @@ TYPED_TEST(BitVectorTest, PortableBitMas
>    const uint32_t Mask1[] = { 0x80000000, 6, 5 };
>
>    A.resize(10);
> -  A.setBitsInMask(Mask1, 3);
> +  A.setBitsInMask(Mask1, 2);
>    EXPECT_EQ(10u, A.size());
>    EXPECT_FALSE(A.test(0));
>
>    A.resize(32);
> -  A.setBitsInMask(Mask1, 3);
> +  A.setBitsInMask(Mask1, 2);
>    EXPECT_FALSE(A.test(0));
>    EXPECT_TRUE(A.test(31));
>    EXPECT_EQ(1u, A.count());
>
>
> _______________________________________________
> llvm-commits mailing list
> llvm-commits at lists.llvm.org
> http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits