[lldb] Fix unaligned load in DataExtractor

[JDevlieghere]

Summary

Somehow UBSan would only report the unaligned load in TestLinuxCore.py when running the tests with reproducers. This patch fixes the issue by using a memcpy in GetDouble and GetFloat method.

[JDevlieghere]

This comment was removed by JDevlieghere.

[shafik]

lldb/include/lldb/Utility/DataExtractor.h

25

What platforms are we not expecting to have __builtin_is_aligned on?

[shafik]

lldb/include/lldb/Utility/DataExtractor.h

25

nvm, I just realized this is coming from clang not llvm.

[clayborg]

There is no requirement that data be aligned when stored in a buffer that DataExtractor points to, so this patch doesn't seem correct. Since GetData() calls can return unaligned pointers, the usual way to make this work is to to make a local variable and do a memcpy into that local variable. See the ReadSwapInt32() function to see how it is done for integers. Then we need to swap the bytes depending on the byte size. So maybe a function that can do byte swapping for any sized data can be made. Then we can call it from the Get<T> function. More comments inline.

[clayborg]

lldb/include/lldb/Utility/DataExtractor.h

25–29

Remove this because we don't require alignment for anything in a DataExtractor.

984

We should probably pass in the fail value if we want this to work for more than floats?

template <typename T> T Get(lldb::offset_t *offset_ptr, const T fail_value) const {

985

Make a local "T" here:

T val = fail_value;
constexpr size_t src_size = sizeof(T);

992

remove this, no alignment is required.

993

Add a memcpy call:

memcpy(&val, src, src_size);

995

remove since "val" was created above

996–1000

Might be nice to place this into a method function that swaps data of any size. Then other functions can re-use it if needed.

1000

return "return val;" here

1003

We can't rely on alignment, so return our local T value:

return val;

[JDevlieghere]

Thanks for the clarifications Greg!

[JDevlieghere]

lldb/include/lldb/Utility/DataExtractor.h

996–1000

I left it inline here so we can copy the data in place instead copy & swapping.

[clayborg]

Just need some DataExtractorTest.cpp additions to test the unaligned loads for float and doubles?

lldb/source/Utility/DataExtractor.cpp

626

nit: "0.0f" instead of "0.0"

[clayborg]

FYI: values like 4.0 or 2.25 work well for float values in tests as they encode perfectly on all systems.

[clayborg]

Here are the next values for 32 bit and 64 bit floats:

float a = 2.25; 0x40100000
double b = 2.25; 0x4002000000000000

You might disable this on windows as I believe they have 10 byte floats for doubles?

[JDevlieghere]

Add unit tests

[clayborg]

Just need some sanity checks to make sure the sizeof(float) == 4 and sizeof(double) == 8 for the tests to make sure this doesn't fail on systems where that isn't true.

lldb/unittests/Utility/DataExtractorTest.cpp

308

You might want to make check if the size of a float is 4 bytes here and return without testing anything if it isn't or this will fail.

332

You might want to make check if the size of a float is 4 bytes here and return without testing anything if it isn't or this will fail.

356

You might want to make check if the size of a double is 8 bytes here and return without testing anything if it isn't or this will fail. Windows used 10 byte floats for doubles I believe?

380

You might want to make check if the size of a double is 8 bytes here and return without testing anything if it isn't or this will fail. Windows used 10 byte floats for doubles I believe?

[JDevlieghere]

I was stil checking the Windows float thing but skipping them based on the sizeof() seems more robust.

[clayborg]

sizeof(double) instead of sizeof(float) for the double tests.

lldb/unittests/Utility/DataExtractorTest.cpp

358

sizeof(double)

385

sizeof(double)

[labath]

The approach is right, but can be simplified.

I'd also remove the sizeof checks in the tests. We have a lot of code depending on the fact that floats and doubles are consistent across targets (e.g. the swapByteOrder function I mention). If that ever becomes untrue it would be probably nice to get an early warning about it.

long doubles are a different story, as they behave differently on pretty much every platform. That's why GetLongDouble is full of ifdefs, and is still wrong, because normally we want to read the targets notion of a "long double" not that of a host. That's why I would ideally want to replace these float accessors with a single unified accessor like `llvm::APFloat GetFloat(uint64_t *, llvm::fltSemantics), but that's a story for another day.

lldb/include/lldb/Utility/DataExtractor.h

992–1002

memcpy(&val, src, src_size);
if (m_byte_order != endian::InlHostByteOrder())
  llvm::sys::swapByteOrder(val);
return val;

will only work float and double (and all integers..) but it doesn't like you need anything else anyway. Long double is broken enough to not care about it.

lldb/source/Utility/DataExtractor.cpp

630

In this case the f suffix is actually not appropriate.
f -> float literal
no suffix -> double literal
L -> long double literal

lldb/unittests/Utility/DataExtractorTest.cpp

380

Windows doesn't use 10 bytes for doubles (definitely not on x86, but I would be very surprised if it was true elsewhere too). You're probably confusing this with x86 "extended" floats, which some ABIs map to "long double". These contain 10 bytes of useful data, but due to alignment considerations their sizeof is usually 12 or 16.

lldb/include/lldb/Utility/DataExtractor.h

 //===-- DataExtractor.h -----------------------------------------*- 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
 //
 //===----------------------------------------------------------------------===//
 
 #ifndef LLDB_UTILITY_DATAEXTRACTOR_H
 #define LLDB_UTILITY_DATAEXTRACTOR_H
 
+#include "lldb/Utility/Endian.h"
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 #include "llvm/ADT/ArrayRef.h"
 #include "llvm/Support/DataExtractor.h"
+#include "llvm/Support/SwapByteOrder.h"
 
 #include <cassert>
 #include <stdint.h>
 #include <string.h>
 
 namespace lldb_private {
 class Log;
 class Stream;
 }
 namespace llvm {
 template <typename T> class SmallVectorImpl;
 }
 
 
 namespace lldb_private {
 
 /// \class DataExtractor DataExtractor.h "lldb/Core/DataExtractor.h" An data
 /// extractor class.
#endif
   }
 
   llvm::DataExtractor GetAsLLVM() const {
     return {GetData(), GetByteOrder() == lldb::eByteOrderLittle,
             uint8_t(GetAddressByteSize())};
   }
 
 protected:
+  template <typename T> T Get(lldb::offset_t *offset_ptr, T fail_value) const {
+    constexpr size_t src_size = sizeof(T);
+    T val = fail_value;
+
+    const T *src = static_cast<const T *>(GetData(offset_ptr, src_size));
+    if (!src)
+      return val;
+
+    memcpy(&val, src, src_size);
+    if (m_byte_order != endian::InlHostByteOrder())
+      llvm::sys::swapByteOrder(val);
+
+    return val;
+  }
+
   // Member variables
   const uint8_t *m_start; ///< A pointer to the first byte of data.
   const uint8_t
       *m_end; ///< A pointer to the byte that is past the end of the data.
   lldb::ByteOrder
       m_byte_order;     ///< The byte order of the data we are extracting from.
   uint32_t m_addr_size; ///< The address size to use when extracting addresses.
   /// The shared pointer to data that can be shared among multiple instances
   lldb::DataBufferSP m_data_sp;
   const uint32_t m_target_byte_size;
 };
 
 } // namespace lldb_private
 
 #endif // LLDB_UTILITY_DATAEXTRACTOR_H

lldb/source/Utility/DataExtractor.cpp

 
 #include "lldb/lldb-defines.h"
 #include "lldb/lldb-enumerations.h"
 #include "lldb/lldb-forward.h"
 #include "lldb/lldb-types.h"
 
 #include "lldb/Utility/DataBuffer.h"
 #include "lldb/Utility/DataBufferHeap.h"
-#include "lldb/Utility/Endian.h"
 #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"
int64_t DataExtractor::GetMaxS64Bitfield(offset_t *offset_ptr, size_t size,
   sval64 &= bitfield_mask;
   // sign extend if needed
   if (sval64 & ((static_cast<uint64_t>(1)) << (bitfield_bit_size - 1)))
     sval64 |= ~bitfield_mask;
   return sval64;
 }
 
 float DataExtractor::GetFloat(offset_t *offset_ptr) const {
-  typedef float float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-      static_cast<const float_type *>(GetData(offset_ptr, src_size));
-  if (src) {
-    if (m_byte_order != endian::InlHostByteOrder()) {
-      const uint8_t *src_data = reinterpret_cast<const uint8_t *>(src);
-      uint8_t *dst_data = reinterpret_cast<uint8_t *>(&val);
-      for (size_t i = 0; i < sizeof(float_type); ++i)
-        dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-    } else {
-      val = *src;
-    }
-  }
-  return val;
+  return Get<float>(offset_ptr, 0.0f);
 }
 
 double DataExtractor::GetDouble(offset_t *offset_ptr) const {
-  typedef double float_type;
-  float_type val = 0.0;
-  const size_t src_size = sizeof(float_type);
-  const float_type *src =
-      static_cast<const float_type *>(GetData(offset_ptr, src_size));
-  if (src) {
-    if (m_byte_order != endian::InlHostByteOrder()) {
-      const uint8_t *src_data = reinterpret_cast<const uint8_t *>(src);
-      uint8_t *dst_data = reinterpret_cast<uint8_t *>(&val);
-      for (size_t i = 0; i < sizeof(float_type); ++i)
-        dst_data[sizeof(float_type) - 1 - i] = src_data[i];
-    } else {
-      val = *src;
-    }
-  }
-  return val;
+  return Get<double>(offset_ptr, 0.0);
 }
 
 long double DataExtractor::GetLongDouble(offset_t *offset_ptr) const {
   long double val = 0.0;
 #if defined(__i386__) || defined(__amd64__) || defined(__x86_64__) ||          \
     defined(_M_IX86) || defined(_M_IA64) || defined(_M_X64)
   *offset_ptr += CopyByteOrderedData(*offset_ptr, 10, &val, sizeof(val),
                                      endian::InlHostByteOrder());

lldb/unittests/Utility/DataExtractorTest.cpp

uint64_t expected =
       0b0111111100000001111111000000011111110000000111111100000001111111;
   offset = 0;
   EXPECT_EQ(expected, LE.GetULEB128(&offset));
   EXPECT_EQ(9U, offset);
   offset = 0;
   EXPECT_EQ(expected, BE.GetULEB128(&offset));
   EXPECT_EQ(9U, offset);
 }
+
+TEST(DataExtractorTest, GetFloat) {
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+    uint8_t buffer[] = {0x00, 0x00, 0x80, 0x40};
+    DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+                     sizeof(void *));
+
+    offset = 0;
+    EXPECT_DOUBLE_EQ(expected, LE.GetFloat(&offset));
+    EXPECT_EQ(4U, offset);
+  }
+
+  {
+    uint8_t buffer[] = {0x40, 0x80, 0x00, 0x00};
+    DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+                     sizeof(void *));
+    offset = 0;
+    EXPECT_DOUBLE_EQ(expected, BE.GetFloat(&offset));
+    EXPECT_EQ(4U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetFloatUnaligned) {
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+    uint8_t buffer[] = {0x00, 0x00, 0x00, 0x80, 0x40};
+    DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+                     sizeof(void *));
+
+    offset = 1;
+    EXPECT_DOUBLE_EQ(expected, LE.GetFloat(&offset));
+    EXPECT_EQ(5U, offset);
+  }
+
+  {
+    uint8_t buffer[] = {0x00, 0x40, 0x80, 0x00, 0x00};
+    DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+                     sizeof(void *));
+    offset = 1;
+    EXPECT_DOUBLE_EQ(expected, BE.GetFloat(&offset));
+    EXPECT_EQ(5U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetDouble) {
+  if (sizeof(double) != 8)
+    return;
+
+  double expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+    uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+    DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+                     sizeof(void *));
+
+    offset = 0;
+    EXPECT_DOUBLE_EQ(expected, LE.GetDouble(&offset));
+    EXPECT_EQ(8U, offset);
+  }
+
+  {
+    uint8_t buffer[] = {0x40, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+    DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+                     sizeof(void *));
+    offset = 0;
+    EXPECT_DOUBLE_EQ(expected, BE.GetDouble(&offset));
+    EXPECT_EQ(8U, offset);
+  }
+}
+
+TEST(DataExtractorTest, GetDoubleUnaligned) {
+  if (sizeof(double) != 8)
+    return;
+
+  float expected = 4.0f;
+  lldb::offset_t offset;
+
+  {
+    uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+    DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
+                     sizeof(void *));
+
+    offset = 1;
+    EXPECT_DOUBLE_EQ(expected, LE.GetDouble(&offset));
+    EXPECT_EQ(9U, offset);
+  }
+
+  {
+    uint8_t buffer[] = {0x00, 0x40, 0x10, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00};
+    DataExtractor BE(buffer, sizeof(buffer), lldb::eByteOrderBig,
+                     sizeof(void *));
+    offset = 1;
+    EXPECT_DOUBLE_EQ(expected, BE.GetDouble(&offset));
+    EXPECT_EQ(9U, offset);
+  }
+}