Replacing to use of ul suffix in GetMaxU64Bitfield since it not guarenteed to be 64 bit

[shafik]

Summary

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).

[teemperor]

LGTM minus a small nitpick. Thanks!

lldb/source/Utility/DataExtractor.cpp

595

How about std::numeric_limits<uint64_t>::max()?

[shafik]

Changing -static_cast<uint64_t>(1) to std::numeric_limits<uint64_t>::max()

lldb/source/Utility/DataExtractor.cpp

595

Good call, I thought about this change too.

lldb/source/Utility/DataExtractor.cpp

 int64_t DataExtractor::GetMaxS64(offset_t *offset_ptr, size_t byte_size) const {
   uint64_t u64 = GetMaxU64(offset_ptr, byte_size);
   return llvm::SignExtend64(u64, 8 * byte_size);
 }
 
 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;
+
+
+
+  uint64_t bitfield_mask =
+      (bitfield_bit_size == 64
+           ? std::numeric_limits<uint64_t>::max()
+           : ((static_cast<uint64_t>(1) << bitfield_bit_size) - 1));
+
+
   return uval64;
 }
 
 int64_t DataExtractor::GetMaxS64Bitfield(offset_t *offset_ptr, size_t size,
                                          uint32_t bitfield_bit_size,
                                          uint32_t bitfield_bit_offset) const {
   int64_t sval64 = GetMaxS64(offset_ptr, size);
   if (bitfield_bit_size > 0) {

lldb/unittests/Utility/DataExtractorTest.cpp

 //===-- DataExtractorTest.cpp -----------------------------------*- C++ -*-===//
 //
 // Part of the LLVM Project, under the Apache License v2.0 with LLVM Exceptions.
 // See https://llvm.org/LICENSE.txt for license information.
 // SPDX-License-Identifier: Apache-2.0 WITH LLVM-exception
 //
 //===----------------------------------------------------------------------===//
 
 #include "gtest/gtest.h"
 
 #include "lldb/Utility/DataExtractor.h"
 
 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 *));
 
   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>(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));
 }