vsk created this revision.
vsk added reviewers: labath, JDevlieghere, teemperor.
DataExtractor::GetMaxS64Bitfield performs a shift with UB in order to
construct a bitmask when bitfield_bit_size is 64. The current
implementation actually does “work” in this case, because the assumption
that the shift result is 0 holds, and 0 minus 1 gives the all-ones value
(the correct mask). However, the more readable/maintainable approach
might be to use an off-the-shelf UB-free helper.
Fixes a UBSan issue:
"col" : 37,
"description" : "invalid-shift-exponent",
"filename" :
"/Users/vsk/src/llvm-project-master/lldb/source/Utility/DataExtractor.cpp",
"instrumentation_class" : "UndefinedBehaviorSanitizer",
"line" : 615,
"memory_address" : 0,
"summary" : "Shift exponent 64 is too large for 64-bit type 'uint64_t' (aka
'unsigned long long')",
rdar://59117758
https://reviews.llvm.org/D73913
Files:
lldb/include/lldb/Utility/DataExtractor.h
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
@@ -25,6 +25,9 @@
offset = 0;
ASSERT_EQ(buffer[1], BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8));
offset = 0;
+ ASSERT_EQ(static_cast<uint64_t>(0xEFCDAB8967452301),
+ LE.GetMaxU64Bitfield(&offset, sizeof(buffer), 64, 0));
+ offset = 0;
ASSERT_EQ(static_cast<uint64_t>(0x0123456789ABCDEF),
BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 64, 0));
offset = 0;
@@ -40,6 +43,12 @@
offset = 0;
ASSERT_EQ(int8_t(buffer[1]),
BE.GetMaxS64Bitfield(&offset, sizeof(buffer), 8, 8));
+ offset = 0;
+ ASSERT_EQ(static_cast<int64_t>(0xEFCDAB8967452301),
+ LE.GetMaxS64Bitfield(&offset, sizeof(buffer), 64, 0));
+ offset = 0;
+ ASSERT_EQ(static_cast<int64_t>(0x0123456789ABCDEF),
+ BE.GetMaxS64Bitfield(&offset, sizeof(buffer), 64, 0));
}
TEST(DataExtractorTest, PeekData) {
Index: lldb/source/Utility/DataExtractor.cpp
===================================================================
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -604,6 +604,8 @@
int64_t DataExtractor::GetMaxS64Bitfield(offset_t *offset_ptr, size_t size,
uint32_t bitfield_bit_size,
uint32_t bitfield_bit_offset) const {
+ assert(size >= 1 && "GetMaxS64Bitfield size must be >= 1");
+ assert(size <= 8 && "GetMaxS64Bitfield size must be <= 8");
int64_t sval64 = GetMaxS64(offset_ptr, size);
if (bitfield_bit_size > 0) {
int32_t lsbcount = bitfield_bit_offset;
@@ -612,7 +614,7 @@
if (lsbcount > 0)
sval64 >>= lsbcount;
uint64_t bitfield_mask =
- ((static_cast<uint64_t>(1)) << bitfield_bit_size) - 1;
+ llvm::maskTrailingOnes<uint64_t>(bitfield_bit_size);
sval64 &= bitfield_mask;
// sign extend if needed
if (sval64 & ((static_cast<uint64_t>(1)) << (bitfield_bit_size - 1)))
Index: lldb/include/lldb/Utility/DataExtractor.h
===================================================================
--- lldb/include/lldb/Utility/DataExtractor.h
+++ lldb/include/lldb/Utility/DataExtractor.h
@@ -535,13 +535,13 @@
uint32_t bitfield_bit_size,
uint32_t bitfield_bit_offset) const;
- /// Extract an signed integer of size \a byte_size from \a *offset_ptr, then
- /// extract and signe extend the bitfield from this value if \a
+ /// Extract an signed integer of size \a size from \a *offset_ptr, then
+ /// extract and sign-extend the bitfield from this value if \a
/// bitfield_bit_size is non-zero.
///
- /// Extract a single signed integer value (sign extending if required) and
+ /// Extract a single signed integer value (sign-extending if required) and
/// update the offset pointed to by \a offset_ptr. The size of the extracted
- /// integer is specified by the \a byte_size argument. \a byte_size must
+ /// integer is specified by the \a size argument. \a size must
/// have a value greater than or equal to one and less than or equal to
/// eight since the return value is 64 bits wide.
///
Index: lldb/unittests/Utility/DataExtractorTest.cpp
===================================================================
--- lldb/unittests/Utility/DataExtractorTest.cpp
+++ lldb/unittests/Utility/DataExtractorTest.cpp
@@ -25,6 +25,9 @@
offset = 0;
ASSERT_EQ(buffer[1], BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 8, 8));
offset = 0;
+ ASSERT_EQ(static_cast<uint64_t>(0xEFCDAB8967452301),
+ LE.GetMaxU64Bitfield(&offset, sizeof(buffer), 64, 0));
+ offset = 0;
ASSERT_EQ(static_cast<uint64_t>(0x0123456789ABCDEF),
BE.GetMaxU64Bitfield(&offset, sizeof(buffer), 64, 0));
offset = 0;
@@ -40,6 +43,12 @@
offset = 0;
ASSERT_EQ(int8_t(buffer[1]),
BE.GetMaxS64Bitfield(&offset, sizeof(buffer), 8, 8));
+ offset = 0;
+ ASSERT_EQ(static_cast<int64_t>(0xEFCDAB8967452301),
+ LE.GetMaxS64Bitfield(&offset, sizeof(buffer), 64, 0));
+ offset = 0;
+ ASSERT_EQ(static_cast<int64_t>(0x0123456789ABCDEF),
+ BE.GetMaxS64Bitfield(&offset, sizeof(buffer), 64, 0));
}
TEST(DataExtractorTest, PeekData) {
Index: lldb/source/Utility/DataExtractor.cpp
===================================================================
--- lldb/source/Utility/DataExtractor.cpp
+++ lldb/source/Utility/DataExtractor.cpp
@@ -604,6 +604,8 @@
int64_t DataExtractor::GetMaxS64Bitfield(offset_t *offset_ptr, size_t size,
uint32_t bitfield_bit_size,
uint32_t bitfield_bit_offset) const {
+ assert(size >= 1 && "GetMaxS64Bitfield size must be >= 1");
+ assert(size <= 8 && "GetMaxS64Bitfield size must be <= 8");
int64_t sval64 = GetMaxS64(offset_ptr, size);
if (bitfield_bit_size > 0) {
int32_t lsbcount = bitfield_bit_offset;
@@ -612,7 +614,7 @@
if (lsbcount > 0)
sval64 >>= lsbcount;
uint64_t bitfield_mask =
- ((static_cast<uint64_t>(1)) << bitfield_bit_size) - 1;
+ llvm::maskTrailingOnes<uint64_t>(bitfield_bit_size);
sval64 &= bitfield_mask;
// sign extend if needed
if (sval64 & ((static_cast<uint64_t>(1)) << (bitfield_bit_size - 1)))
Index: lldb/include/lldb/Utility/DataExtractor.h
===================================================================
--- lldb/include/lldb/Utility/DataExtractor.h
+++ lldb/include/lldb/Utility/DataExtractor.h
@@ -535,13 +535,13 @@
uint32_t bitfield_bit_size,
uint32_t bitfield_bit_offset) const;
- /// Extract an signed integer of size \a byte_size from \a *offset_ptr, then
- /// extract and signe extend the bitfield from this value if \a
+ /// Extract an signed integer of size \a size from \a *offset_ptr, then
+ /// extract and sign-extend the bitfield from this value if \a
/// bitfield_bit_size is non-zero.
///
- /// Extract a single signed integer value (sign extending if required) and
+ /// Extract a single signed integer value (sign-extending if required) and
/// update the offset pointed to by \a offset_ptr. The size of the extracted
- /// integer is specified by the \a byte_size argument. \a byte_size must
+ /// integer is specified by the \a size argument. \a size must
/// have a value greater than or equal to one and less than or equal to
/// eight since the return value is 64 bits wide.
///
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
