[Lldb-commits] [lldb] d0fa52c - [lldb] Rewrite Scalar::GetBytes
Pavel Labath via lldb-commits
lldb-commits at lists.llvm.org
Thu Jun 25 06:31:57 PDT 2020
Author: Pavel Labath
Date: 2020-06-25T15:31:48+02:00
New Revision: d0fa52cc3797fd8805d24a04e6b8198154cd7b53
URL: https://github.com/llvm/llvm-project/commit/d0fa52cc3797fd8805d24a04e6b8198154cd7b53
DIFF: https://github.com/llvm/llvm-project/commit/d0fa52cc3797fd8805d24a04e6b8198154cd7b53.diff
LOG: [lldb] Rewrite Scalar::GetBytes
This function was modifying and returning pointers to static storage,
which meant that any two accesses to different Scalar objects could
potentially race (depending on which types the objects were storing and
the host endianness).
In the new version the user is responsible for providing a buffer into
which this method will store its binary representation. The main caller
(RegisterValue::GetBytes) already has one such buffer handy, so this did
not require any major rewrites.
To make that work, I've needed to mark the RegisterValue value buffer
mutable -- not an ideal solution, but definitely better than modifying
global storage. This could be further improved by changing
RegisterValue::GetBytes to take a buffer too.
Added:
Modified:
lldb/include/lldb/Utility/RegisterValue.h
lldb/include/lldb/Utility/Scalar.h
lldb/source/Utility/RegisterValue.cpp
lldb/source/Utility/Scalar.cpp
lldb/unittests/Utility/ScalarTest.cpp
Removed:
################################################################################
diff --git a/lldb/include/lldb/Utility/RegisterValue.h b/lldb/include/lldb/Utility/RegisterValue.h
index 494f8be5391c..c9f295a8d95a 100644
--- a/lldb/include/lldb/Utility/RegisterValue.h
+++ b/lldb/include/lldb/Utility/RegisterValue.h
@@ -260,8 +260,9 @@ class RegisterValue {
Scalar m_scalar;
struct {
- uint8_t bytes[kMaxRegisterByteSize]; // This must be big enough to hold any
- // register for any supported target.
+ mutable uint8_t
+ bytes[kMaxRegisterByteSize]; // This must be big enough to hold any
+ // register for any supported target.
uint16_t length;
lldb::ByteOrder byte_order;
} buffer;
diff --git a/lldb/include/lldb/Utility/Scalar.h b/lldb/include/lldb/Utility/Scalar.h
index 203ba565b223..eda7528a9212 100644
--- a/lldb/include/lldb/Utility/Scalar.h
+++ b/lldb/include/lldb/Utility/Scalar.h
@@ -106,7 +106,10 @@ class Scalar {
bool ClearBit(uint32_t bit);
- const void *GetBytes() const;
+ /// Store the binary representation of this value into the given storage.
+ /// Exactly GetByteSize() bytes will be stored, and the buffer must be large
+ /// enough to hold this data.
+ void GetBytes(llvm::MutableArrayRef<uint8_t> storage) const;
size_t GetByteSize() const;
diff --git a/lldb/source/Utility/RegisterValue.cpp b/lldb/source/Utility/RegisterValue.cpp
index 91f4025c923c..7f545c214a4c 100644
--- a/lldb/source/Utility/RegisterValue.cpp
+++ b/lldb/source/Utility/RegisterValue.cpp
@@ -728,7 +728,8 @@ const void *RegisterValue::GetBytes() const {
case eTypeFloat:
case eTypeDouble:
case eTypeLongDouble:
- return m_scalar.GetBytes();
+ m_scalar.GetBytes(buffer.bytes);
+ return buffer.bytes;
case eTypeBytes:
return buffer.bytes;
}
diff --git a/lldb/source/Utility/Scalar.cpp b/lldb/source/Utility/Scalar.cpp
index 748dd5541908..b34311b5ef3e 100644
--- a/lldb/source/Utility/Scalar.cpp
+++ b/lldb/source/Utility/Scalar.cpp
@@ -7,7 +7,7 @@
//===----------------------------------------------------------------------===//
#include "lldb/Utility/Scalar.h"
-
+#include "lldb/Utility/DataBufferHeap.h"
#include "lldb/Utility/DataExtractor.h"
#include "lldb/Utility/Endian.h"
#include "lldb/Utility/Status.h"
@@ -73,36 +73,36 @@ Scalar::Scalar() : m_type(e_void), m_float(static_cast<float>(0)) {}
bool Scalar::GetData(DataExtractor &data, size_t limit_byte_size) const {
size_t byte_size = GetByteSize();
- if (byte_size > 0) {
- const uint8_t *bytes = static_cast<const uint8_t *>(GetBytes());
-
- if (limit_byte_size < byte_size) {
- if (endian::InlHostByteOrder() == eByteOrderLittle) {
- // On little endian systems if we want fewer bytes from the current
- // type we just specify fewer bytes since the LSByte is first...
- byte_size = limit_byte_size;
- } else if (endian::InlHostByteOrder() == eByteOrderBig) {
- // On big endian systems if we want fewer bytes from the current type
- // have to advance our initial byte pointer and trim down the number of
- // bytes since the MSByte is first
- bytes += byte_size - limit_byte_size;
- byte_size = limit_byte_size;
- }
+ if (byte_size == 0) {
+ data.Clear();
+ return false;
+ }
+ auto buffer_up = std::make_unique<DataBufferHeap>(byte_size, 0);
+ GetBytes(buffer_up->GetData());
+ lldb::offset_t offset = 0;
+
+ if (limit_byte_size < byte_size) {
+ if (endian::InlHostByteOrder() == eByteOrderLittle) {
+ // On little endian systems if we want fewer bytes from the current
+ // type we just specify fewer bytes since the LSByte is first...
+ byte_size = limit_byte_size;
+ } else if (endian::InlHostByteOrder() == eByteOrderBig) {
+ // On big endian systems if we want fewer bytes from the current type
+ // have to advance our initial byte pointer and trim down the number of
+ // bytes since the MSByte is first
+ offset = byte_size - limit_byte_size;
+ byte_size = limit_byte_size;
}
-
- data.SetData(bytes, byte_size, endian::InlHostByteOrder());
- return true;
}
- data.Clear();
- return false;
+
+ data.SetData(std::move(buffer_up), offset, byte_size);
+ data.SetByteOrder(endian::InlHostByteOrder());
+ return true;
}
-const void *Scalar::GetBytes() const {
- const uint64_t *apint_words;
- const uint8_t *bytes;
- static float_t flt_val;
- static double_t dbl_val;
- static uint64_t swapped_words[8];
+void Scalar::GetBytes(llvm::MutableArrayRef<uint8_t> storage) const {
+ assert(storage.size() >= GetByteSize());
+
switch (m_type) {
case e_void:
break;
@@ -112,73 +112,31 @@ const void *Scalar::GetBytes() const {
case e_ulong:
case e_slonglong:
case e_ulonglong:
- bytes = reinterpret_cast<const uint8_t *>(m_integer.getRawData());
- // getRawData always returns a pointer to an uint64_t. If we have a
- // smaller type, we need to update the pointer on big-endian systems.
- if (endian::InlHostByteOrder() == eByteOrderBig) {
- size_t byte_size = m_integer.getBitWidth() / 8;
- if (byte_size < 8)
- bytes += 8 - byte_size;
- }
- return bytes;
- // getRawData always returns a pointer to an array of uint64_t values,
- // where the least-significant word always comes first. On big-endian
- // systems we need to swap the words.
case e_sint128:
case e_uint128:
- apint_words = m_integer.getRawData();
- if (endian::InlHostByteOrder() == eByteOrderBig) {
- swapped_words[0] = apint_words[1];
- swapped_words[1] = apint_words[0];
- apint_words = swapped_words;
- }
- return static_cast<const void *>(apint_words);
case e_sint256:
case e_uint256:
- apint_words = m_integer.getRawData();
- if (endian::InlHostByteOrder() == eByteOrderBig) {
- swapped_words[0] = apint_words[3];
- swapped_words[1] = apint_words[2];
- swapped_words[2] = apint_words[1];
- swapped_words[3] = apint_words[0];
- apint_words = swapped_words;
- }
- return static_cast<const void *>(apint_words);
case e_sint512:
case e_uint512:
- apint_words = m_integer.getRawData();
- if (endian::InlHostByteOrder() == eByteOrderBig) {
- swapped_words[0] = apint_words[7];
- swapped_words[1] = apint_words[6];
- swapped_words[2] = apint_words[5];
- swapped_words[3] = apint_words[4];
- swapped_words[4] = apint_words[3];
- swapped_words[5] = apint_words[2];
- swapped_words[6] = apint_words[1];
- swapped_words[7] = apint_words[0];
- apint_words = swapped_words;
- }
- return static_cast<const void *>(apint_words);
- case e_float:
- flt_val = m_float.convertToFloat();
- return static_cast<const void *>(&flt_val);
- case e_double:
- dbl_val = m_float.convertToDouble();
- return static_cast<const void *>(&dbl_val);
- case e_long_double:
- llvm::APInt ldbl_val = m_float.bitcastToAPInt();
- apint_words = ldbl_val.getRawData();
- // getRawData always returns a pointer to an array of two uint64_t values,
- // where the least-significant word always comes first. On big-endian
- // systems we need to swap the two words.
- if (endian::InlHostByteOrder() == eByteOrderBig) {
- swapped_words[0] = apint_words[1];
- swapped_words[1] = apint_words[0];
- apint_words = swapped_words;
- }
- return static_cast<const void *>(apint_words);
+ StoreIntToMemory(m_integer, storage.data(),
+ (m_integer.getBitWidth() + 7) / 8);
+ break;
+ case e_float: {
+ float val = m_float.convertToFloat();
+ memcpy(storage.data(), &val, sizeof(val));
+ break;
+ }
+ case e_double: {
+ double val = m_float.convertToDouble();
+ memcpy(storage.data(), &val, sizeof(double));
+ break;
+ }
+ case e_long_double: {
+ llvm::APInt val = m_float.bitcastToAPInt();
+ StoreIntToMemory(val, storage.data(), storage.size());
+ break;
+ }
}
- return nullptr;
}
size_t Scalar::GetByteSize() const {
diff --git a/lldb/unittests/Utility/ScalarTest.cpp b/lldb/unittests/Utility/ScalarTest.cpp
index be23320a045a..960161c01bac 100644
--- a/lldb/unittests/Utility/ScalarTest.cpp
+++ b/lldb/unittests/Utility/ScalarTest.cpp
@@ -78,6 +78,7 @@ TEST(ScalarTest, RightShiftOperator) {
}
TEST(ScalarTest, GetBytes) {
+ uint8_t Storage[256];
int a = 0x01020304;
long long b = 0x0102030405060708LL;
float c = 1234567.89e32f;
@@ -99,14 +100,20 @@ TEST(ScalarTest, GetBytes) {
sizeof(void *));
Status f_error =
f_scalar.SetValueFromData(f_data, lldb::eEncodingUint, sizeof(f));
- ASSERT_EQ(0, memcmp(&a, a_scalar.GetBytes(), sizeof(a)));
- ASSERT_EQ(0, memcmp(&b, b_scalar.GetBytes(), sizeof(b)));
- ASSERT_EQ(0, memcmp(&c, c_scalar.GetBytes(), sizeof(c)));
- ASSERT_EQ(0, memcmp(&d, d_scalar.GetBytes(), sizeof(d)));
+ a_scalar.GetBytes(Storage);
+ ASSERT_EQ(0, memcmp(&a, Storage, sizeof(a)));
+ b_scalar.GetBytes(Storage);
+ ASSERT_EQ(0, memcmp(&b, Storage, sizeof(b)));
+ c_scalar.GetBytes(Storage);
+ ASSERT_EQ(0, memcmp(&c, Storage, sizeof(c)));
+ d_scalar.GetBytes(Storage);
+ ASSERT_EQ(0, memcmp(&d, Storage, sizeof(d)));
ASSERT_EQ(0, e_error.Fail());
- ASSERT_EQ(0, memcmp(e, e_scalar.GetBytes(), sizeof(e)));
+ e_scalar.GetBytes(Storage);
+ ASSERT_EQ(0, memcmp(e, Storage, sizeof(e)));
ASSERT_EQ(0, f_error.Fail());
- ASSERT_EQ(0, memcmp(f, f_scalar.GetBytes(), sizeof(f)));
+ f_scalar.GetBytes(Storage);
+ ASSERT_EQ(0, memcmp(f, Storage, sizeof(f)));
}
TEST(ScalarTest, CastOperations) {
@@ -137,21 +144,21 @@ TEST(ScalarTest, ExtractBitfield) {
long long b1 = 0xff1f2f3f4f5f6f7fLL;
Scalar s_scalar(a1);
ASSERT_TRUE(s_scalar.ExtractBitfield(0, 0));
- ASSERT_EQ(0, memcmp(&a1, s_scalar.GetBytes(), sizeof(a1)));
+ EXPECT_EQ(s_scalar, a1);
ASSERT_TRUE(s_scalar.ExtractBitfield(len, 0));
- ASSERT_EQ(0, memcmp(&a1, s_scalar.GetBytes(), sizeof(a1)));
+ EXPECT_EQ(s_scalar, a1);
ASSERT_TRUE(s_scalar.ExtractBitfield(len - 4, 4));
- ASSERT_EQ(0, memcmp(&b1, s_scalar.GetBytes(), sizeof(b1)));
+ EXPECT_EQ(s_scalar, b1);
unsigned long long a2 = 0xf1f2f3f4f5f6f7f8ULL;
unsigned long long b2 = 0x0f1f2f3f4f5f6f7fULL;
Scalar u_scalar(a2);
ASSERT_TRUE(u_scalar.ExtractBitfield(0, 0));
- ASSERT_EQ(0, memcmp(&a2, u_scalar.GetBytes(), sizeof(a2)));
+ EXPECT_EQ(u_scalar, a2);
ASSERT_TRUE(u_scalar.ExtractBitfield(len, 0));
- ASSERT_EQ(0, memcmp(&a2, u_scalar.GetBytes(), sizeof(a2)));
+ EXPECT_EQ(u_scalar, a2);
ASSERT_TRUE(u_scalar.ExtractBitfield(len - 4, 4));
- ASSERT_EQ(0, memcmp(&b2, u_scalar.GetBytes(), sizeof(b2)));
+ EXPECT_EQ(u_scalar, b2);
}
template <typename T> static std::string ScalarGetValue(T value) {
More information about the lldb-commits
mailing list