[Lldb-commits] [PATCH] D83256: [lldb] Fix unaligned load in DataExtractor

[Pavel Labath via Phabricator via lldb-commits]

labath added a comment.

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.



================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:991-1001
+    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 < src_size; ++i)
+        dst_data[src_size - 1 - i] = src_data[i];
+      return val;
+    } else {
----------------
```
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.


================
Comment at: lldb/source/Utility/DataExtractor.cpp:630
 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.0f);
 }
----------------
In this case the `f` suffix is actually not appropriate.
`f` -> `float` literal
no suffix -> `double` literal
`L` -> `long double` literal


================
Comment at: lldb/unittests/Utility/DataExtractorTest.cpp:380
+  {
+    uint8_t buffer[] = {0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x00, 0x10, 0x40};
+    DataExtractor LE(buffer, sizeof(buffer), lldb::eByteOrderLittle,
----------------
clayborg wrote:
> 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?
> 
> 
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.


CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D83256/new/

https://reviews.llvm.org/D83256