[lldb/DataExtractor] Fix UB shift in GetMaxS64Bitfield

[vsk]

Summary

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

[shafik]

I wish I had caught that one when I did D70992, might be worth checking out more of the code for similar issues.

[shafik]

lldb/source/Utility/DataExtractor.cpp

610

We could do an early exit if bitfield_bit_size is zero.

[davide]

In D73913#1855486, @shafik wrote:

I wish I had caught that one when I did D70992, might be worth checking out more of the code for similar issues.

I guess this is why we have tooling =)

[davide]

LGTM, thanks for taking care of this.

[vsk]

lldb/source/Utility/DataExtractor.cpp

610

Yes, I think that'd simplify things. I'll do this in a follow up commit.

lldb/include/lldb/Utility/DataExtractor.h

public:
   ///
   /// \return
   ///     The unsigned bitfield integer value that was extracted, or
   ///     zero on failure.
   uint64_t GetMaxU64Bitfield(lldb::offset_t *offset_ptr, size_t size,
                              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.
   ///
   /// \param[in,out] offset_ptr
   ///     A pointer to an offset within the data that will be advanced
   ///     by the appropriate number of bytes if the value is extracted
   ///     correctly. If the offset is out of bounds or there are not
   ///     enough bytes to extract this value, the offset will be left

lldb/source/Utility/DataExtractor.cpp

uint64_t DataExtractor::GetMaxU64Bitfield(offset_t *offset_ptr, size_t size,
   uval64 &= bitfield_mask;
 
   return uval64;
 }
 
 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;
     if (m_byte_order == eByteOrderBig)
       lsbcount = size * 8 - bitfield_bit_offset - bitfield_bit_size;
     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)))
       sval64 |= ~bitfield_mask;
   }
   return sval64;
 }
 

lldb/unittests/Utility/DataExtractorTest.cpp

TEST(DataExtractorTest, GetBitfield) {
 
   lldb::offset_t offset;
 
   offset = 0;
   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>(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;
   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]),
             LE.GetMaxS64Bitfield(&offset, sizeof(buffer), 8, 8));
   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) {
   uint8_t buffer[] = {0x01, 0x02, 0x03, 0x04};
   DataExtractor E(buffer, sizeof buffer, lldb::eByteOrderLittle, 4);
 
   EXPECT_EQ(buffer + 0, E.PeekData(0, 0));
   EXPECT_EQ(buffer + 0, E.PeekData(0, 4));