Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[lldb] Fix Scalar::GetData for non-multiple-of-8-bits values #90846

Merged
merged 1 commit into from
May 3, 2024

Conversation

labath
Copy link
Collaborator

@labath labath commented May 2, 2024

It was aligning the byte size down. Now it aligns up. This manifested itself as SBTypeStaticField::GetConstantValue returning a zero-sized value for bool fields (because clang represents bool as a 1-bit value).

I've changed the code for float Scalars as well, although I'm not aware of floating point values that are not multiples of 8 bits.

@labath labath requested a review from JDevlieghere as a code owner May 2, 2024 11:06
@llvmbot llvmbot added the lldb label May 2, 2024
@llvmbot
Copy link
Collaborator

llvmbot commented May 2, 2024

@llvm/pr-subscribers-lldb

Author: Pavel Labath (labath)

Changes

It was aligning the byte size down. Now it aligns up. This manifested itself as SBTypeStaticField::GetConstantValue returning a zero-sized value for bool fields (because clang represents bool as a 1-bit value).

I've changed the code for float Scalars as well, although I'm not aware of floating point values that are not multiples of 8 bits.


Full diff: https://github.com/llvm/llvm-project/pull/90846.diff

4 Files Affected:

  • (modified) lldb/source/Utility/Scalar.cpp (+2-2)
  • (modified) lldb/test/API/python_api/type/TestTypeList.py (+11)
  • (modified) lldb/test/API/python_api/type/main.cpp (+1)
  • (modified) lldb/unittests/Utility/ScalarTest.cpp (+30)
diff --git a/lldb/source/Utility/Scalar.cpp b/lldb/source/Utility/Scalar.cpp
index e94fd459623665..c70c5e10799187 100644
--- a/lldb/source/Utility/Scalar.cpp
+++ b/lldb/source/Utility/Scalar.cpp
@@ -134,9 +134,9 @@ size_t Scalar::GetByteSize() const {
   case e_void:
     break;
   case e_int:
-    return (m_integer.getBitWidth() / 8);
+    return (m_integer.getBitWidth() + 7) / 8;
   case e_float:
-    return m_float.bitcastToAPInt().getBitWidth() / 8;
+    return (m_float.bitcastToAPInt().getBitWidth() + 7) / 8;
   }
   return 0;
 }
diff --git a/lldb/test/API/python_api/type/TestTypeList.py b/lldb/test/API/python_api/type/TestTypeList.py
index 81c44f7a39d61a..4dbfb06f8fc58c 100644
--- a/lldb/test/API/python_api/type/TestTypeList.py
+++ b/lldb/test/API/python_api/type/TestTypeList.py
@@ -52,6 +52,17 @@ def _find_static_field_in_Task_pointer(self, task_pointer):
         self.DebugSBValue(value)
         self.assertEqual(value.GetValueAsSigned(), 47)
 
+        static_constexpr_bool_field = task_type.GetStaticFieldWithName(
+            "static_constexpr_bool_field"
+        )
+        self.assertTrue(static_constexpr_bool_field)
+        self.assertEqual(static_constexpr_bool_field.GetName(), "static_constexpr_bool_field")
+        self.assertEqual(static_constexpr_bool_field.GetType().GetName(), "const bool")
+
+        value = static_constexpr_bool_field.GetConstantValue(self.target())
+        self.DebugSBValue(value)
+        self.assertEqual(value.GetValueAsUnsigned(), 1)
+
         static_mutable_field = task_type.GetStaticFieldWithName("static_mutable_field")
         self.assertTrue(static_mutable_field)
         self.assertEqual(static_mutable_field.GetName(), "static_mutable_field")
diff --git a/lldb/test/API/python_api/type/main.cpp b/lldb/test/API/python_api/type/main.cpp
index c86644d918279a..7384a3d8da16fb 100644
--- a/lldb/test/API/python_api/type/main.cpp
+++ b/lldb/test/API/python_api/type/main.cpp
@@ -28,6 +28,7 @@ class Task {
     union U {
     } u;
     static constexpr long static_constexpr_field = 47;
+    static constexpr bool static_constexpr_bool_field = true;
     static int static_mutable_field;
     Task(int i, Task *n):
         id(i),
diff --git a/lldb/unittests/Utility/ScalarTest.cpp b/lldb/unittests/Utility/ScalarTest.cpp
index 8d957d16593ee7..500cb8bb2286e0 100644
--- a/lldb/unittests/Utility/ScalarTest.cpp
+++ b/lldb/unittests/Utility/ScalarTest.cpp
@@ -13,8 +13,11 @@
 #include "lldb/Utility/Scalar.h"
 #include "lldb/Utility/Status.h"
 #include "lldb/Utility/StreamString.h"
+#include "lldb/lldb-enumerations.h"
+#include "llvm/ADT/APSInt.h"
 #include "llvm/Testing/Support/Error.h"
 
+#include <algorithm>
 #include <cmath>
 
 using namespace lldb_private;
@@ -163,6 +166,33 @@ TEST(ScalarTest, GetBytes) {
   ASSERT_EQ(0, memcmp(f, Storage, sizeof(f)));
 }
 
+TEST(ScalarTest, GetData) {
+  auto get_data = [](llvm::APSInt v) {
+    DataExtractor data;
+    Scalar(v).GetData(data);
+    return data.GetData().vec();
+  };
+
+  auto vec = [](std::initializer_list<uint8_t> l) {
+    std::vector<uint8_t> v(l.begin(), l.end());
+    if (endian::InlHostByteOrder() == lldb::eByteOrderLittle)
+      std::reverse(v.begin(), v.end());
+    return v;
+  };
+
+  EXPECT_THAT(
+      get_data(llvm::APSInt::getMaxValue(/*numBits=*/1, /*Unsigned=*/true)),
+      vec({0x01}));
+
+  EXPECT_THAT(
+      get_data(llvm::APSInt::getMaxValue(/*numBits=*/8, /*Unsigned=*/true)),
+      vec({0xff}));
+
+  EXPECT_THAT(
+      get_data(llvm::APSInt::getMaxValue(/*numBits=*/9, /*Unsigned=*/true)),
+      vec({0x01, 0xff}));
+}
+
 TEST(ScalarTest, SetValueFromData) {
   uint8_t a[] = {1, 2, 3, 4};
   Scalar s;

It was aligning the byte size down. Now it aligns up. This manifested
itself as SBTypeStaticField::GetConstantValue returning a zero-sized
value for `bool` fields (because clang represents bool as a 1-bit value).

I've changed the code for float Scalars as well, although I'm not aware
of floating point values that are not multiples of 8 bits.
Copy link
Collaborator

@DavidSpickett DavidSpickett left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM

@labath labath merged commit e450f98 into llvm:main May 3, 2024
4 checks passed
@labath labath deleted the bool branch June 7, 2024 10:51
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

3 participants