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

[Greg Clayton via Phabricator via lldb-commits]

clayborg requested changes to this revision.
clayborg added inline comments.
This revision now requires changes to proceed.


================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:24-28
+#ifndef __builtin_is_aligned
+#define __builtin_is_aligned(POINTER, BYTE_COUNT)                              \
+  (((uintptr_t)(const void *)(POINTER)) % (BYTE_COUNT) == 0)
+#endif
+
----------------
shafik wrote:
> shafik wrote:
> > What platforms are we not expecting to have `__builtin_is_aligned` on?
> nvm, I just realized this is coming from clang not llvm.
Remove this because we don't require alignment for anything in a DataExtractor.


================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:988
 protected:
+  template <typename T> T Get(lldb::offset_t *offset_ptr) const {
+    constexpr size_t src_size = sizeof(T);
----------------
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 {
```


================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:989
+  template <typename T> T Get(lldb::offset_t *offset_ptr) const {
+    constexpr size_t src_size = sizeof(T);
+
----------------
Make a local "T" here:
```
T val = fail_value;
constexpr size_t src_size = sizeof(T);
```


================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:996
+
+    assert(__builtin_is_aligned(src, alignof(T)));
+
----------------
remove this, no alignment is required.


================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:997
+    assert(__builtin_is_aligned(src, alignof(T)));
+
+    if (m_byte_order != endian::InlHostByteOrder()) {
----------------
Add a memcpy call:

```
memcpy(&val, src, src_size);
```


================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:999
+    if (m_byte_order != endian::InlHostByteOrder()) {
+      T val = 0.0;
+      uint8_t *dst_data = reinterpret_cast<uint8_t *>(&val);
----------------
remove since "val" was created above


================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:1000-1004
+      uint8_t *dst_data = reinterpret_cast<uint8_t *>(&val);
+      const uint8_t *src_data = reinterpret_cast<const uint8_t *>(src);
+      for (size_t i = 0; i < src_size; ++i)
+        dst_data[src_size - 1 - i] = src_data[i];
+      return val;
----------------
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.


================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:1004
+        dst_data[src_size - 1 - i] = src_data[i];
+      return val;
+    }
----------------
return "return val;" here


================
Comment at: lldb/include/lldb/Utility/DataExtractor.h:1007
+
+    return *src;
+  }
----------------
We can't rely on alignment, so return our local T value:
```
return val;
```


Repository:
  rLLDB LLDB

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

https://reviews.llvm.org/D83256