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