<div dir="rtl"><div dir="ltr">Thanks, this code will not be reached in 32 bits as the assert will fail but that won't hold in release config, the shift code should probably be in a conditional of both NumBaseBits and MaskWords, as was the original code. It should be still possible to merge them but more carefully.</div><div dir="ltr"><br></div></div><div class="gmail_extra"><br><div class="gmail_quote"><div dir="ltr">2015-09-18 15:20 GMT+03:00 Aaron Ballman <span dir="ltr"><<a href="mailto:aaron@aaronballman.com" target="_blank">aaron@aaronballman.com</a>></span>:</div><blockquote class="gmail_quote" style="margin:0 0 0 .8ex;border-left:1px #ccc solid;padding-left:1ex"><div class="HOEnZb"><div class="h5">On Fri, Sep 18, 2015 at 2:35 AM, Yaron Keren via llvm-commits<br>
<<a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a>> wrote:<br>
> Author: yrnkrn<br>
> Date: Fri Sep 18 01:35:12 2015<br>
> New Revision: 247972<br>
><br>
> URL: <a href="http://llvm.org/viewvc/llvm-project?rev=247972&view=rev" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project?rev=247972&view=rev</a><br>
> Log:<br>
> Simplify SmallBitVector::applyMask by consolidating common code for 32-bit and 64-bit builds.<br>
> Extend mask value to 64 bits before taking its complement and assert when mask is<br>
> too large to apply in the small case (previously the extra words were silently ignored).<br>
><br>
> <a href="http://reviews.llvm.org/D11890" rel="noreferrer" target="_blank">http://reviews.llvm.org/D11890</a><br>
><br>
> Patch by James Touton!<br>
><br>
><br>
> Modified:<br>
> llvm/trunk/include/llvm/ADT/SmallBitVector.h<br>
> llvm/trunk/unittests/ADT/BitVectorTest.cpp<br>
><br>
> Modified: llvm/trunk/include/llvm/ADT/SmallBitVector.h<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallBitVector.h?rev=247972&r1=247971&r2=247972&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/include/llvm/ADT/SmallBitVector.h?rev=247972&r1=247971&r2=247972&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/include/llvm/ADT/SmallBitVector.h (original)<br>
> +++ llvm/trunk/include/llvm/ADT/SmallBitVector.h Fri Sep 18 01:35:12 2015<br>
> @@ -553,17 +553,15 @@ public:<br>
> private:<br>
> template<bool AddBits, bool InvertMask><br>
> void applyMask(const uint32_t *Mask, unsigned MaskWords) {<br>
> - if (NumBaseBits == 64 && MaskWords >= 2) {<br>
> - uint64_t M = Mask[0] | (uint64_t(Mask[1]) << 32);<br>
> - if (InvertMask) M = ~M;<br>
> - if (AddBits) setSmallBits(getSmallBits() | M);<br>
> - else setSmallBits(getSmallBits() & ~M);<br>
> - } else {<br>
> - uint32_t M = Mask[0];<br>
> - if (InvertMask) M = ~M;<br>
> - if (AddBits) setSmallBits(getSmallBits() | M);<br>
> - else setSmallBits(getSmallBits() & ~M);<br>
> + uintptr_t M = Mask[0];<br>
> + if (MaskWords != 1) {<br>
> + assert(NumBaseBits == 64 && MaskWords == 2 &&<br>
> + "Mask is larger than base!");<br>
> + M |= uintptr_t(Mask[1]) << 32;<br>
<br>
</div></div>This patch introduces undefined behavior on platforms where uintptr_t<br>
is 32-bits, such as on Windows when compiling with MSVC in 23-bit<br>
mode.<br>
<br>
67>E:\llvm\llvm\include\llvm/ADT/SmallBitVector.h(560): warning C4293:<br>
'<<': shift count negative or too big, undefined behavior (compiling<br>
source file E:\llvm\llvm\tools\clang\lib\StaticAnalyzer\Core\FunctionSummary.cpp)<br>
67> E:\llvm\llvm\include\llvm/ADT/SmallBitVector.h(521): note: see<br>
reference to function template instantiation 'void<br>
llvm::SmallBitVector::applyMask<true,false>(const uint32_t *,unsigned<br>
int)' being compiled (compiling source file<br>
E:\llvm\llvm\tools\clang\lib\StaticAnalyzer\Core\FunctionSummary.cpp)<br>
<br>
I am reverting the patch (and r247972) temporarily since this also<br>
generates a lot of warnings (everywhere SmallBitVector.h ends up being<br>
included from) in r247983.<br>
<span class="HOEnZb"><font color="#888888"><br>
~Aaron<br>
</font></span><div class="HOEnZb"><div class="h5"><br>
> }<br>
> + if (InvertMask) M = ~M;<br>
> + if (AddBits) setSmallBits(getSmallBits() | M);<br>
> + else setSmallBits(getSmallBits() & ~M);<br>
> }<br>
> };<br>
><br>
><br>
> Modified: llvm/trunk/unittests/ADT/BitVectorTest.cpp<br>
> URL: <a href="http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/BitVectorTest.cpp?rev=247972&r1=247971&r2=247972&view=diff" rel="noreferrer" target="_blank">http://llvm.org/viewvc/llvm-project/llvm/trunk/unittests/ADT/BitVectorTest.cpp?rev=247972&r1=247971&r2=247972&view=diff</a><br>
> ==============================================================================<br>
> --- llvm/trunk/unittests/ADT/BitVectorTest.cpp (original)<br>
> +++ llvm/trunk/unittests/ADT/BitVectorTest.cpp Fri Sep 18 01:35:12 2015<br>
> @@ -235,12 +235,12 @@ TYPED_TEST(BitVectorTest, PortableBitMas<br>
> const uint32_t Mask1[] = { 0x80000000, 6, 5 };<br>
><br>
> A.resize(10);<br>
> - A.setBitsInMask(Mask1, 3);<br>
> + A.setBitsInMask(Mask1, 2);<br>
> EXPECT_EQ(10u, A.size());<br>
> EXPECT_FALSE(A.test(0));<br>
><br>
> A.resize(32);<br>
> - A.setBitsInMask(Mask1, 3);<br>
> + A.setBitsInMask(Mask1, 2);<br>
> EXPECT_FALSE(A.test(0));<br>
> EXPECT_TRUE(A.test(31));<br>
> EXPECT_EQ(1u, A.count());<br>
><br>
><br>
> _______________________________________________<br>
> llvm-commits mailing list<br>
> <a href="mailto:llvm-commits@lists.llvm.org">llvm-commits@lists.llvm.org</a><br>
> <a href="http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits" rel="noreferrer" target="_blank">http://lists.llvm.org/cgi-bin/mailman/listinfo/llvm-commits</a><br>
</div></div></blockquote></div><br></div>