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
_______________________________________________
lldb-commits mailing list
lldb-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/lldb-commits
