shafik created this revision. shafik added reviewers: aprantl, teemperor, JDevlieghere.
`GetMaxU64Bitfield(...)` uses the `ul` suffix but we require a 64 bit unsigned integer and `ul` could be 32 bit. So this replacing it with a explicit cast and refactors the code around it to use an early exit. This bug came up in [[lldb-dev] Possible bug in DataExtractor::GetMaxU64Bitfield](http://lists.llvm.org/pipermail/lldb-dev/2019-November/015780.html). https://reviews.llvm.org/D70992 Files: lldb/source/Utility/DataExtractor.cpp lldb/unittests/Utility/DataExtractorTest.cpp Index: lldb/unittests/Utility/DataExtractorTest.cpp =================================================================== --- lldb/unittests/Utility/DataExtractorTest.cpp +++ lldb/unittests/Utility/DataExtractorTest.cpp @@ -13,7 +13,7 @@ using namespace lldb_private; TEST(DataExtractorTest, GetBitfield) { - uint8_t buffer[] = {0x01, 0x23, 0x45, 0x67}; + uint8_t buffer[] = {0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF}; DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle, sizeof(void *)); DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *)); @@ -24,6 +24,15 @@ ASSERT_EQ(buffer[1], LE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8)); offset = 0; ASSERT_EQ(buffer[1], BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8)); + offset = 0; + ASSERT_EQ(static_cast<uint64_t>(0x0123456789ABCDEF), + BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 64, 0)); + offset = 0; + ASSERT_EQ(static_cast<uint64_t>(0x01234567), + BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 32, 0)); + offset = 0; + ASSERT_EQ(static_cast<uint64_t>(0x012345678), + BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 36, 0)); offset = 0; ASSERT_EQ(int8_t(buffer[1]), Index: lldb/source/Utility/DataExtractor.cpp =================================================================== --- lldb/source/Utility/DataExtractor.cpp +++ lldb/source/Utility/DataExtractor.cpp @@ -577,18 +577,28 @@ uint64_t DataExtractor::GetMaxU64Bitfield(offset_t *offset_ptr, size_t size, uint32_t bitfield_bit_size, uint32_t bitfield_bit_offset) const { + assert(bitfield_bit_size <= 64); uint64_t uval64 = GetMaxU64(offset_ptr, size); - if (bitfield_bit_size > 0) { - int32_t lsbcount = bitfield_bit_offset; - if (m_byte_order == eByteOrderBig) - lsbcount = size * 8 - bitfield_bit_offset - bitfield_bit_size; - if (lsbcount > 0) - uval64 >>= lsbcount; - uint64_t bitfield_mask = ((1ul << bitfield_bit_size) - 1); - if (!bitfield_mask && bitfield_bit_offset == 0 && bitfield_bit_size == 64) - return uval64; - uval64 &= bitfield_mask; - } + + if (bitfield_bit_size == 0) + return uval64; + + int32_t lsbcount = bitfield_bit_offset; + if (m_byte_order == eByteOrderBig) + lsbcount = size * 8 - bitfield_bit_offset - bitfield_bit_size; + + if (lsbcount > 0) + uval64 >>= lsbcount; + + uint64_t bitfield_mask = + (bitfield_bit_size == 64 + ? -static_cast<uint64_t>(1) + : ((static_cast<uint64_t>(1) << bitfield_bit_size) - 1)); + if (!bitfield_mask && bitfield_bit_offset == 0 && bitfield_bit_size == 64) + return uval64; + + uval64 &= bitfield_mask; + return uval64; }
Index: lldb/unittests/Utility/DataExtractorTest.cpp =================================================================== --- lldb/unittests/Utility/DataExtractorTest.cpp +++ lldb/unittests/Utility/DataExtractorTest.cpp @@ -13,7 +13,7 @@ using namespace lldb_private; TEST(DataExtractorTest, GetBitfield) { - uint8_t buffer[] = {0x01, 0x23, 0x45, 0x67}; + uint8_t buffer[] = {0x01, 0x23, 0x45, 0x67, 0x89, 0xAB, 0xCD, 0xEF}; DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle, sizeof(void *)); DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig, sizeof(void *)); @@ -24,6 +24,15 @@ ASSERT_EQ(buffer[1], LE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8)); offset = 0; ASSERT_EQ(buffer[1], BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8)); + offset = 0; + ASSERT_EQ(static_cast<uint64_t>(0x0123456789ABCDEF), + BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 64, 0)); + offset = 0; + ASSERT_EQ(static_cast<uint64_t>(0x01234567), + BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 32, 0)); + offset = 0; + ASSERT_EQ(static_cast<uint64_t>(0x012345678), + BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 36, 0)); offset = 0; ASSERT_EQ(int8_t(buffer[1]), Index: lldb/source/Utility/DataExtractor.cpp =================================================================== --- lldb/source/Utility/DataExtractor.cpp +++ lldb/source/Utility/DataExtractor.cpp @@ -577,18 +577,28 @@ uint64_t DataExtractor::GetMaxU64Bitfield(offset_t *offset_ptr, size_t size, uint32_t bitfield_bit_size, uint32_t bitfield_bit_offset) const { + assert(bitfield_bit_size <= 64); uint64_t uval64 = GetMaxU64(offset_ptr, size); - if (bitfield_bit_size > 0) { - int32_t lsbcount = bitfield_bit_offset; - if (m_byte_order == eByteOrderBig) - lsbcount = size * 8 - bitfield_bit_offset - bitfield_bit_size; - if (lsbcount > 0) - uval64 >>= lsbcount; - uint64_t bitfield_mask = ((1ul << bitfield_bit_size) - 1); - if (!bitfield_mask && bitfield_bit_offset == 0 && bitfield_bit_size == 64) - return uval64; - uval64 &= bitfield_mask; - } + + if (bitfield_bit_size == 0) + return uval64; + + int32_t lsbcount = bitfield_bit_offset; + if (m_byte_order == eByteOrderBig) + lsbcount = size * 8 - bitfield_bit_offset - bitfield_bit_size; + + if (lsbcount > 0) + uval64 >>= lsbcount; + + uint64_t bitfield_mask = + (bitfield_bit_size == 64 + ? -static_cast<uint64_t>(1) + : ((static_cast<uint64_t>(1) << bitfield_bit_size) - 1)); + if (!bitfield_mask && bitfield_bit_offset == 0 && bitfield_bit_size == 64) + return uval64; + + uval64 &= bitfield_mask; + return uval64; }
_______________________________________________ lldb-commits mailing list lldb-commits@lists.llvm.org https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits