[lldb] Replace the LEB128 decoding logic in LLDB's DataExtractor with calls to LLVM's LEB128 implementation

[teemperor]

• Fixed include ordering.

[labath]

lldb/source/Utility/DataExtractor.cpp

884

Ummm.. why don't you just pass m_end here? The last argument is the end of the buffer, not the end of the number...

[teemperor]

lldb/source/Utility/DataExtractor.cpp

884

From what I can see decodeSLEB128 doesn't have any detection logic for ints > 64bits, so it would just keep reading data until it finds something that looks like the terminating byte (or the end of the buffer) in case of malformed input. But if we explicitly limit the buffer to 10 bytes then it will stop and set a reasonable error message (even though we don't use that yet in LLDB). I did the same for decodeULEB128 just for the sake of symmetry (as that function curiously figures out this error case on its own).

I'm fine with just passing m_end here though (that would make this patch more NFC as we currently also don't handle malformed input).

[labath]

lldb/source/Utility/DataExtractor.cpp

884

If we're going to be using that function, I think we ought to rely on it to determine what constitutes a malformed LEB128. Also, the current version has a bug in that it does not check whether src + max_int64_size is still inside the data extractor range.

[teemperor]

• Pass m_end as buffer end instead.

[teemperor]

lldb/source/Utility/DataExtractor.cpp

884

Yeah, I think you're right. Updated the patch, thanks!

[JDevlieghere]

Looks good

lldb/source/Utility/DataExtractor.cpp

911

Should we hoist this into llvm as well (as a separate patch). It seems like at least lld/ELF/EhFrame.cpp is using the same functionality.

[labath]

lldb/source/Utility/DataExtractor.cpp

911

TBH, I'm tempted to just delete it. I doubt that "skipping" an LEB128 is going to be noticeably faster than "reading" it, and llvm::DataExtractor has no equivalent function...

[JDevlieghere]

lldb/source/Utility/DataExtractor.cpp

911

Even better :-)

lldb/source/Utility/DataExtractor.cpp

 #include "lldb/Utility/LLDBAssert.h"
 #include "lldb/Utility/Log.h"
 #include "lldb/Utility/Stream.h"
 #include "lldb/Utility/StreamString.h"
 #include "lldb/Utility/UUID.h"
 
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/ADT/SmallVector.h"
+#include "llvm/Support/LEB128.h"
 #include "llvm/Support/MD5.h"
 #include "llvm/Support/MathExtras.h"
 
 #include <algorithm>
 #include <array>
 #include <cassert>
 #include <cstdint>
 #include <string>

 // byte.
 //
 // Returned the extracted integer value.
 uint64_t DataExtractor::GetULEB128(offset_t *offset_ptr) const {
   const uint8_t *src = PeekData(*offset_ptr, 1);
   if (src == nullptr)
     return 0;
 
-  const uint8_t *end = m_end;
-
-  if (src < end) {
-    uint64_t result = *src++;
-    if (result >= 0x80) {
-      result &= 0x7f;
-      int shift = 7;
-      while (src < end) {
-        uint8_t byte = *src++;
-        result |= static_cast<uint64_t>(byte & 0x7f) << shift;
-        if ((byte & 0x80) == 0)
-          break;
-        shift += 7;
-      }
-    }
-    *offset_ptr = src - m_start;
-    return result;
-  }
+  unsigned byte_count = 0;
+  uint64_t result = llvm::decodeULEB128(src, &byte_count, m_end);
+  *offset_ptr += byte_count;
 
-  return 0;
-}
-
 // Extracts an signed LEB128 number from this object's data starting at the
 // offset pointed to by "offset_ptr". The offset pointed to by "offset_ptr"
 // will be updated with the offset of the byte following the last extracted
 // byte.
 //
 // Returned the extracted integer value.
 int64_t DataExtractor::GetSLEB128(offset_t *offset_ptr) const {
   const uint8_t *src = PeekData(*offset_ptr, 1);
   if (src == nullptr)
     return 0;
 
-  const uint8_t *end = m_end;
-
-  if (src < end) {
-    int64_t result = 0;
-    int shift = 0;
-    int size = sizeof(int64_t) * 8;
-
-    uint8_t byte = 0;
-    int bytecount = 0;
-
-    while (src < end) {
-      bytecount++;
-      byte = *src++;
-      result |= static_cast<int64_t>(byte & 0x7f) << shift;
-      shift += 7;
-      if ((byte & 0x80) == 0)
-        break;
-    }
-
-    // Sign bit of byte is 2nd high order bit (0x40)
-    if (shift < size && (byte & 0x40)) {
-      // -(static_cast<int64_t>(1) << 63) errors on the negation with UBSan.
-      result |= -(static_cast<uint64_t>(1) << shift);
-    }
-
-    *offset_ptr += bytecount;
-    return result;
-  }
-  return 0;
-}
+  unsigned byte_count = 0;
+  int64_t result = llvm::decodeSLEB128(src, &byte_count, m_end);
+  *offset_ptr += byte_count;
 
 // Skips a ULEB128 number (signed or unsigned) from this object's data starting
 // at the offset pointed to by "offset_ptr". The offset pointed to by
 // "offset_ptr" will be updated with the offset of the byte following the last
 // extracted byte.
 //
 // Returns the number of bytes consumed during the extraction.
 uint32_t DataExtractor::Skip_LEB128(offset_t *offset_ptr) const {
   uint32_t bytes_consumed = 0;
   const uint8_t *src = PeekData(*offset_ptr, 1);
   if (src == nullptr)
     return 0;
 
   const uint8_t *end = m_end;
 
   if (src < end) {
     const uint8_t *src_pos = src;